Split up evaluating whether 'Set' is required and evaluating whether 'Set' needs to be added. Reduced scope of inspection.

Author: retailcoder

Rubberduck.Inspections/Concrete/ObjectVariableNotSetInspection.cs

@@ -1,11 +1,7 @@
-using System;
-using System.Collections.Generic;
-using System.Diagnostics;
+using System.Collections.Generic;
 using System.Linq;
 using Rubberduck.Inspections.Abstract;
 using Rubberduck.Inspections.Results;
-using Rubberduck.Parsing;
-using Rubberduck.Parsing.Grammar;
 using Rubberduck.Parsing.Inspections.Abstract;
 using Rubberduck.Parsing.Inspections.Resources;
 using Rubberduck.Parsing.Symbols;
@@ -33,19 +29,19 @@ protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
         private IEnumerable<IdentifierReference> InterestingReferences()
         {
             var result = new List<IdentifierReference>();
-            foreach (var qmn in State.DeclarationFinder.AllModules.Where(m => m.ComponentType != ComponentType.Undefined && m.ComponentType != ComponentType.ComComponent))
+            foreach (var moduleReferences in State.DeclarationFinder.IdentifierReferences())
             {
-                var module = State.DeclarationFinder.ModuleDeclaration(qmn);
+                var module = State.DeclarationFinder.ModuleDeclaration(moduleReferences.Key);
                 if (module == null || !module.IsUserDefined || IsIgnoringInspectionResultFor(module, AnnotationName))
                 {
-                    // module isn't user code, or this inspection is ignored at module-level
+                    // module isn't user code (?), or this inspection is ignored at module-level
                     continue;
                 }
 
-                foreach (var reference in State.DeclarationFinder.IdentifierReferences(qmn))
+                foreach (var reference in moduleReferences.Value)
                 {
                     if (!IsIgnoringInspectionResultFor(reference, AnnotationName) 
-                        && VariableRequiresSetAssignmentEvaluator.RequiresSetAssignment(reference, State))
+                        && VariableRequiresSetAssignmentEvaluator.NeedsSetKeywordAdded(reference, State))
                     {
                         result.Add(reference);
                     }

Rubberduck.Inspections/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFix.cs

@@ -116,21 +116,17 @@ private void InsertLocalVariableDeclarationAndAssignment(IModuleRewriter rewrite
             var requiresAssignmentUsingSet =
                 target.References.Any(refItem => VariableRequiresSetAssignmentEvaluator.RequiresSetAssignment(refItem, _parserState));
 
-            var localVariableAssignment = string.Format("{0}{1}",
-                                                        requiresAssignmentUsingSet ? $"{Tokens.Set} " : string.Empty,
-                                                        $"{localIdentifier} = {target.IdentifierName}");
+            var localVariableAssignment =
+                $"{(requiresAssignmentUsingSet ? $"{Tokens.Set} " : string.Empty)}{localIdentifier} = {target.IdentifierName}";
 
             var endOfStmtCtxt = ((ParserRuleContext)target.Context.Parent.Parent).GetChild<VBAParser.EndOfStatementContext>();
             var eosContent = endOfStmtCtxt.GetText();
-            var idxLastNewLine = eosContent.LastIndexOf(Environment.NewLine);
+            var idxLastNewLine = eosContent.LastIndexOf(Environment.NewLine, StringComparison.InvariantCultureIgnoreCase);
             var endOfStmtCtxtComment = eosContent.Substring(0, idxLastNewLine);
             var endOfStmtCtxtEndFormat = eosContent.Substring(idxLastNewLine);
 
-            var insertCtxt = (ParserRuleContext)((ParserRuleContext)target.Context.Parent.Parent).GetChild<VBAParser.AsTypeClauseContext>();
-            if (insertCtxt == null)
-            {
-                insertCtxt = (ParserRuleContext)target.Context.Parent;
-            }
+            var insertCtxt = ((ParserRuleContext) target.Context.Parent.Parent).GetChild<VBAParser.AsTypeClauseContext>()
+                ?? (ParserRuleContext) target.Context.Parent;
 
             rewriter.Remove(endOfStmtCtxt);
             rewriter.InsertAfter(insertCtxt.Stop.TokenIndex, $"{endOfStmtCtxtComment}{endOfStmtCtxtEndFormat}{localVariableDeclaration}" + $"{endOfStmtCtxtEndFormat}{localVariableAssignment}{endOfStmtCtxtEndFormat}");

Rubberduck.Inspections/VariableRequiresSetAssignmentEvaluator.cs

@@ -20,6 +20,22 @@ public static IEnumerable<Declaration> GetDeclarationsPotentiallyRequiringSetAss
             return relevantDeclarations;
         }
 
+        /// <summary>
+        /// Determines whether the 'Set' keyword needs to be added in the context of the specified identifier reference.
+        /// </summary>
+        /// <param name="reference">The identifier reference to analyze</param>
+        /// <param name="state">The parser state</param>
+        public static bool NeedsSetKeywordAdded(IdentifierReference reference, RubberduckParserState state)
+        {
+            var setStmtContext = reference.Context.GetAncestor<VBAParser.SetStmtContext>();
+            return RequiresSetAssignment(reference, state) && setStmtContext == null;
+        }
+
+        /// <summary>
+        /// Determines whether the 'Set' keyword is required (whether it's present or not) for the specified identifier reference.
+        /// </summary>
+        /// <param name="reference">The identifier reference to analyze</param>
+        /// <param name="state">The parser state</param>
         public static bool RequiresSetAssignment(IdentifierReference reference, RubberduckParserState state)
         {
             if (!reference.IsAssignment)
@@ -31,21 +47,14 @@ public static bool RequiresSetAssignment(IdentifierReference reference, Rubberdu
             var setStmtContext = reference.Context.GetAncestor<VBAParser.SetStmtContext>();
             if (setStmtContext != null)
             {
-                // assignment already has a Set keyword
-                return true;
-                // (but is it misplaced? ...hmmm... beyond the scope of *this* inspection though)
-                // if we're only ever assigning to 'Nothing', might as well flag it though
-                if (reference.Declaration.References.Where(r => r.IsAssignment).All(r =>
-                    r.Context.GetAncestor<VBAParser.SetStmtContext>().expression().GetText() == Tokens.Nothing))
-                {
-                    return true;
-                }
+                // don't assume Set keyword is legit...
+                return reference.Declaration.IsObject;
             }
 
             var letStmtContext = reference.Context.GetAncestor<VBAParser.LetStmtContext>();
             if (letStmtContext == null)
             {
-                // we're probably in a For Each loop
+                // not an assignment
                 return false;
             }
 

Rubberduck.Parsing/Symbols/Declaration.cs

@@ -339,13 +339,13 @@ private static string CorrectlyFormatedDescription(string literalDescription)
         /// </summary>
         public bool IsEnumeratorMember => _attributes.Any(a => a.Name.EndsWith("VB_UserMemId") && a.Values.Contains("-4"));
 
-        public virtual bool IsObject => 
-            AsTypeName == Tokens.Object
-            || (AsTypeDeclaration?.DeclarationType.HasFlag(DeclarationType.ClassModule) ?? false)
-            && !AsTypeIsBaseType 
-            && !IsArray 
-            && !DeclarationType.HasFlag(DeclarationType.UserDefinedType) 
-            && !DeclarationType.HasFlag(DeclarationType.Enumeration);
+        public virtual bool IsObject =>
+            AsTypeName == Tokens.Object || (
+                AsTypeDeclaration?.DeclarationType.HasFlag(DeclarationType.ClassModule) ?? 
+                    !AsTypeIsBaseType
+                    && !IsArray
+                    && !DeclarationType.HasFlag(DeclarationType.UserDefinedType)
+                    && !DeclarationType.HasFlag(DeclarationType.Enumeration));
 
         public void AddReference(
             QualifiedModuleName module,

Rubberduck.Parsing/Symbols/DeclarationFinder.cs

@@ -5,6 +5,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Diagnostics;
 using System.Linq;
 using Antlr4.Runtime;
@@ -1059,13 +1060,28 @@ private static bool IsSubroutineOrProperty(Declaration declaration)
                 || declaration.DeclarationType == DeclarationType.Procedure;
         }
 
+        /// <summary>
+        /// Creates a dictionary of identifier references, keyed by module.
+        /// </summary>
+        public IReadOnlyDictionary<QualifiedModuleName,IEnumerable<IdentifierReference>> IdentifierReferences()
+        {
+            return new ReadOnlyDictionary<QualifiedModuleName, IEnumerable<IdentifierReference>>(
+                _referencesByModule.ToDictionary(kvp => kvp.Key, kvp => kvp.Value.AsEnumerable()));
+        }
+
+        /// <summary>
+        /// Gets all identifier references in the specified module.
+        /// </summary>
         public IEnumerable<IdentifierReference> IdentifierReferences(QualifiedModuleName module)
         {
             return _referencesByModule.TryGetValue(module, out List<IdentifierReference> value)
                 ? value
                 : Enumerable.Empty<IdentifierReference>();
         }
 
+        /// <summary>
+        /// Gets all identifier references in the specified member.
+        /// </summary>
         public IEnumerable<IdentifierReference> IdentifierReferences(QualifiedMemberName member)
         {
             return _referencesByMember.TryGetValue(member, out List<IdentifierReference> value)

RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs

@@ -2,7 +2,6 @@
 using System.Threading;
 using NUnit.Framework;
 using Rubberduck.Inspections.Concrete;
-using Rubberduck.Parsing.VBA;
 using Rubberduck.VBEditor.SafeComWrappers;
 using RubberduckTests.Common;
 using RubberduckTests.Mocks;
@@ -13,22 +12,6 @@ namespace RubberduckTests.Inspections
     [DeploymentItem(@"TestFiles\")]
     public class ObjectVariableNotSetInspectionTests
     {
-        [Test]
-        [Category("Inspections")]
-        public void ObjectVariableNotSet_OnlyAssignedToNothing_ReturnsResult()
-        {
-            var expectResultCount = 1;
-            var input =
-                @"
-Private Sub DoSomething()
-    Dim target As Object
-    target.DoSomething ' error 91
-    Set target = Nothing
-End Sub
-";
-            AssertInputCodeYieldsExpectedInspectionResultCount(input, expectResultCount);
-        }
-
         [Test]
         [Category("Inspections")]
         public void ObjectVariableNotSet_AlsoAssignedToNothing_ReturnsNoResult()

RubberduckTests/QuickFixes/AssignedByValParameterMakeLocalCopyQuickFixTests.cs

@@ -360,7 +360,6 @@ private string ApplyLocalVariableQuickFixToCodeFragment(string inputCode, string
 
             using (var state = MockParser.CreateAndParse(vbe.Object))
             {
-
                 var inspection = new AssignedByValParameterInspection(state);
                 var inspectionResults = inspection.GetInspectionResults();
                 var result = inspectionResults.FirstOrDefault();