Rubberduck: Clean up declarations

[Hosch250]

This story is about cleaning up the declarations. Please comment on and extend the design until we are sure it will work before implementing.

So far, I have this system:

public abstract class SyntaxNode
{
    public ParserNodeContext { get; set; }    // Define the qualified selection on the context
    public SyntaxNode Parent { get; set; }
}

public abstract class DeclarationSyntaxNode
{
    public List<IdentifierReference> References { get; set; }
    public VBAParser.AsTypeClauseContext AsTypeContext { get; set;}
    public Accessibility Accessibility { get; set; }
    public bool IsUndeclared { get; set; }
    public string IdentifierName
}

public class ModuleSyntax : DeclarationSyntaxNode
{
    public List<SyntaxNode> ChildDeclarations { get; set; }
}

public class MemberSyntax : DeclarationSyntaxNode
{
    List<ParameterSyntax> Parameters { get; set; }
}

public class VariableSyntax : DeclarationSyntaxNode
{
    // variable, field
}

public class ParameterSyntax : DeclarationSyntaxNode
{
    public PassByModifier PassByModifier { get; set; }    // implicit, byval, byref
}

public abstract class StatementSyntaxNode
{
    // todo: eventually implement a bunch of these for assignments, comparisons, etc.
    // to make it easier than mucking around with the ANTLR parse tree
    // bye bye, handling whitespace and line continuations :)
}

[Hosch250]

It was brought up in the chat that we shouldn't kill QualifiedModuleName because it maps a declaration to a component, etc. I agree with this, but I also think A) it shouldn't be exposed to anything but the part of the code that actually needs it (i.e., the rewriter), and B) it should only be on the project/module level nodes in a custom implementation--if we need this information from a lower-level declaration, we can just climb back up the tree to the parent module/project.

[Vogel612]

TBH traversing the node-hierarchy seems like a great place to use Extension Methods

[MDoerner]

First, to clarify on the statement about the QualifiedModuleName: it is not about mapping a declaration to a component. It is a proxy for a component that reduces COM access by caching the most important properties, in particular those needed to identify a component.

In my humble opinion, we should not ban the QualifiedModuleName from the QualifiedMemberName on the declaration, but refactor split it in two parts. In 90% of all uses, we only need the cached information. So, my proposal is to introduce a QualifiedComponent consisting of a QualifiedModuleName and the IVBComponent and to remove the reference to teh component from the QualifiedModuleName. But that is different issue than cleaning up the declaration.

Now, regarding the design, II do not see why we should rewrite the declaration design; refactoring it a bit would be more appropriate in my humble opinion. Let me explain why.

The first reason I would prefer a refectoring compared to a rewrite is that about 80% of RD actually depends on the declarations. So, rewriting would be a huge pain and risk.
My second reason is that the design proposed is already there apart from the StatementSyntaxNode and the list of child declarations on the ModuleSyntax. (Currently one has to go through the DeclarationFinder to get them.) Moreover, the proposed new design so far does not deal with a lot of information we collect and use, like the attributes, annotations, the information whether the declaration is user-defined, etc.

From my point of view, the main problem with the current Declaration class is that some callers pushed their concerns into the class, in particular the COM collector. E.g. all the constructors dealing with COM declarations ought to be factory calls in the namespace of the COM collector. It is not a concern of the declaration how it gets constructed from this special data.

The second problem I see with the design currently in place is that we persist some information on declarations that should probably be collected from other declarations, e.g. CustomFolder which should just work like Project, i.e. it should get its result from the parentDeclaration unless it is a ClassDeclaration, ProcduralModuleDeclaration or a ProjectDeclaration.

Finally, the current design lacks some declaration types, noy in the sense of the enum but as classes in the hierarchy. Two classes missing are a ModuleLevelVariableDeclaration and a LocalVariableDeclaration, whose existence should allow to make Declaration abstract. (Currently, variable declarations are plain declarations.)
Moreover, it would probably beneficial to add some abstract intermediary classes for declaration types with shared concerns like ModuleBodyElementDeclaration (Function, Sub, PropertyGet, PropertyLet, PropertySet), as kin of also proposed above.

To sum it up, I would suggest to refactor the current design instead of a rewrite. The two primary reasons being the following:

1) We could first change it without changing the interface of Declaration and thus only affect the constructing side first, and deal with the callers later.

2) The current design is not as bad as it looks: the basic structure of the proposed design above is already there; the structure just has to be imposed more consistently and some pollution by callers has to be pushed back to them.