10
\$\begingroup\$

Building on my ANTLR tree listener, I'm now starting to see how the whole thing is coming together.

As I proceed to implement the numerous Node classes I'm going to need to make sense out of the ANTLR parse tree, I feel like I'm doing more and more weird things.. oh, the tests pass. But the implementations... here, have a look:

ProcedureNode

Used for Sub, Function, Property Get, Property Let and Property Set code blocks, this node can have children and defines a scope.

public class ProcedureNode : Node
{
    public enum VBProcedureKind
    {
        Sub,
        Function,
        PropertyGet,
        PropertyLet,
        PropertySet
    }

    public ProcedureNode(VisualBasic6Parser.PropertySetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertySet, context.visibility(), context.ambiguousIdentifier(), null)
    {
    }

    public ProcedureNode(VisualBasic6Parser.PropertyLetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertyLet, context.visibility(), context.ambiguousIdentifier(), null)
    {
    }

    public ProcedureNode(VisualBasic6Parser.PropertyGetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertyGet, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
    {
    }

    public ProcedureNode(VisualBasic6Parser.FunctionStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.Function, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
    {
    }

    public ProcedureNode(VisualBasic6Parser.SubStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.Sub, context.visibility(), context.ambiguousIdentifier(), null)
    {
    }

    private ProcedureNode(ParserRuleContext context, string scope, string localScope, 
                          VBProcedureKind kind, 
                          VisualBasic6Parser.VisibilityContext visibility, 
                          VisualBasic6Parser.AmbiguousIdentifierContext name, 
                          Func<VisualBasic6Parser.AsTypeClauseContext> asType)
        : base(context, scope, localScope)
    {
        _kind = kind;
        _name = name.GetText();
        _accessibility = visibility.GetAccessibility();

        if (asType != null)
        {
            var returnTypeClause = asType();
            _isImplicitReturnType = returnTypeClause == null;

            _returnType = returnTypeClause == null 
                            ? ReservedKeywords.Variant 
                            : returnTypeClause.type().GetText();
        }
    }

    private readonly string _name;
    public string Name { get { return _name; } }

    private readonly string _returnType;
    public string ReturnType { get { return _returnType; } }

    private readonly bool _isImplicitReturnType;
    public bool IsImplicitReturnType { get { return _isImplicitReturnType; } }

    private readonly VBProcedureKind _kind;
    public VBProcedureKind Kind { get { return _kind; } }

    private readonly VBAccessibility _accessibility;
    public VBAccessibility Accessibility { get { return _accessibility; } }
}

I'm making use of a few extension methods here, extending some ParserRuleContext (and auto-generated derived types):

public static class ParserRuleContextExtensions
{
    public static Selection GetSelection(this ParserRuleContext context)
    {
        if (context == null)
            return Selection.Empty;

        // adding +1 because ANTLR indexes are 0-based, but VBE's are 1-based.
        return new Selection(
            context.Start.Line + 1,
            context.Start.StartIndex + 1, // todo: figure out why this is off and how to fix it
            context.Stop.Line + 1,
            context.Stop.StopIndex + 1); // todo: figure out why this is off and how to fix it
    }

    public static VBAccessibility GetAccessibility(this VisualBasic6Parser.VisibilityContext context)
    {
        if (context == null)
            return VBAccessibility.Implicit;

        return (VBAccessibility) Enum.Parse(typeof (VBAccessibility), context.GetText());
    }
}

VariableDeclarationNode

VBA variables can be declared in various ways; a declaration statement can actually declare more than a single variable. Hence, this node has child nodes too (VariableNode instances), but doesn't define a scope:

public class VariableDeclarationNode : Node
{
    public VariableDeclarationNode(VisualBasic6Parser.VariableStmtContext context, string scope)
        :base(context, scope, null, new List<Node>())
    {
        foreach (var variable in context.variableListStmt().variableSubStmt())
        {
            AddChild(new VariableNode(variable, scope, context.visibility(), context.DIM() != null || context.STATIC() != null));
        }
    }
}

public class VariableNode : Node
{
    private static readonly IDictionary<string, string> TypeSpecifiers = new Dictionary<string, string>
    {
        { "%", ReservedKeywords.Integer },
        { "&", ReservedKeywords.Long },
        { "@", ReservedKeywords.Decimal },
        { "!", ReservedKeywords.Single },
        { "#", ReservedKeywords.Double },
        { "$", ReservedKeywords.String }
    };

    public VariableNode(VisualBasic6Parser.VariableSubStmtContext context, string scope, 
                        VisualBasic6Parser.VisibilityContext visibility, bool isLocal = true)
        : base(context, scope)
    {
        _name = context.ambiguousIdentifier().GetText();
        if (context.asTypeClause() == null)
        {
            if (context.typeHint() == null)
            {
                _isImplicitlyTyped = true;
                _typeName = ReservedKeywords.Variant;
            }
            else
            {
                var hint = context.typeHint().GetText();
                _isUsingTypeHint = true;
                _typeName = TypeSpecifiers[hint];
            }
        }
        else
        {
            _typeName = context.asTypeClause().type().GetText();
        }

        _accessibility = isLocal ? VBAccessibility.Private : visibility.GetAccessibility();
    }

    private readonly string _name;
    public string Name { get { return _name; } }

    private readonly string _typeName;
    public string TypeName { get { return _typeName; } }

    private readonly bool _isImplicitlyTyped;
    public bool IsImplicitlyTyped { get { return _isImplicitlyTyped; } }

    private bool _isUsingTypeHint;
    public bool IsUsingTypeHint { get { return _isUsingTypeHint; } }

    private readonly VBAccessibility _accessibility;
    public VBAccessibility Accessibility { get { return _accessibility; } }
}

Here are a few tests showing how the parser is ultimately used and how nodes are ultimately accessed:

    [TestMethod]
    public void UnspecifiedProcedureVisibilityIsImplicit()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Sub Foo()\n    Dim bar As Integer\nEnd Sub";

        var module = parser.Parse("project", "component", code);
        var procedure = (ProcedureNode)module.Children.First();

        Assert.AreEqual(procedure.Accessibility, VBAccessibility.Implicit);
    }

    [TestMethod]
    public void UnspecifiedReturnTypeGetsFlagged()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Function Foo()\n    Dim bar As Integer\nEnd Function";

        var module = parser.Parse("project", "component", code);
        var procedure = (ProcedureNode)module.Children.First();

        Assert.AreEqual(procedure.ReturnType, "Variant");            
        Assert.IsTrue(procedure.IsImplicitReturnType);
    }

    [TestMethod]
    public void LocalDimMakesPrivateVariable()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Sub Foo()\n    Dim bar As Integer\nEnd Sub";

        var module = parser.Parse("project", "component", code);
        var procedure = module.Children.First();
        var declaration = procedure.Children.First();
        var variable = (VariableNode)declaration.Children.First();

        Assert.AreEqual(variable.Accessibility, VBAccessibility.Private);
    }

    [TestMethod]
    public void TypeHintsGetFlagged()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Sub Foo()\n    Dim bar$\nEnd Sub";

        var module = parser.Parse("project", "component", code);
        var procedure = module.Children.First();
        var declaration = procedure.Children.First();
        var variable = (VariableNode)declaration.Children.First();

        Assert.IsTrue(variable.IsUsingTypeHint);
    }

    [TestMethod]
    public void ImplicitTypeGetsFlagged()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Sub Foo()\n    Dim bar\nEnd Sub";

        var module = parser.Parse("project", "component", code);
        var procedure = module.Children.First();
        var declaration = procedure.Children.First();
        var variable = (VariableNode)declaration.Children.First();

        Assert.IsTrue(variable.IsImplicitlyTyped);
    }
\$\endgroup\$
4
\$\begingroup\$

Avoid early and perhaps useless enums:

public enum VBProcedureKind
{
    Sub,
    Function,
    PropertyGet,
    PropertyLet,
    PropertySet
}

Enums are perhaps one of the most annoying, exploited, anti-pattern, anti oop, that no language should ever support kind of thing and ruins every programmer carrer. This was in fact so bad idea that you had a moment of dissatisfaction and started to do bad code:

public ProcedureNode(VisualBasic6Parser.PropertySetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertySet, context.visibility(), context.ambiguousIdentifier(), null)
{
}

public ProcedureNode(VisualBasic6Parser.PropertyLetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertyLet, context.visibility(), context.ambiguousIdentifier(), null)
{
}

public ProcedureNode(VisualBasic6Parser.PropertyGetStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.PropertyGet, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
{
}

public ProcedureNode(VisualBasic6Parser.FunctionStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.Function, context.visibility(), context.ambiguousIdentifier(), context.asTypeClause)
{
}

public ProcedureNode(VisualBasic6Parser.SubStmtContext context, string scope, string localScope)
        : this(context, scope, localScope, VBProcedureKind.Sub, context.visibility(), context.ambiguousIdentifier(), null)
{
}

You can only be unhappy after seeing/writing this. One other thing that may be wrong about your ctors is the fact that you're using a object to pass derivative values eg context.visibility(). Everything I mentioned would imply that your ProcedureNode could have the following signature:

private ProcedureNode(ParserRuleContext context, string scope, string localScope)

In fact this would be your only ctor. This is obviously very cute and everything but now we have a problem to solve, don't we? We need to identify which type of node we have, we also have to make some calls that are available in only some of the classes, etc...

Let those classes: PropertySetStmtContext, PropertyLetStmtContext, PropertyGetStmtContext, FunctionStmtContext implement an Interface that have the following properties:

interface IMemberContext{//maybe you can find a more suitable name
    VisibilityContext Visibility{get;}
    AmbiguousIdentifierContext Name{get;}
    Func<VisualBasic6Parser.AsTypeClauseContext> AsType{get;} //maybe you can return the result, instead of returning the function
} 

And just use that in your ProcedureNode class:

public class ProcedureNode : Node
{
    publlic ProcedureNode(IMemberContext memberContext, string scope, string localScope)
        : base((ParserRuleContext)memberContext, scope, localScope)
    {
        if (memberContext.AsType != null)
        {
            var returnTypeClause = memberContext.AsType();
            IsImplicitReturnType = returnTypeClause == null;

            ReturnType = returnTypeClause == null 
                            ? ReservedKeywords.Variant 
                            : returnTypeClause.type().GetText();
        }
    }
    public IMemberContext MemberContext{get {return (IMemberContext)base.Context;}}
    public string ReturnType { get; private set;}

    public bool IsImplicitReturnType { get; private set; }
}

And magic is done, all the mess is gone. Let the Context object be the majesty of your class, let it proudly be the object that you may easily inspect to get those nice properties values while debugging, let it rule your class ;)!

I don't know about the details of your project but let's imagine that you could say: "But it doesn't make sense that my ParserRuleContext implements that interface, because not all rules have accessibility etc...". You could also pass the same object twice in the same ctor, one time for the rule, other time for the Context:

public class ProcedureNode : Node
{
    publlic ProcedureNode(ParserRuleContext ruleContext, IMemberContext memberContext, string scope, string localScope)
        : base(ruleContext, scope, localScope)
    {
        MemberContext = memberContext;
        if (memberContext.AsType != null)
        {
            var returnTypeClause = memberContext.AsType();
            IsImplicitReturnType = returnTypeClause == null;

            ReturnType = returnTypeClause == null 
                            ? ReservedKeywords.Variant 
                            : returnTypeClause.type().GetText();
        }
    }
    public IMemberContext MemberContext{get; private set;}
    public string ReturnType { get; private set;}
    public bool IsImplicitReturnType { get; private set; }
}

Not the best, but still it's way better than your previous attempt IMHO.

\$\endgroup\$
10
  • \$\begingroup\$ The XxxxContext classes are generated by ANTLR, are you suggesting to edit the generated code? (I'd to it, but...) \$\endgroup\$ – Mathieu Guindon Jan 26 '15 at 13:03
  • \$\begingroup\$ @Mat'sMug are they generated with the keyword partial? If so then it's simple, if not you can always inherit from them... If they are generated with the keyword sealed then you can Create a class that implements the interface and has a object of the type generated... \$\endgroup\$ – Bruno Costa Jan 26 '15 at 13:20
  • \$\begingroup\$ They're neither sealed nor partial; and if I pass in a type that's common to all, then I have none of the methods that enable me to identify whether I'm dealing with a function or getter (with an AsTypeClauseContext), or a sub or setter (without a return type)... \$\endgroup\$ – Mathieu Guindon Jan 26 '15 at 13:56
  • \$\begingroup\$ @Mat'sMug It is a shame that the classes aren't generated with the keyword partial here. But I don't see the problem with the functions and setters, their implementation could return null if/when necessary. Is there a place where can I see all the generated classes? \$\endgroup\$ – Bruno Costa Jan 26 '15 at 14:24
  • 1
    \$\begingroup\$ I'm marking this one as accepted, because it's suggesting to leverage the partial nature of the generated code... and that's something I completely overlooked. Extending these classes to make a common interface is the ideal solution IMO. \$\endgroup\$ – Mathieu Guindon Jan 27 '15 at 4:43
5
\$\begingroup\$

ProcedureNode constructor

The private constructor can be simplified / made be more readable by returning early if asType == null. You also shouldn't check for returnTypeClause == null twice.

You could consider to change the private readonly string _returnType; to a "normal" string and add a private setter to the property. By doing this you could get rid of the tenary expression by setting _returnType = ReservedKeywords.Variant at initialization.

private ProcedureNode(ParserRuleContext context, string scope, string localScope, 
                      VBProcedureKind kind, 
                      VisualBasic6Parser.VisibilityContext visibility, 
                      VisualBasic6Parser.AmbiguousIdentifierContext name, 
                      Func<VisualBasic6Parser.AsTypeClauseContext> asType)
    : base(context, scope, localScope)
{
    _kind = kind;
    _name = name.GetText();
    _accessibility = visibility.GetAccessibility();

    if (asType == null) { return; }

    var returnTypeClause = asType();

    var returnTypeClauseIsNull  = returnTypeClause == null;

    _isImplicitReturnType = returnTypeClauseIsNull;

    _returnType = returnTypeClauseIsNull   
                    ? ReservedKeywords.Variant 
                    : returnTypeClause.type().GetText();
}  

ParserRuleContextExtensions

With the given name of the class, the GetAccessibility() method doesn't belong there. If you consider to keep both methods in this class, you should consider to rename the inputparameters to parserRuleContext and visibilityContext.

VariableNode constructor

After reordering the assignments in the way _accessibility is assigned at the top you can reduce horizontal spacing by using guard clauses.

Storing the result of context.asTypeClause() and context.typeHint() will reduce code duplication.

public VariableNode(VisualBasic6Parser.VariableSubStmtContext context, string scope, 
                    VisualBasic6Parser.VisibilityContext visibility, bool isLocal = true)
    : base(context, scope)
{
    _name = context.ambiguousIdentifier().GetText();
    _accessibility = isLocal ? VBAccessibility.Private : visibility.GetAccessibility();

    var asTypeClause = context.asTypeClause();
    if (asTypeClause != null)
    {
        _typeName = asTypeClause.type().GetText();
        return;
    }

    var hint = context.typeHint();
    if (hint  == null)
    {
        _isImplicitlyTyped = true;
        _typeName = ReservedKeywords.Variant;
        return;
    }

    _isUsingTypeHint = true;
    _typeName = TypeSpecifiers[hint.GetText()];
}
\$\endgroup\$
2
  • \$\begingroup\$ ParserRuleContext is a base class from which VisibilityContext is derived; doesn't it make sense to extend both in the same static class? \$\endgroup\$ – Mathieu Guindon Jan 26 '15 at 12:30
  • 1
    \$\begingroup\$ If this is the case (which I couldn't figure out based on the posted code), I would say it could make sense. But renaming the parameter should be done to make the difference evident. \$\endgroup\$ – Heslacher Jan 26 '15 at 12:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.