8
\$\begingroup\$

VBA comment syntax is fun... and VBA line continuation makes it even more fun.

Picture a VBA module like this:

Rem this is an old-style comment.
' this is a more standard comment

Rem this _
    is _
    a _
    multiline _
    comment

Private Sub Foo() ' this _
                    is _
                    also _
                    a _
                    multiline _
                    comment _
                    _
...don't do this at home.

End Sub

'@TestMethod
Private Sub Bar()
    ' todo: call Foo
End Sub

(no wonder syntax highlighting is getting confused!)

If you don't know what Rubberduck is: Rubberduck is a COM add-in for the VBE / VBA's IDE that I'm building with ...@RubberDuck. I have a branch where I've burned the whole parser namespace and replaced it with ANTLR-generated code.

The only problem is that the .g4 VB6 grammar file I'm using to generate the parser, does not support comments. So I ended up [re-]inserting an abstraction layer between ANTLR's IParseTree and the rest of Rubberduck.. albeit very differently this time.

I added two methods to the IRubberduckParser interface:

/// <summary>
/// Parses all code modules in specified project.
/// </summary>
/// <returns>Returns an <c>IParseTree</c> for each code module in the project; the qualified module name being the key.</returns>
IEnumerable<VbModuleParseResult> Parse(VBProject vbProject);

IEnumerable<CommentNode> ParseComments(VBComponent vbComponent);

The VbModuleParseResult class encapsulates a module's IParseTree and its CommentNodes:

public class VbModuleParseResult
{
    public VbModuleParseResult(QualifiedModuleName qualifiedName, IParseTree parseTree, IEnumerable<CommentNode> comments)
    {
        _qualifiedName = qualifiedName;
        _parseTree = parseTree;
        _comments = comments;
    }

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

    private IParseTree _parseTree;
    public IParseTree ParseTree { get { return _parseTree; } }

    private IEnumerable<CommentNode> _comments;
    public IEnumerable<CommentNode> Comments { get { return _comments; } }

}

That object is returned by VBParser methods, that the rest of Rubberduck uses (via IRubberduckParser):

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);

        var result = parser.startRule();
        return result;
    }

    public IEnumerable<VbModuleParseResult> Parse(VBProject project)
    {
        return project.VBComponents.Cast<VBComponent>()
                      .Select(component => new VbModuleParseResult(new QualifiedModuleName(project.Name, component.Name), 
                                           Parse(component.CodeModule.ToString()), ParseComments(component)));
    }

Here we are, the IEnumerable<CommentNode> ParseComments(VBComponent component) implementation:

    public IEnumerable<CommentNode> ParseComments(VBComponent component)
    {
        var code = component.CodeModule.Code();
        var qualifiedName = new QualifiedModuleName(component.Collection.Parent.Name, component.Name);

        var commentBuilder = new StringBuilder();
        var continuing = false;

        var startLine = 0;
        var startColumn = 0;

        for (var i = 0; i < code.Length; i++)
        {
            var line = code[i];                
            var index = 0;

            if (continuing || line.HasComment(out index))
            {
                startLine = continuing ? startLine : i;
                startColumn = continuing ? startColumn : index;

                var commentLength = line.Length - index;

                continuing = line.EndsWith("_");
                if (!continuing)
                {
                    commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
                    var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

                    var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
                    commentBuilder.Clear();

                    yield return result;
                }
                else
                {
                    // ignore line continuations in comment text:
                    commentBuilder.Append(line.Substring(index, commentLength).TrimStart()); 
                }
            }
        }
    }
}

The code works perfectly:

debugging session showing the parsed CommentNodes

...but am I correct to read this last method and think there might be room for improvement? Anything else jumps at you?

\$\endgroup\$
7
\$\begingroup\$

Let's tackle this piece of code, shall we?

public IEnumerable<CommentNode> ParseComments(VBComponent component)
{
    var code = component.CodeModule.Code();
    var qualifiedName = new QualifiedModuleName(component.Collection.Parent.Name, component.Name);

    var commentBuilder = new StringBuilder();
    var continuing = false;

    var startLine = 0;
    var startColumn = 0;

    for (var i = 0; i < code.Length; i++)
    {
        var line = code[i];                
        var index = 0;

        if (continuing || line.HasComment(out index))
        {
            startLine = continuing ? startLine : i;
            startColumn = continuing ? startColumn : index;

            var commentLength = line.Length - index;

            continuing = line.EndsWith("_");
            if (!continuing)
            {
                commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
                var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

                var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
                commentBuilder.Clear();

                yield return result;
            }
            else
            {
                // ignore line continuations in comment text:
                commentBuilder.Append(line.Substring(index, commentLength).TrimStart()); 
            }
        }
    }
}

Alright, first thing I see is some duplication:

continuing = line.EndsWith("_");
if (!continuing)
{
    commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
    var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

    var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
    commentBuilder.Clear();

    yield return result;
}
else
{
    // ignore line continuations in comment text:
    commentBuilder.Append(line.Substring(index, commentLength).TrimStart()); 
}

commentBuilder.Append(line.Substring(index, commentLength).TrimStart()); is duplicated.

So lets remove it.

continuing = line.EndsWith("_");
commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
if (!continuing)
{
    var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

    var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
    commentBuilder.Clear();

    yield return result;
}

Additionally, there's some duplication here...

if (continuing || line.HasComment(out index))
{
    startLine = continuing ? startLine : i;
    startColumn = continuing ? startColumn : index;

    var commentLength = line.Length - index;

    continuing = line.EndsWith("_");
    commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
    if (!continuing)
    {
        var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

        var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
        commentBuilder.Clear();

        yield return result;
    }
}

continuing is checked three times between changes. That's a bit of a waste.

Maybe we can fix it?

if (continuing || line.HasComment(out index))
{
    if(!continuing){
        startLine = i;
        startColumn = index;
    }
    var commentLength = line.Length - index;

    continuing = line.EndsWith("_");
    commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
    if (!continuing)
    {
        var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

        var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
        commentBuilder.Clear();

        yield return result;
    }
}

Hmm, we're still checking it twice... and if (a || b) { if(!a) structures are messy. I wonder if there's something we can do about that?

Invert it, maybe? (if A OR B is true then NOT A implies B)

if (continuing || line.HasComment(out index))
{
    if(line.HasComment(out index)){
        startLine = i;
        startColumn = index;
    }

Hmm...
Here's a batshit crazy idea.

Put a for loop inside the current for loop to repeat the bits you need.

    for (var i = 0; i < code.Length; i++)
    {
        var line = code[i];                
        var index = 0;

        if (line.HasComment(out index))
        {
            startLine = i;
            startColumn = index;

            //multiline comment forloop...
            for (; i < code.Length; i++)
            {
                line = code[i];
                var commentLength = line.Length - index;
                commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
                if(!line.EndsWith("_"))
                {
                   break;
                }
            }
            var selection = new Selection(startLine + 1, startColumn + 1, i + 1, line.Length);

            var result = new CommentNode(commentBuilder.ToString(), new QualifiedSelection(qualifiedName, selection));
            commentBuilder.Clear();

            yield return result;
        }
    }

Yep, it works! * Mad programmer cackle *

You can even move this section

            for (; i < code.Length; i++)
            {
                line = code[i];
                var commentLength = line.Length - index;
                commentBuilder.Append(line.Substring(index, commentLength).TrimStart());
                if(!line.EndsWith("_"))
                {
                   break;
                }
            }

to a new function:

private int appendComment(code, lineNumber, commentBuilder, commentColumn)

But it takes... 4 arguments. So that's maybe not ideal.

New bugs ... err, features

There's one sideeffect:

Previously, if input was

' This is a file with a single line that claims to contain a multiline comment _

You would ignore it.

Now, it will make a new selection instead, which supposedly ends at code.Length + 1. So past EOF. This can be a feature ("Your comment goes past EOF!") or something that will break some asserts.

\$\endgroup\$
  • 2
    \$\begingroup\$ About the "bug": the line that immediately follows a comment that ends with an underscore, is also a comment, regardless of its content. As for the comment+underline+EOF, that's not something that is possible in VBA - I tried very hard to test it: the VBE automatically adds a new line (the continued comment) under it, and another new line under that (the properly terminated line of code). Conclusion: there is no such bug; importing a code file that would have been edited as such (say, in Notepad) to force it, results in the VBE itself ignoring the EOF-comment. \$\endgroup\$ – Mathieu Guindon Feb 5 '15 at 3:07
  • 1
    \$\begingroup\$ Also the +1 in code.Length + 1 is because line and column numbers in the VBE object model are 1-based: that +1 makes the Selection readily usable. \$\endgroup\$ – Mathieu Guindon Feb 5 '15 at 3:08
4
\$\begingroup\$

Disclaimer 1: I'm the other dev working on the project.
Disclaimer 2: This will be more of a pie in the sky design review than code review.


Co-dev Hat

It works, so I don't really care how it's implemented. (At least until I actually have to make some kind of change to it. Gods help me that may I never have to truly understand your parser. Not because it's bad, but because I haven't spent any time learning it.)

Reviewer Hat

I would expect the CommendNodes to be part of the ParseTree. I understand why they're not. The grammar file you borrowed from another Open Source project didn't include comments. This is because that project was interested in executing VBA code. Therefore it, naturally, excluded them. Our project is different. We're interested in analyzing the code and this includes the comments. I feel like the technically right thing to do would be to modify the grammar file so that comments are parsed along with the rest of the language. At that point, implementing features based off of parsed comments becomes identical to implementing any other feature based off of parsed code. As it is, any features that need to know about both the code and the comments is likely to need special handling for each. That's double the work and double the chance to introduce a bug.

Now, is that worth the effort? I don't know yet. I haven't tried to modify the grammar file to pick up comments yet.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 you're right - this is just a huge work-around, to work with the .g4 grammar without spending the time & effort to tweak it to support comments. FWIW I did try to modify the grammar file, but it would pick up comments under an ErrorNode... if that's the best I can get, I like the work-around better. \$\endgroup\$ – Mathieu Guindon Feb 4 '15 at 14:52

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.