Inspector Rubberduck - Take Two

Release 1.1 of Rubberduck only had a handful of implemented code inspections, more as a proof of concept than anything else. For release 1.2, we now have 19 implementations of our IInspection interface - here is what happens when the user clicks the refresh button on the Code Inspections toolwindow:

private void Refresh()
{
    var code = new VBProjectParseResult(_parser.Parse(VBE.ActiveVBProject));

    var results = new ConcurrentBag();
    var inspections = _inspections.Where(inspection => inspection.Severity != CodeInspectionSeverity.DoNotShow)
        .Select(inspection =>
            new Task(() =>
            {
                var result = inspection.GetInspectionResults(code);
                foreach (var inspectionResult in result)
                {
                    results.Add(inspectionResult);
                }
            })).ToArray();

    foreach (var inspection in inspections)
    {
        inspection.Start();
    }

    Task.WaitAll(inspections);

    _results = results.ToList();
    Control.SetContent(_results.Select(item => new CodeInspectionResultGridViewItem(item)).OrderBy(item => item.Component).ThenBy(item => item.Line));
}

The idea is to collect inspection results concurrently, wait for all results to be collected, and then to refresh the grid showing the inspection results. The thought of populating the grid as results come in occurred to me, but I presume the cost of repainting the grid every time would outweight the benefits, especially with larger VBA projects - I tested one that yield 177 code issues (including some false positives, but that's a separate issue, I'm working on that!).

Before I implemented the changes I'm about to describe, inspecting issues in that project would simply freeze the VBE (and the host app) and I'd end up stopping the debugger - the project with the 177 issues completely brought our ducky to its knees. Actually, it's not the issues themselves; it's the inefficiency of walking the parse tree a

//result.AddRange(declarations.Select(declaration => declaration.Context)
//                            .OfType()
//                            .Select(context => 
//        context.AmbiguousIdentifier().ToQualifiedContext(module.QualifiedName)));

Commented code is dead code and dead code should be deleted.

In

private HashSet> 
    GetFields(IEnumerable> globals)

You are calling ToList() on the IEnumerable and the again using Select() thow changing to an IEnumerable again which then is added to a List by calling AddRange().

Because it is an IEnumerable and not an ICollection the call to AddRange() will again iterate over the elements of the IEnumerable to add them to the List.

So refactoring to

private HashSet> 
    GetFields(IEnumerable> globals)
{
    var result = new List>();
    foreach (var module in _parseResult)
    {
        var listener = new DeclarationSectionListener(module.QualifiedName);
        var fields = module.ParseTree
            .GetContexts(listener)
            .Where(field => globals.All(global => global.QualifiedName.ModuleName == field.QualifiedName.ModuleName 
                                               && global.Context.GetText() != field.Context.GetText()))
            .Select(declaration => declaration.Context)
            .OfType()
            .Select(context => context.AmbiguousIdentifier().ToQualifiedContext(module.QualifiedName))

        result.AddRange(fields);

    }

    return new HashSet>(result);
}

will be faster and better looking.

private HashSet> GetAssignments()

The variable var HashSetener = new VariableAssignmentListener(module.QualifiedName); should at least be named using camelCase casing, but what the heck is a HashSetener ?

I would also suggest to introduce a bool IsValidAssignmentContext() to be called by the Where() like

private bool IsValidAssignmentContext(VBParser.AmbiguousIdentifierContext context)
{
    return !IsConstant(context) && !IsJoinedAssignemntDeclaration(context)
}

and to be called

result.AddRange(module.ParseTree
            .GetContexts(HashSetener)
            .Where(identifier => IsValidAssignmentContext(identifier.Context)));

StackExchange Code Review Q#82446, answer score: 3