12
\$\begingroup\$

One of the inspections we wanted to implement in Rubberduck for the next release, is one that finds all unassigned variables in a VBA project.

unassigned module-scoped variable appears as an "error" in the Code Inspections window

Implementing this inspection has been ...complicated (it's the most complex inspection in the Rubberduck code base so far). Here's the code:

public class VariableNotAssignedInspection : IInspection
{
    public VariableNotAssignedInspection()
    {
        Severity = CodeInspectionSeverity.Error;
    }

    public string Name { get { return InspectionNames.VariableNotAssigned; } }
    public CodeInspectionType InspectionType { get { return CodeInspectionType.CodeQualityIssues; } }
    public CodeInspectionSeverity Severity { get; set; }

    public IEnumerable<CodeInspectionResultBase> GetInspectionResults(IEnumerable<VBComponentParseResult> parseResult)
    {
        var parseResults = parseResult.ToList();

        // publics & globals delared at module-scope in standard modules:
        var globals = 
            parseResults.Select(result => 
                            new 
                            { 
                                Name = result.QualifiedName, 
                                Globals = result.ParseTree.GetContexts<DeclarationSectionListener, ParserRuleContext>(new DeclarationSectionListener())
                                                          .OfType<VisualBasic6Parser.VariableStmtContext>()
                                                          .Where(context => 
                                                              context.visibility() != null && 
                                                              context.visibility().GetText() != Tokens.Private)
                                                          .SelectMany(context => context.variableListStmt().variableSubStmt()
                                                                                .Select(variable => variable.ambiguousIdentifier()))
                            })
                         .SelectMany(module => module.Globals.Select(global => 
                            new
                            {
                                Name = module.Name,
                                Global = global
                            })).ToList();

        var assignedGlobals = new List<VisualBasic6Parser.AmbiguousIdentifierContext>();
        var unassignedDeclarations = new List<CodeInspectionResultBase>();

        foreach (var result in parseResults)
        {
            // module-scoped in this module:
            var declarations = result.ParseTree.GetContexts<DeclarationSectionListener, ParserRuleContext>(new DeclarationSectionListener())
                                     .OfType<VisualBasic6Parser.VariableSubStmtContext>()
                                     .Where(variable => globals.All(global => global.Global.GetText() != variable.GetText()))
                                     .ToList();
            var procedures = result.ParseTree.GetContexts<ProcedureListener, ParserRuleContext>(new ProcedureListener()).ToList();

            // fetch & scope all assignments:
            var assignments = procedures.SelectMany(
                procedure => procedure.GetContexts<VariableAssignmentListener, VisualBasic6Parser.AmbiguousIdentifierContext>(new VariableAssignmentListener())
                                     .Select(context => new
                                         {
                                             Scope = new QualifiedMemberName(result.QualifiedName, ((dynamic)procedure).ambiguousIdentifier().GetText()),
                                             Name = context.GetText()
                                         }));

            // fetch & scope all procedure-scoped declarations:
            var locals = procedures.SelectMany(
                procedure => procedure.GetContexts<DeclarationListener, ParserRuleContext>(new DeclarationListener())
                                      .OfType<VisualBasic6Parser.VariableSubStmtContext>()
                                      .Select(context => new
                                         {
                                             Context = context,
                                             Scope = new QualifiedMemberName(result.QualifiedName, ((dynamic)procedure).ambiguousIdentifier().GetText()),
                                             Name = context.ambiguousIdentifier().GetText()
                                         }));

            // identify unassigned module-scoped declarations:
            unassignedDeclarations.AddRange(
                declarations.Select(d => d.ambiguousIdentifier())
                            .Where(d => globals.All(g => g.Global.GetText() != d.GetText()) 
                                    && assignments.All(a => a.Name != d.GetText()))
                            .Select(identifier => new VariableNotAssignedInspetionResult(Name, Severity, identifier, result.QualifiedName)));

            // identify unassigned procedure-scoped declarations:
            unassignedDeclarations.AddRange(
                locals.Where(local => assignments.All(a => (a.Scope.MemberName + a.Name) != (local.Scope.MemberName + local.Name)))
                      .Select(identifier => new VariableNotAssignedInspetionResult(Name, Severity, identifier.Context.ambiguousIdentifier(), result.QualifiedName)));

            // identify globals assigned in this module:
            assignedGlobals.AddRange(globals.Where(global => assignments.Any(a => a.Name == global.Global.GetText()))
                                            .Select(global => global.Global));
        }

        // identify unassigned globals:
        var assignedIdentifiers = assignedGlobals.Select(assigned => assigned.GetText());
        var unassignedGlobals = globals.Where(global => !assignedIdentifiers.Contains(global.Global.GetText()))
                                       .Select(identifier => new VariableNotAssignedInspetionResult(Name, Severity, identifier.Global, identifier.Name));
        unassignedDeclarations.AddRange(unassignedGlobals);

        return unassignedDeclarations;
    }
}

Could this be simplified?

The VariableNotAssignedInspectionResult class is much simpler - it simply removes the unassigned variable declaration:

public class VariableNotAssignedInspetionResult : CodeInspectionResultBase
{
    public VariableNotAssignedInspetionResult(string inspection, CodeInspectionSeverity type,
        ParserRuleContext context, QualifiedModuleName qualifiedName)
        : base(inspection, type, qualifiedName, context)
    {
    }

    public override IDictionary<string, Action<VBE>> GetQuickFixes()
    {
        return
            new Dictionary<string, Action<VBE>>
            {
                {"Remove unused variable", RemoveVariableDeclaration}
            };
    }

    private void RemoveVariableDeclaration(VBE vbe)
    {
        var module = vbe.FindCodeModules(QualifiedName).First();
        var selection = QualifiedSelection.Selection;

        var originalCodeLines = module.get_Lines(selection.StartLine, selection.LineCount)
            .Replace("\r\n", " ")
            .Replace("_", string.Empty);

        var originalInstruction = Context.GetText();
        module.DeleteLines(selection.StartLine, selection.LineCount);

        var newInstruction = string.Empty;
        var newCodeLines = string.IsNullOrEmpty(newInstruction)
            ? string.Empty
            : originalCodeLines.Replace(originalInstruction, newInstruction);

        if (!string.IsNullOrEmpty(newCodeLines))
        {
            module.InsertLines(selection.StartLine, newCodeLines);
        }
    }
}

I'll have to change it a bit - an unassigned variable isn't necessarily unused, and removing its declaration can actually make the code uncompilable (with Option Explicit specified) if the unassigned variable is actually used anyway. But the crux is there, and works as intended - at least it seems to, in all cases I've tested.


Rubberduck uses reflection to find all implementations of the IInspection interface; this means the above code (IInspection and CodeInspectionResultBase implementations) is all I needed to get a working brand new inspection. Below is the code that actually performs all inspections - the order in which inspection results come in isn't important, so I'm creating a Task for each one of them:

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

        var results = new ConcurrentBag<CodeInspectionResultBase>();
        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));
    }

Is this an efficient approach?

\$\endgroup\$
8
\$\begingroup\$

This doesn't address your actually question (simplifying the logic/implementation), but I feel like it's important.

For the sake of readability, I would recommend replacing some of the nested lambdas with a function reference to a named methods. The assignment to globals in the first section of code has 5 levels of indentation. That is a bit much for an assignment statement. Looking at the lambda passed to the first Select() is scary. If I had more context with the code, it might not seem that daunting. But at first glance, there is a lot going on.

The way the code is now, I feel like I would have to dive in and understand all the details just to understand the basic steps it is taking. The first time I read through code, I don't care about the small implementation details. A well named method is a great way to extract away the details and convey your intent. Then, when I have a higher level understanding, then I can dive into the things that caught my eye as potential issues. Requiring this much up-front commitment is likely hindering your actual question from being looked into.

A simple example from your code:

.Where(context =>
       context.visibility() != null &&
       context.visibility().GetText() != Tokens.Private)

I just want to know you are filtering for non-private tokens, not how you check for one.

.Where(IsNonPrivateToken)

This takes the pressure of the reader as the LINQ statement reads more like a sentence than code.

Doing this will also help with the excessive indentation. The majority of that first Select() lambda requires horizontal scrolling.

\$\endgroup\$
3
\$\begingroup\$

Extract a whole class out of this code, call it IdentifierUsageInspector, and then have your identifier usage inspections use that class.

Keep the methods simple, straightforward, and to the point, and the result will be (much) cleaner code.

I boiled the GetInspectionResults implementation down to this:

public IEnumerable<CodeInspectionResultBase> GetInspectionResults(IEnumerable<VBComponentParseResult> parseResult)
{
    var inspector = new IdentifierUsageInspector(parseResult);
    var issues = inspector.UnassignedGlobals()
          .Union(inspector.UnassignedFields())
          .Union(inspector.UnassignedLocals());

    foreach (var issue in issues)
    {
        yield return new VariableNotAssignedInspectionResult(Name, Severity, issue.Context, issue.QualifiedName);
    }
}

I don't think it gets much simpler than that! And now all inspections that need to know what's unassigned where and in which scope, can reuse the extracted code.

\$\endgroup\$

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.