Rewrite the IgnoreOnceQuickfix

MDoerner · MDoerner · commit b011dff4c7b3 · 2018-12-02T20:25:07.000+01:00
This removes some bugs around module inspection results, removes the use of the code modules and makes the quickfix honor indentation.
Moreover, it now recognizes an existing Ignore annotation that it not the first one in an annotation list.
diff --git a/Rubberduck.CodeAnalysis/QuickFixes/IgnoreOnceQuickFix.cs b/Rubberduck.CodeAnalysis/QuickFixes/IgnoreOnceQuickFix.cs
@@ -1,10 +1,10 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using Antlr4.Runtime;
 using Antlr4.Runtime.Misc;
 using Antlr4.Runtime.Tree;
 using Rubberduck.Inspections.Abstract;
+using Rubberduck.Parsing.Annotations;
 using Rubberduck.Parsing.Grammar;
 using Rubberduck.Parsing.Inspections;
 using Rubberduck.Parsing.Inspections.Abstract;
@@ -31,97 +31,109 @@ public IgnoreOnceQuickFix(RubberduckParserState state, IEnumerable<IInspection>
 
         public override void Fix(IInspectionResult result, IRewriteSession rewriteSession)
         {
-            var annotationText = result.Target?.DeclarationType.HasFlag(DeclarationType.Module) == true
-                                    ? $"'@IgnoreModule {result.Inspection.AnnotationName}"
-                                    : $"'@Ignore {result.Inspection.AnnotationName}";
-
-            int annotationLine;
-            //TODO: Make this use the parse tree instead of the code module.
-            var component = _state.ProjectsProvider.Component(result.QualifiedSelection.QualifiedName);
-            using (var codeModule = component.CodeModule)
+            if (result.Target?.DeclarationType.HasFlag(DeclarationType.Module) == true)
             {
-                annotationLine = result.QualifiedSelection.Selection.StartLine;
-                while (annotationLine != 1 && codeModule.GetLines(annotationLine - 1, 1).EndsWith(" _"))
-                {
-                    annotationLine--;
-                }
+                FixModule(result, rewriteSession);
             }
+            else
+            {
+                FixNonModule(result, rewriteSession);
+            }
+        }
+
+        private void FixNonModule(IInspectionResult result, IRewriteSession rewriteSession)
+        {
+            int insertionIndex;
+            string insertText;
+            var annotationText = $"'@Ignore {result.Inspection.AnnotationName}";
 
             var module = result.QualifiedSelection.QualifiedName;
             var parseTree = _state.GetParseTree(module, CodeKind.CodePaneCode);
-
-            var listener = new CommentOrAnnotationListener();
-            ParseTreeWalker.Default.Walk(listener, parseTree);
-            var commentContext = listener.Contexts.LastOrDefault(i => i.Stop.TokenIndex <= result.Context.Start.TokenIndex);
-            var commented = commentContext?.Stop.Line + 1 == annotationLine;
+            var eolListener = new EndOfLineListener();
+            ParseTreeWalker.Default.Walk(eolListener, parseTree);
+            var previousEol = eolListener.Contexts
+                .OrderBy(eol => eol.Start.TokenIndex)
+                .LastOrDefault(eol => eol.Start.Line < result.QualifiedSelection.Selection.StartLine);
 
             var rewriter = rewriteSession.CheckOutModuleRewriter(module);
 
-            if (commented)
+            if (previousEol == null)
             {
-                var annotation = commentContext.annotationList()?.annotation(0);
-                if (annotation != null && annotation.GetText().StartsWith("Ignore"))
-                {
-                    rewriter.InsertAfter(annotation.annotationName().Stop.TokenIndex, $" {result.Inspection.AnnotationName},");
-                }
-                else
-                {
-                    var indent = new string(Enumerable.Repeat(' ', commentContext.Start.Column).ToArray());
-                    rewriter.InsertAfter(commentContext.Stop.TokenIndex, $"{indent}{annotationText}{Environment.NewLine}");
-                }
+                // The context to get annotated is on the first line; we need to insert before token index 0.
+                insertionIndex = 0;
+                insertText = annotationText + Environment.NewLine;
+                rewriter.InsertBefore(insertionIndex, insertText);
+                return;
             }
-            else
+
+            var commentContext = previousEol.commentOrAnnotation();
+            if (commentContext == null)
             {
-                int insertIndex;
-
-                // this value is used when the annotation should be on line 1--we need to insert before token index 0
-                if (annotationLine == 1)
-                {
-                    insertIndex = 0;
-                    annotationText += Environment.NewLine;
-                }
-                else
-                {
-                    var eol = new EndOfLineListener();
-                    ParseTreeWalker.Default.Walk(eol, parseTree);
-
-                    // we subtract 2 here to get the insertion index to A) account for VBE's one-based indexing
-                    // and B) to get the newline token that introduces that line
-                    var eolContext = eol.Contexts.OrderBy(o => o.Start.TokenIndex).ElementAt(annotationLine - 2);
-                    insertIndex = eolContext.Start.TokenIndex;
-
-                    annotationText = Environment.NewLine + annotationText;
-                }
-
-                rewriter.InsertBefore(insertIndex, annotationText);
+                insertionIndex = previousEol.Start.TokenIndex;
+                var indent = WhitespaceAfter(previousEol);
+                insertText = $"{Environment.NewLine}{indent}{annotationText}";
+                rewriter.InsertBefore(insertionIndex, insertText);
+                return;
             }
+
+            var ignoreAnnotation = commentContext.annotationList()?.annotation()
+                .FirstOrDefault(annotationContext => annotationContext.annotationName().GetText() == AnnotationType.Ignore.ToString());
+            if (ignoreAnnotation == null)
+            {
+                insertionIndex = commentContext.Stop.TokenIndex;
+                var indent = WhitespaceAfter(previousEol);
+                insertText = $"{indent}{annotationText}{Environment.NewLine}";
+                rewriter.InsertAfter(insertionIndex, insertText);
+                return;
+            }
+
+            insertionIndex = ignoreAnnotation.annotationName().Stop.TokenIndex;
+            insertText = $" {result.Inspection.AnnotationName},";
+            rewriter.InsertAfter(insertionIndex, insertText);
         }
 
-        public override string Description(IInspectionResult result) => Resources.Inspections.QuickFixes.IgnoreOnce;
+        private string WhitespaceAfter(VBAParser.EndOfLineContext endOfLine)
+        {
+            var individualEndOfStatement = (VBAParser.IndividualNonEOFEndOfStatementContext) endOfLine.Parent;
+            var whiteSpaceOnNextLine = individualEndOfStatement.whiteSpace(0);
+            return whiteSpaceOnNextLine != null
+                ? whiteSpaceOnNextLine.GetText()
+                : string.Empty;
+        }
 
-        private class CommentOrAnnotationListener : VBAParserBaseListener
+        private void FixModule(IInspectionResult result, IRewriteSession rewriteSession)
         {
-            private readonly IList<VBAParser.CommentOrAnnotationContext> _contexts = new List<VBAParser.CommentOrAnnotationContext>();
-            public IEnumerable<VBAParser.CommentOrAnnotationContext> Contexts => _contexts;
+            var module = result.QualifiedSelection.QualifiedName;
+            var moduleAnnotations = _state.GetModuleAnnotations(module);
+            var firstIgnoreModuleAnnotation = moduleAnnotations
+                .Where(annotation => annotation.AnnotationType == AnnotationType.IgnoreModule)
+                .OrderBy(annotation => annotation.Context.Start.TokenIndex)
+                .FirstOrDefault();
 
-            public override void ExitCommentOrAnnotation([NotNull] VBAParser.CommentOrAnnotationContext context)
+            var rewriter = rewriteSession.CheckOutModuleRewriter(module);
+
+            int insertionIndex;
+            string insertText;
+
+            if (firstIgnoreModuleAnnotation == null)
             {
-                _contexts.Add(context);
+                insertionIndex = 0;
+                insertText = $"'@IgnoreModule {result.Inspection.AnnotationName}{Environment.NewLine}";
+                rewriter.InsertBefore(insertionIndex, insertText);
+                return;
             }
+
+            insertionIndex = firstIgnoreModuleAnnotation.Context.annotationName().Stop.TokenIndex;
+            insertText = $" {result.Inspection.AnnotationName},";
+            rewriter.InsertAfter(insertionIndex, insertText);
         }
 
+        public override string Description(IInspectionResult result) => Resources.Inspections.QuickFixes.IgnoreOnce;
+
         private class EndOfLineListener : VBAParserBaseListener
         {
-            private readonly IList<ParserRuleContext> _contexts = new List<ParserRuleContext>();
-            public IEnumerable<ParserRuleContext> Contexts => _contexts;
-
-            public override void ExitWhiteSpace([NotNull] VBAParser.WhiteSpaceContext context)
-            {
-                if (context.GetText().Contains(Environment.NewLine))
-                {
-                    _contexts.Add(context);
-                }
-            }
+            private readonly IList<VBAParser.EndOfLineContext> _contexts = new List<VBAParser.EndOfLineContext>();
+            public IEnumerable<VBAParser.EndOfLineContext> Contexts => _contexts;
 
             public override void ExitEndOfLine([NotNull] VBAParser.EndOfLineContext context)
             {
diff --git a/Rubberduck.Parsing/Grammar/VBAParser.g4 b/Rubberduck.Parsing/Grammar/VBAParser.g4
@@ -898,7 +898,7 @@ statementKeyword :
 ;
 
 endOfLine :
-    whiteSpace? NEWLINE whiteSpace?
+    whiteSpace? NEWLINE
     | whiteSpace? commentOrAnnotation
 ;
 
diff --git a/RubberduckTests/QuickFixes/IgnoreOnceQuickFixTests.cs b/RubberduckTests/QuickFixes/IgnoreOnceQuickFixTests.cs
@@ -31,7 +31,7 @@ Dim foo As Double
             const string expectedCode =
                 @"Sub ExcelSub()
     Dim foo As Double
-'@Ignore ApplicationWorksheetFunction
+    '@Ignore ApplicationWorksheetFunction
     foo = Application.Pi
 End Sub";
 
@@ -69,7 +69,7 @@ public void ConstantNotUsed_IgnoreQuickFixWorks()
 
             const string expectedCode =
                 @"Public Sub Foo()
-'@Ignore ConstantNotUsed
+    '@Ignore ConstantNotUsed
     Const const1 As Integer = 9
 End Sub";
 
@@ -88,7 +88,7 @@ public void EmptyStringLiteral_IgnoreQuickFixWorks()
 
             const string expectedCode =
                 @"Public Sub Foo(ByRef arg1 As String)
-'@Ignore EmptyStringLiteral
+    '@Ignore EmptyStringLiteral
     arg1 = """"
 End Sub";
 
@@ -174,7 +174,7 @@ Dim arr1() As Variant
             const string expectedCode =
                 @"Sub foo()
     Dim arr1() As Variant
-'@Ignore ImplicitActiveSheetReference
+    '@Ignore ImplicitActiveSheetReference
     arr1 = Range(""A1:B2"")
 End Sub";
 
@@ -197,7 +197,7 @@ Dim sheet As Worksheet
                 @"
 Sub foo()
     Dim sheet As Worksheet
-'@Ignore ImplicitActiveWorkbookReference
+    '@Ignore ImplicitActiveWorkbookReference
     Set sheet = Worksheets(""Sheet1"")
 End Sub";
 
@@ -356,7 +356,7 @@ public void MultipleDeclarations_IgnoreQuickFixWorks()
 
             const string expectedCode =
                 @"Public Sub Foo()
-'@Ignore MultipleDeclarations
+    '@Ignore MultipleDeclarations
     Dim var1 As Integer, var2 As String
 End Sub";
 
@@ -401,7 +401,7 @@ Dim target As Object
 Private Sub DoSomething()
     
     Dim target As Object
-'@Ignore ObjectVariableNotSet
+    '@Ignore ObjectVariableNotSet
     target = New Object
     
     target.Value = ""forgot something?""
@@ -428,12 +428,12 @@ Call Foo
 
             const string expectedCode =
                 @"Sub Foo()
-'@Ignore ObsoleteCallStatement
+    '@Ignore ObsoleteCallStatement
     Call Goo(1, ""test"")
 End Sub
 
 Sub Goo(arg1 As Integer, arg1 As String)
-'@Ignore ObsoleteCallStatement
+    '@Ignore ObsoleteCallStatement
     Call Foo
 End Sub";
 
@@ -467,7 +467,7 @@ Error 91
 
             const string expectedCode =
                 @"Sub Foo()
-'@Ignore ObsoleteErrorSyntax
+    '@Ignore ObsoleteErrorSyntax
     Error 91
 End Sub";
 
@@ -508,7 +508,7 @@ Dim var2 As Integer
     Dim var1 As Integer
     Dim var2 As Integer
     
-'@Ignore ObsoleteLetStatement
+    '@Ignore ObsoleteLetStatement
     Let var2 = var1
 End Sub";
 
@@ -657,7 +657,7 @@ Dim b As New Collection
 
             const string expectedCode =
                 @"Sub Foo()
-'@Ignore SelfAssignedDeclaration
+    '@Ignore SelfAssignedDeclaration
     Dim b As New Collection
 End Sub";
 
@@ -681,7 +681,7 @@ Dim bb As Boolean
                 @"Sub Foo()
     Dim b As Boolean
     Dim bb As Boolean
-'@Ignore UnassignedVariableUsage
+    '@Ignore UnassignedVariableUsage
     bb = b
 End Sub";
 
@@ -896,7 +896,7 @@ If True Then
             const string expectedCode =
                 @"Sub Foo()
     Dim d As Boolean
-'@Ignore BooleanAssignedInIfElse
+    '@Ignore BooleanAssignedInIfElse
     If True Then
         d = True
     Else
@@ -928,6 +928,60 @@ public void IgnoreQuickFixAppendsToExistingAnnotation()
             Assert.AreEqual(expectedCode, actualCode);
         }
 
+        [Test]
+        [Category("QuickFixes")]
+        public void IgnoreQuickFixPrependsToExistingAnnotation_Module()
+        {
+            const string inputCode =
+                @"'@IgnoreModule EmptyModule
+Option Explicit";
+
+            const string expectedCode =
+                @"'@IgnoreModule ModuleWithoutFolder, EmptyModule
+Option Explicit";
+
+            var actualCode = ApplyIgnoreOnceToFirstResult(inputCode, state => new ModuleWithoutFolderInspection(state), TestStandardModuleVbeSetup);
+            Assert.AreEqual(expectedCode, actualCode);
+        }
+
+        [Test]
+        [Category("QuickFixes")]
+        public void IgnoreQuickFixDoesNotAppendToExistingAnnotationMixed_ModuleAfterNonModule()
+        {
+            const string inputCode =
+                @"'@Ignore ParameterCanBeByVal
+Private Sub Foo(arg)
+End Sub";
+
+            const string expectedCode =
+                @"'@IgnoreModule ModuleWithoutFolder
+'@Ignore ParameterCanBeByVal
+Private Sub Foo(arg)
+End Sub";
+
+            var actualCode = ApplyIgnoreOnceToFirstResult(inputCode, state => new ModuleWithoutFolderInspection(state), TestStandardModuleVbeSetup);
+            Assert.AreEqual(expectedCode, actualCode);
+        }
+
+        [Test]
+        [Category("QuickFixes")]
+        public void IgnoreQuickFixDoesNotPrependToExistingAnnotationMixed_NonModuleAfterModule()
+        {
+            const string inputCode =
+                @"'@IgnoreModule ModuleWithoutFolder
+Private Sub Foo(arg)
+End Sub";
+
+            const string expectedCode =
+                @"'@IgnoreModule ModuleWithoutFolder
+'@Ignore ParameterCanBeByVal
+Private Sub Foo(arg)
+End Sub";
+
+            var actualCode = ApplyIgnoreOnceToFirstResult(inputCode, state => new ParameterCanBeByValInspection(state), TestStandardModuleVbeSetup);
+            Assert.AreEqual(expectedCode, actualCode);
+        }
+
         [Test]
         [Category("QuickFixes")]
         public void IgnoreQuickFixAddsAnnotationAfterComment()

Original file line number	Diff line number	Diff line change
`@@ -898,7 +898,7 @@ statementKeyword :`
`898`	`898`	`;`
`899`	`899`
`900`	`900`	`endOfLine :`
`901`		`- whiteSpace? NEWLINE whiteSpace?`
	`901`	`+ whiteSpace? NEWLINE`
`902`	`902`	`\| whiteSpace? commentOrAnnotation`
`903`	`903`	`;`
`904`	`904`