Rubberduck VBA Parser, Episode V: The ANTLR Strikes Back

Question

I changed my mind. I don't want to implement 200-some Node classes. Instead, I'll be working directly with the ANTLR generated classes, to implement the Rubberduck code inspections, unit test method discovery, the "Code Explorer" tree view, and everything else Rubberduck might need a parse tree for.

The Node derived classes already implemented aren't wasted though: I'll eventually use them to expose a high-level view of the VBA code, to VBA itself (through COM interop) - that will enable very cool stuff, like VBA code that can enumerate its modules' members.

I changed the VBParser - I renamed ParseInternal to Parse (it becomes an overload of the public Parse method), and made it public as well:

public class VBParser : IRubberduckParser
{
    public INode Parse(string projectName, string componentName, string code)
    {
        var result = Parse(code);
        var walker = new ParseTreeWalker();

        var listener = new VBTreeListener(projectName, componentName);
        walker.Walk(listener, result);

        return listener.Root;
    }

    public IParseTree Parse(string code)
    {
        var input = new AntlrInputStream(code);
        var lexer = new VisualBasic6Lexer(input);
        var tokens = new CommonTokenStream(lexer);
        var parser = new VisualBasic6Parser(tokens);

        return parser.startRule();
    }
}

Now I can have an ANTLR IParseTree wherever I need it. One place I'm going to need it, is to discover test methods in the active VBA project. Rubberduck test methods are always public, parameterless methods, so I wrote this extension method/class:

public static class ParseTreeExtensions
{
    /// <summary>
    /// Finds all public procedures in specified parse tree.
    /// </summary>
    public static IEnumerable<VisualBasic6Parser.SubStmtContext> GetPublicProcedures(this IParseTree parseTree)
    {
        var walker = new ParseTreeWalker();

        var listener = new PublicSubListener();
        walker.Walk(listener, parseTree);

        return listener.Members;
    }

    private class PublicSubListener : VisualBasic6BaseListener
    {
        private readonly IList<VisualBasic6Parser.SubStmtContext> _members = new List<VisualBasic6Parser.SubStmtContext>();
        public IEnumerable<VisualBasic6Parser.SubStmtContext> Members { get { return _members; } }

        public override void EnterSubStmt(VisualBasic6Parser.SubStmtContext context)
        {
            var visibility = context.visibility();
            if (visibility == null || visibility.PUBLIC() != null)
            {
                _members.Add(context);
            }
        }
    }
}

Here's a unit test for it:

    [TestMethod]
    public void GetPublicProceduresReturnsPublicSubs()
    {
        IRubberduckParser parser = new VBParser();
        var code = "Sub Foo()\nEnd Sub\n\nPrivate Sub FooBar()\nEnd Sub\n\nPublic Sub Bar()\nEnd Sub\n\nPublic Sub BarFoo(ByVal fb As Long)\nEnd Sub\n\nFunction GetFoo() As Bar\nEnd Function";

        var module = parser.Parse(code);
        var procedures = module.GetPublicProcedures().ToList();
        var parameterless = procedures.Where(p => p.argList().arg().Count == 0).ToList();

        Assert.AreEqual(3, procedures.Count);
        Assert.AreEqual(2, parameterless.Count);
    }

This works, so I'm going to run with it. I don't want to expose the ANTLR generated classes to COM, so I'm moving it all into its own assembly, which Rubberduck will reference.

It looks to me like VBParser is somewhat mixing abstraction levels... but is that much of an issue? Is it a good idea to "walk" the tree like this? I'm not re-parsing the code, but I'll probably end up walking it multiple times when I run code inspections, and I'll have to implement several [simple] tree listeners, to retrieve the "interesting" nodes. Is this how I'm supposed to be doing this?

Anything else strikes you as weird with this approach?

Answer 1

One would expect that a method of a parser which returns an IParseTree would be named Parse() instead of startRule() which is in addition violating the naming guidelines.

---

You should declare variables as near as possible to their usage. You could also omit the assignment of the call to Parse()

public INode Parse(string projectName, string componentName, string code)
{
    var walker = new ParseTreeWalker();                
    var listener = new VBTreeListener(projectName, componentName);

    walker.Walk(listener, Parse(code));

    return listener.Root;
}  

---

The VisualBasic6Parser.SubStmtContext.visibility() method is also not going conform with the naming guidelines.

---

Is it a good idea to "walk" the tree like this?

It is hard to say something about something one can't see.

---

It looks to me like VBParser is somewhat mixing abstraction levels... but is that much of an issue?

It is a little bit distracting to see inside a Parse() method of a VBParser class which implements an IRubberduckParser interface that a VisualBasic6Parser class is used. But I have no idea how to solve this and if this has to be solved.