Who's using what where - turning code into symbols

The Rubberduck project is coming along pretty nicely, and for the next version the parsing strategy is getting yet another revision, and this time around it really feels like it's done right.

Or is it?

The idea is to walk the parse tree once to locate every single declaration.

Here are the types of declarations we're talking about:

namespace Rubberduck.Parsing.Symbols
{
    public enum DeclarationType
    {
        Module,
        Class,
        Procedure,
        Function,
        PropertyGet,
        PropertyLet,
        PropertySet,
        Parameter,
        Variable,
        Constant,
        Enumeration,
        EnumerationMember,
        Event,
        UserDefinedType,
        UserDefinedTypeMember,
        LibraryFunction
    }
}

And here's what a Declaration looks like:

```
namespace Rubberduck.Parsing.Symbols
{
///
/// Defines a declared identifier.
///
public class Declaration
{
public Declaration(QualifiedMemberName qualifiedName, string parentScope,
string identifierName, string asTypeName, bool isSelfAssigned,
Accessibility accessibility, DeclarationType declarationType, ParserRuleContext context)
{
_qualifiedName = qualifiedName;
_parentScope = parentScope;
_identifierName = identifierName;
_asTypeName = asTypeName;
_isSelfAssigned = isSelfAssigned;
_accessibility = accessibility;
_declarationType = declarationType;
_selection = context.GetSelection();
_context = context;
}

private readonly QualifiedMemberName _qualifiedName;
public QualifiedMemberName QualifiedName { get { return _qualifiedName; } }

private readonly ParserRuleContext _context;
public ParserRuleContext Context { get { return _context; } }

private readonly IList _references = new List();
public IEnumerable References { get { return _references;

I can't see it because of some missing classes but are the types being returned also immutable? You've made types like Declaration nicely immutable but if the complex objects themselves are mutable then that is still worrisome.

Unless you want this behaviour so you can easily change semantics, but I doubt it considering that would cause a lot of inconsistencies.

public override bool Equals(object obj)
{
    if (!(obj is Declaration))
    {
        return false;
    }

    return GetHashCode() == ((Declaration)obj).GetHashCode();
}

Now this is something I don't like. First of all there's the obvious: you are casting twice. Instead use the as + == null idiom to only use one cast.

However what really stubs my pinkytoe is the fact that you are implementing equality in your GetHashCode() which is against all conventions, including the one from Geneva.

Two equal objects are supposed to return true for Equals(). Two distinct objects are supposed to return false for Equals(). Two distinct objects can have the same GetHashCode(). By definition this means that you would consider two objects as equal when they aren't; they just have the same hashcode.

Implementing equality in your GetHashCode() instead is not an excuse, that's on par with genocide.

public override int GetHashCode()
{
    return string.Concat(ProjectHashCode.ToString(), ProjectName, ComponentName, _parentScope, _identifierName).GetHashCode();
}

I like the approach taken here although I'm not sure how scientifically correct this is. More common is to use primes and stuff.

private readonly string _projectName;
public string ProjectName { get { return _projectName; } }

These can be simply turned into

public string ProjectName { get; private set; }

I assume you did this to preserve uniformity with the Selection property? I would rather look into making Selection immutable, once again.

EnterSubStmt

Bleigh.

// a field isn't a field if it's not a variable or a constant.
if (declaration.DeclarationType != DeclarationType.Variable ||
    declaration.DeclarationType != DeclarationType.Constant)
{
    return false;
}

Boolean algebra! This should be &&. Right now it will return true if the type is a variable or a constant or anything else.

It would be useful to write this in the positive sense: "a field is a field if it's a variable or a constant".

This same idea is behind the following snippet:

// a procedure is global if it's a Sub or Function (properties are never global).
// since we have no visibility on module attributes,
// we must assume a class member can be called from a default instance.

if (declaration.DeclarationType != DeclarationType.Procedure ||
declaration.DeclarationType != DeclarationType.Function)
{
    return false;
}

return isSameProject && declaration.Accessibility == Accessibility.Friend
               || declaration.Accessibility != Accessibility.Private;

Personally I would prefer to have some brackets in here to clarify the order of operations, simply because this stuff is easy to make mistakes in.

return parentContext is VBAParser.VariableSubStmtContext
    || parentContext is VBAParser.ConstSubStmtContext
    || parentContext is VBAParser.ArgContext
    || parentContext is VBAParser.SubStmtContext
    || parentContext is VBAParser.FunctionStmtContext
    || parentContext is VBAParser.PropertyGetStmtContext
    || parentContext is VBAParser.PropertyLetStmtContext
    || parentContext is VBAParser.PropertySetStmtContext
    || parentContext is VBAParser.TypeStmtContext
    || parentContext is VBAParser.TypeStmt_ElementContext
    || parentContext is VBAParser.EnumerationStmtContext
    || parentContext is VBAParser.EnumerationStmt_ConstantContext
    || parentContext is VBAParser.DeclareStmtContext
    || parentContext is VBAParser.EventStmtContext;

Instead of doing all these casts, I would just create an anonymous array with all the types and checking if it contains the type of parentContext.

In response to the request for unit-testing: if you can create your own Declarations objects then you're already good to go. Perhaps create small snippets of code, pull them through ANTLR and use them to verify the workings of each property/method. The thing about unit-testing code-trees like this is that your tests will mainly be about testing small snippets of code and seeing if they're parsed correctly; you'll see the same approach in the Roslyn source.

If you do want to test the private methods, you can use a PrivateObject to expose them.

StackExchange Code Review Q#84888, answer score: 12