RubberduckParser 2.0 - Asynchronous Parsing

Question

The IRubberduckParser interface has seen breaking changes, and now looks like this:

public interface IRubberduckParser
{
    RubberduckParserState State { get; }
    Task ParseAsync(VBComponent component, CancellationToken token);
    void Resolve(CancellationToken token);
}

This new interface does more than breaking all the existing unit tests and pretty much every single feature: it flips things around and centralizes parser state, which can now be shared amongst all features... and instead of being fired up by the many features, it's now activated by a keyhook that captures keypresses in the VBE and starts parser tasks asynchronously - features can be disabled while the parser is working in the background, and re-enabled when it's in a "ready" state... without ever annoying the user with a blocking call.

Here's the implementation:

public class RubberduckParser : IRubberduckParser
{
    private readonly VBE _vbe;
    private readonly Logger _logger;

    public RubberduckParser(VBE vbe, RubberduckParserState state)
    {
        _vbe = vbe;
        _state = state;
        _logger = LogManager.GetCurrentClassLogger();
    }

    private readonly RubberduckParserState _state;
    public RubberduckParserState State { get { return _state; } }

    public async Task ParseAsync(VBComponent vbComponent, CancellationToken token)
    {
        var component = vbComponent;

        var parseTask = Task.Run(() => ParseInternal(component, token), token);

        try
        {
            await parseTask;
        }
        catch (SyntaxErrorException exception)
        {
            State.SetModuleState(component, ParserState.Error, exception);
        }
        catch (OperationCanceledException)
        {
            // no need to blow up
        } 
    }

    public void Resolve(CancellationToken token)
    {
        var options = new ParallelOptions { CancellationToken = token };
        Parallel.ForEach(_state.ParseTrees, options, kvp =>
        {
            token.ThrowIfCancellationRequested();
            ResolveReferences(kvp.Key, kvp.Value, token);
        });
    }

    private IEnumerable<CommentNode> ParseComments(QualifiedModuleName qualifiedName)
    {
        var code = qualifiedName.Component.CodeModule.Code();
        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 + 1);

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

    private void ParseInternal(VBComponent vbComponent, CancellationToken token)
    {
        _state.ClearDeclarations(vbComponent);
        State.SetModuleState(vbComponent, ParserState.Parsing);

        var qualifiedName = new QualifiedModuleName(vbComponent);
        _state.SetModuleComments(vbComponent, ParseComments(qualifiedName));

        var obsoleteCallsListener = new ObsoleteCallStatementListener();
        var obsoleteLetListener = new ObsoleteLetStatementListener();

        var listeners = new IParseTreeListener[]
        {
            obsoleteCallsListener,
            obsoleteLetListener
        };

        token.ThrowIfCancellationRequested();

        ITokenStream stream;
        var code = vbComponent.CodeModule.Lines();
        var tree = ParseInternal(code, listeners, out stream);

        token.ThrowIfCancellationRequested();
        _state.AddTokenStream(vbComponent, stream);
        _state.AddParseTree(vbComponent, tree);

        // cannot locate declarations in one pass *the way it's currently implemented*,
        // because the context in EnterSubStmt() doesn't *yet* have child nodes when the context enters.
        // so we need to EnterAmbiguousIdentifier() and evaluate the parent instead - this *might* work.
        var declarationsListener = new DeclarationSymbolsListener(qualifiedName, Accessibility.Implicit, vbComponent.Type, _state.Comments, token);

        token.ThrowIfCancellationRequested();
        declarationsListener.NewDeclaration += declarationsListener_NewDeclaration;
        declarationsListener.CreateModuleDeclarations();

        token.ThrowIfCancellationRequested();
        var walker = new ParseTreeWalker();
        walker.Walk(declarationsListener, tree);
        declarationsListener.NewDeclaration -= declarationsListener_NewDeclaration;

        _state.ObsoleteCallContexts = obsoleteCallsListener.Contexts.Select(context => new QualifiedContext(qualifiedName, context));
        _state.ObsoleteLetContexts = obsoleteLetListener.Contexts.Select(context => new QualifiedContext(qualifiedName, context));

        State.SetModuleState(vbComponent, ParserState.Parsed);
    }

    private IParseTree ParseInternal(string code, IEnumerable<IParseTreeListener> listeners, out ITokenStream outStream)
    {
        var input = new AntlrInputStream(code);
        var lexer = new VBALexer(input);
        var tokens = new CommonTokenStream(lexer);
        var parser = new VBAParser(tokens);

        parser.AddErrorListener(new ExceptionErrorListener());
        foreach (var listener in listeners)
        {
            parser.AddParseListener(listener);
        }

        outStream = tokens;
        return parser.startRule();
    }

    private void declarationsListener_NewDeclaration(object sender, DeclarationEventArgs e)
    {
         _state.AddDeclaration(e.Declaration);
    }

    private void ResolveReferences(VBComponent component, IParseTree tree, CancellationToken token)
    {
        if (_state.GetModuleState(component) != ParserState.Parsed)
        {
            return;
        }

        _state.SetModuleState(component, ParserState.Resolving);
        var declarations = _state.AllDeclarations;

        var resolver = new IdentifierReferenceResolver(new QualifiedModuleName(component), declarations);
        var listener = new IdentifierReferenceListener(resolver, token);
        var walker = new ParseTreeWalker();
        walker.Walk(listener, tree);

        _state.SetModuleState(component, ParserState.Ready);
    }

    private class ObsoleteCallStatementListener : VBABaseListener
    {
        private readonly IList<VBAParser.ExplicitCallStmtContext> _contexts = new List<VBAParser.ExplicitCallStmtContext>();
        public IEnumerable<VBAParser.ExplicitCallStmtContext> Contexts { get { return _contexts; } }

        public override void EnterExplicitCallStmt(VBAParser.ExplicitCallStmtContext context)
        {
            var procedureCall = context.eCS_ProcedureCall();
            if (procedureCall != null)
            {
                if (procedureCall.CALL() != null)
                {
                    _contexts.Add(context);
                    return;
                }
            }

            var memberCall = context.eCS_MemberProcedureCall();
            if (memberCall == null) return;
            if (memberCall.CALL() == null) return;
            _contexts.Add(context);
        }
    }

    private class ObsoleteLetStatementListener : VBABaseListener
    {
        private readonly IList<VBAParser.LetStmtContext> _contexts = new List<VBAParser.LetStmtContext>();
        public IEnumerable<VBAParser.LetStmtContext> Contexts { get { return _contexts; } }

        public override void EnterLetStmt(VBAParser.LetStmtContext context)
        {
            if (context.LET() != null)
            {
                _contexts.Add(context);
            }
        }
    }
}

I'm introducing a centralized RubberduckParserState class, which replaces the old and awkward Declarations type:

public class RubberduckParserState
{
    // keys are the declarations; values indicate whether a declaration is resolved.
    private readonly ConcurrentDictionary<Declaration, ResolutionState> _declarations =
        new ConcurrentDictionary<Declaration, ResolutionState>();

    private readonly ConcurrentDictionary<VBComponent, ITokenStream> _tokenStreams =
        new ConcurrentDictionary<VBComponent, ITokenStream>();

    private readonly ConcurrentDictionary<VBComponent, IParseTree> _parseTrees =
        new ConcurrentDictionary<VBComponent, IParseTree>();

    public event EventHandler StateChanged;

    private void OnStateChanged()
    {
        var handler = StateChanged;
        if (handler != null)
        {
            handler.Invoke(this, EventArgs.Empty);
        }
    }

    private readonly ConcurrentDictionary<VBComponent, ParserState> _moduleStates =
        new ConcurrentDictionary<VBComponent, ParserState>();

    private readonly ConcurrentDictionary<VBComponent, SyntaxErrorException> _moduleExceptions =
        new ConcurrentDictionary<VBComponent, SyntaxErrorException>();

    public void SetModuleState(VBComponent component, ParserState state, SyntaxErrorException parserError = null)
    {
        _moduleStates[component] = state;
        _moduleExceptions[component] = parserError;

        Status = _moduleStates.Values.Any(value => value == ParserState.Error)
            ? ParserState.Error
            : _moduleStates.Values.Any(value => value == ParserState.Parsing)
                ? ParserState.Parsing
                : _moduleStates.Values.Any(value => value == ParserState.Resolving)
                    ? ParserState.Resolving
                    : ParserState.Ready;

    }

    public ParserState GetModuleState(VBComponent component)
    {
        return _moduleStates[component];
    }

    private ParserState _status;
    public ParserState Status { get { return _status; } private set { if(_status != value) {_status = value; OnStateChanged();} } }

    private IEnumerable<QualifiedContext> _obsoleteCallContexts = new List<QualifiedContext>();

    /// <summary>
    /// Gets <see cref="ParserRuleContext"/> objects representing 'Call' statements in the parse tree.
    /// </summary>
    public IEnumerable<QualifiedContext> ObsoleteCallContexts
    {
        get { return _obsoleteCallContexts; }
        internal set { _obsoleteCallContexts = value; }
    }

    private IEnumerable<QualifiedContext> _obsoleteLetContexts = new List<QualifiedContext>();

    /// <summary>
    /// Gets <see cref="ParserRuleContext"/> objects representing explicit 'Let' statements in the parse tree.
    /// </summary>
    public IEnumerable<QualifiedContext> ObsoleteLetContexts
    {
        get { return _obsoleteLetContexts; }
        internal set { _obsoleteLetContexts = value; }
    }

    private readonly ConcurrentDictionary<VBComponent, IEnumerable<CommentNode>> _comments =
        new ConcurrentDictionary<VBComponent, IEnumerable<CommentNode>>();

    public IEnumerable<CommentNode> Comments
    {
        get 
        {
            return _comments.Values.SelectMany(comments => comments);
        }
    }

    public void SetModuleComments(VBComponent component, IEnumerable<CommentNode> comments)
    {
        _comments[component] = comments;
    }

    /// <summary>
    /// Gets a copy of the collected declarations.
    /// </summary>
    public IEnumerable<Declaration> AllDeclarations { get { return _declarations.Keys.ToList(); } }

    /// <summary>
    /// Adds the specified <see cref="Declaration"/> to the collection (replaces existing).
    /// </summary>
    public void AddDeclaration(Declaration declaration)
    {
        if (_declarations.TryAdd(declaration, ResolutionState.Unresolved))
        {
            return;
        }

        if (RemoveDeclaration(declaration))
        {
            _declarations.TryAdd(declaration, ResolutionState.Unresolved);
        }
    }

    public void ClearDeclarations(VBComponent component)
    {
        var declarations = _declarations.Keys.Where(k =>
            k.QualifiedName.QualifiedModuleName.Project == component.Collection.Parent
            && k.ComponentName == component.Name);

        foreach (var declaration in declarations)
        {
            ResolutionState state;
            _declarations.TryRemove(declaration, out state);
        }
    }

    public void AddTokenStream(VBComponent component, ITokenStream stream)
    {
        _tokenStreams[component] = stream;
    }

    public void AddParseTree(VBComponent component, IParseTree parseTree)
    {
        _parseTrees[component] = parseTree;
    }

    public IEnumerable<KeyValuePair<VBComponent, IParseTree>> ParseTrees { get { return _parseTrees; } }

    public TokenStreamRewriter GetRewriter(VBComponent component)
    {
        return new TokenStreamRewriter(_tokenStreams[component]);
    }

    /// <summary>
    /// Removes the specified <see cref="declaration"/> from the collection.
    /// </summary>
    /// <param name="declaration"></param>
    /// <returns>Returns true when successful.</returns>
    private bool RemoveDeclaration(Declaration declaration)
    {
        ResolutionState state;
        return _declarations.TryRemove(declaration, out state);
    }

    /// <summary>
    /// Ensures parser state accounts for built-in declarations.
    /// This method has no effect if built-in declarations have already been loaded.
    /// </summary>
    public void AddBuiltInDeclarations(IHostApplication hostApplication)
    {
        if (_declarations.Any(declaration => declaration.Key.IsBuiltIn))
        {
            return;
        }

        var builtInDeclarations = VbaStandardLib.Declarations;

        // cannot be strongly-typed here because of constraints on COM interop and generics in the inheritance hierarchy. </rant>
        if (hostApplication /*is ExcelApp*/ .ApplicationName == "Excel") 
        {
            builtInDeclarations = builtInDeclarations.Concat(ExcelObjectModel.Declarations);
        }

        foreach (var declaration in builtInDeclarations)
        {
            AddDeclaration(declaration);
        }
    }
}

---

Additional context

The parser is fired up at startup in the App class, like this:

public void Startup()
{
    CleanReloadConfig();

    _appMenus.Initialize();
    _appMenus.Localize();

    Task.Delay(1000).ContinueWith(t =>
    {
        _parser.State.AddBuiltInDeclarations(_vbe.HostApplication());
        ParseAll();
    });

    _hook.Attach();
}

The 1000ms delay is to allow the VBE itself to initialize and so the HostApplication() extension method doesn't return a null. The _hook is a low-level keyboard hook that also fires up the parser, like this:

private async void _hook_KeyPressed(object sender, KeyHookEventArgs e)
{
    await ParseComponentAsync(e.Component);
}

private async Task ParseComponentAsync(VBComponent component, bool resolve = true)
{
    var tokenSource = RenewTokenSource(component);

    var token = tokenSource.Token;
    await _parser.ParseAsync(component, token);

    if (resolve && !token.IsCancellationRequested)
    {
        using (var source = new CancellationTokenSource())
        {
            _parser.Resolve(source.Token);
        }
    }
}

private CancellationTokenSource RenewTokenSource(VBComponent component)
{
    if (_tokenSources.ContainsKey(component))
    {
        CancellationTokenSource existingTokenSource;
        _tokenSources.TryRemove(component, out existingTokenSource);
        existingTokenSource.Cancel();
        existingTokenSource.Dispose();
    }

    var tokenSource = new CancellationTokenSource();
    _tokenSources[component] = tokenSource;
    return tokenSource;
}

The only known issue so far, is that the low-level key hook doesn't seem to work in Office 2016 hosts - but works as expected in Office 2010 / on my machine. Full code on GitHub (at last commit as of this writing).

Any & all feedback and improvements are welcome.

Answer 1

Nested ternary conditionals are ugly. Ternary conditionals nested three deep are ugly to the third power:

Status = _moduleStates.Values.Any(value => value == ParserState.Error)
    ? ParserState.Error
    : _moduleStates.Values.Any(value => value == ParserState.Parsing)
        ? ParserState.Parsing
        : _moduleStates.Values.Any(value => value == ParserState.Resolving)
            ? ParserState.Resolving
            : ParserState.Ready;

I would use ifs here. TopinFrassi has already addressed the issue that this may be unnecessarily expensive.

---

You can combine these ifs into one condition:

if (procedureCall != null)
{
    if (procedureCall.CALL() != null)
    {
        _contexts.Add(context);
        return;
    }
}

---

You can combine these conditions, and be sure to put braces on the body:

if (memberCall == null) return;
if (memberCall.CALL() == null) return;

Because you usually use braces, I would guess those are from accepting R# suggestions. You can turn braces on in the settings by going to the Options and choosing Code Editing -> C# -> Formatting Style -> Braces Layout and changing the settings in the Force Braces group.

---

There is a bug in here in which the resolver is started before the parser finishes, causing messed up results. The parser must be finished before the resolver starts. This is accomplished by changing the following method:

public async Task ParseAsync(VBComponent vbComponent, CancellationToken token)
{
    var component = vbComponent;

    var parseTask = Task.Run(() => ParseInternal(component, token), token);

    try
    {
        await parseTask;
    }
    catch (SyntaxErrorException exception)
    {
        State.SetModuleState(component, ParserState.Error, exception);
    }
    catch (OperationCanceledException)
    {
        // no need to blow up
    } 
}

First, we might as well check for a cancellation request. Add this line before both statements involving parseTask:

token.ThrowIfCancellationRequested();

Next, change the await parseTask; statement to:

parseTask.Wait(token);

The async modifier is now unnecessary. Because this method is called asynchronously, the parser will still run asynchronously, but the resolver cannot start until the parse task completes.

Answer 2

You don't check your parameters for null! When a method/constructor is public, you need to check that because it is assumed that anyone could reuse your code. If your class was internal, protected or private, I wouldn't mind but now you do have a problem that a NullReferenceException will be thrown at a moment that isn't expected.

You use var a lot. Which isn't bad. But I think it's useful for readability to use var only when you can guess the type by reading the code. In such a case :

var code = qualifiedName.Component.CodeModule.Code();

I don't know what code's type is. And it is used alot.

Also, I don't know if that Code() method is yours or an external DLL, but that doesn't respect the naming conventions. It should be, I believe : GetCode(), or a property Code. Unless for sure, this method call codes something. But it doesn't look that way.

This :

if (memberCall == null) return;
if (memberCall.CALL() == null) return;

Could be changed to : if (memberCall?.CALL() == null) return;

And while we're there, I know many people in this community values the use of brackets every time it is possible, as in this case. It might prevent a bug someday, but yeah, I think it bloats the code. Now that's your call to make :)

I don't know if performance is important in that piece of code (I'd guess it is), but that :

Status = _moduleStates.Values.Any(value => value == ParserState.Error)
    ? ParserState.Error
    : _moduleStates.Values.Any(value => value == ParserState.Parsing)
        ? ParserState.Parsing
        : _moduleStates.Values.Any(value => value == ParserState.Resolving)
            ? ParserState.Resolving
            : ParserState.Ready;

Might cause problems if there's a lot of Values. You iterate through the collection three times when one would be enough. From what I understand, you need to check all the collection is there are parsing errors, then you check it all for the other states.

Why don't you build an algorithm for this that'd iterate once? (That algorithm would also deserve its own method I think)

Something like this :

var state = ParserState.Ready;

for (var value in _moduleStates.Values)
{
    if(value == ParserState.Error) 
    {
        state = ParserState.Error;  
        break;
    }
    else if(value == ParserState.Parsing)
        state = ParserState.Parsing;
    else if(state != ParserState.Parsing && value == ParserState.Resolving)
        state = ParserState.Resolving;
}

return state;

I think it would be faster.