Scope annotations to next non-whitespace line

MDoerner · MDoerner · commit 84cf953b3bb1 · 2018-12-18T02:15:28.000+01:00
This also deems all annotations on non-whitespace lines illegal.

The reasoning behind this change is that the previous system was somewhat unintuitive and fragile in that comments and non-member annotations could break a member annotation section. This is inconvenient in particular when removing annotations with a comment on the same line. Moreover, the new scoping is better aligned with VBE's own procedure separators.

As a side-effect, the logic for determining the annotated line has been centralized and simplified. In addition, stacking identifier annotations along a procedure is no longer possible, which could be problematic before.
diff --git a/Rubberduck.CodeAnalysis/Inspections/Abstract/InspectionBase.cs b/Rubberduck.CodeAnalysis/Inspections/Abstract/InspectionBase.cs
@@ -86,28 +86,22 @@ protected virtual IEnumerable<Declaration> BuiltInDeclarations
 
         protected bool IsIgnoringInspectionResultFor(QualifiedModuleName module, int line)
         {
-            var annotations = State.GetModuleAnnotations(module).ToList();
-
-            if (State.GetModuleAnnotations(module) == null)
-            {
-                return false;
-            }
-
-            // VBE 1-based indexing
-            for (var i = line; i >= 1; i--)
+            var lineScopedAnnotations = State.DeclarationFinder.FindAnnotations(module, line);
+            foreach (var ignoreAnnotation in lineScopedAnnotations.OfType<IgnoreAnnotation>())
             {
-                var annotation = annotations.SingleOrDefault(a => a.QualifiedSelection.Selection.StartLine == i);
-                var ignoreAnnotation = annotation as IgnoreAnnotation;
-                var ignoreModuleAnnotation = annotation as IgnoreModuleAnnotation;
-
-                if (ignoreAnnotation?.InspectionNames.Contains(AnnotationName) == true)
+                if (ignoreAnnotation.InspectionNames.Contains(AnnotationName))
                 {
                     return true;
                 }
+            }
+
+            var moduleDeclaration = State.DeclarationFinder.Members(module)
+                .First(decl => decl.DeclarationType.HasFlag(DeclarationType.Module));
 
-                if (ignoreModuleAnnotation != null
-                    && (ignoreModuleAnnotation.InspectionNames.Contains(AnnotationName)
-                    || !ignoreModuleAnnotation.InspectionNames.Any()))
+            foreach (var ignoreModuleAnnotation in moduleDeclaration.Annotations.OfType<IgnoreModuleAnnotation>())
+            {
+                if (ignoreModuleAnnotation.InspectionNames.Contains(AnnotationName)
+                    || !ignoreModuleAnnotation.InspectionNames.Any())
                 {
                     return true;
                 }
@@ -119,7 +113,10 @@ protected bool IsIgnoringInspectionResultFor(QualifiedModuleName module, int lin
         protected bool IsIgnoringInspectionResultFor(Declaration declaration, string inspectionName)
         {
             var module = Declaration.GetModuleParent(declaration);
-            if (module == null) { return false; }
+            if (module == null)
+            {
+                return false;
+            }
 
             var isIgnoredAtModuleLevel = module.Annotations
                     .Any(annotation => annotation.AnnotationType == AnnotationType.IgnoreModule
diff --git a/Rubberduck.CodeAnalysis/Inspections/Concrete/IllegalAnnotationInspection.cs b/Rubberduck.CodeAnalysis/Inspections/Concrete/IllegalAnnotationInspection.cs
@@ -25,7 +25,8 @@ protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
             var annotations = State.AllAnnotations;
 
             var illegalAnnotations = UnboundAnnotations(annotations, userDeclarations, identifierReferences)
-                .Where(annotation => !annotation.AnnotationType.HasFlag(AnnotationType.GeneralAnnotation));
+                .Where(annotation => !annotation.AnnotationType.HasFlag(AnnotationType.GeneralAnnotation)
+                                        || annotation.AnnotatedLine == null);
 
             return illegalAnnotations.Select(annotation => 
                 new QualifiedContextInspectionResult(
diff --git a/Rubberduck.Parsing/Annotations/AnnotationBase.cs b/Rubberduck.Parsing/Annotations/AnnotationBase.cs
@@ -1,4 +1,5 @@
-﻿using Rubberduck.Parsing.Grammar;
+﻿using System;
+using Rubberduck.Parsing.Grammar;
 using Rubberduck.VBEditor;
 
 namespace Rubberduck.Parsing.Annotations
@@ -7,19 +8,46 @@ public abstract class AnnotationBase : IAnnotation
     {
         public const string ANNOTATION_MARKER = "'@";
 
+        private readonly Lazy<int?> _annotatedLine;
+
         protected AnnotationBase(AnnotationType annotationType, QualifiedSelection qualifiedSelection, VBAParser.AnnotationContext context)
         {
             AnnotationType = annotationType;
             QualifiedSelection = qualifiedSelection;
             Context = context;
+            _annotatedLine = new Lazy<int?>(GetAnnotatedLine);
         }
 
         public AnnotationType AnnotationType { get; }
         public QualifiedSelection QualifiedSelection { get; }
         public VBAParser.AnnotationContext Context { get; }
 
+        public int? AnnotatedLine => _annotatedLine.Value;
+
         public virtual bool AllowMultiple { get; } = false;
 
         public override string ToString() => $"Annotation Type: {AnnotationType}";
+
+
+        private int? GetAnnotatedLine()
+        {
+            var enclosingEndOfStatement = Context.GetAncestor<VBAParser.EndOfStatementContext>();
+
+            //Annotations on the same line as non-whitespace statements do not scope to anything.
+            if (enclosingEndOfStatement.Start.TokenIndex != 0)
+            {
+                var firstEndOfLine = enclosingEndOfStatement.GetDescendent<VBAParser.EndOfLineContext>();
+                var parentEndOfLine = Context.GetAncestor<VBAParser.EndOfLineContext>();
+                if (firstEndOfLine.Equals(parentEndOfLine))
+                {
+                    return null;
+                }
+            }
+
+            var lastToken = enclosingEndOfStatement.stop;
+            return lastToken.Type == VBAParser.NEWLINE 
+                   ? lastToken.Line + 1
+                   : lastToken.Line;
+        }
     }
 }
diff --git a/Rubberduck.Parsing/Annotations/IAnnotation.cs b/Rubberduck.Parsing/Annotations/IAnnotation.cs
@@ -9,5 +9,6 @@ public interface IAnnotation
         QualifiedSelection QualifiedSelection { get; }
         bool AllowMultiple { get; }
         VBAParser.AnnotationContext Context { get; }
+        int? AnnotatedLine { get; }
     }
 }
diff --git a/Rubberduck.Parsing/Annotations/IdentifierAnnotationService.cs b/Rubberduck.Parsing/Annotations/IdentifierAnnotationService.cs
@@ -16,24 +16,7 @@ public IdentifierAnnotationService(DeclarationFinder declarationFinder)
 
         public IEnumerable<IAnnotation> FindAnnotations(QualifiedModuleName module, int line)
         {
-            var annotations = new List<IAnnotation>();
-            var moduleAnnotations = _declarationFinder.FindAnnotations(module).ToList();
-            // VBE 1-based indexing
-            for (var currentLine = line - 1; currentLine >= 1; currentLine--)
-            {
-                //Identifier annotation sections end at the first line above without an identifier annotation.
-                if (!moduleAnnotations.Any(annotation => annotation.QualifiedSelection.Selection.StartLine <= currentLine
-                                                            && annotation.QualifiedSelection.Selection.EndLine >= currentLine
-                                                            && annotation.AnnotationType.HasFlag(AnnotationType.IdentifierAnnotation)))
-                {
-                    break;
-                }
-
-                var annotationsStartingOnCurrentLine = moduleAnnotations.Where(a => a.QualifiedSelection.Selection.StartLine == currentLine && a.AnnotationType.HasFlag(AnnotationType.IdentifierAnnotation));
-
-                annotations.AddRange(annotationsStartingOnCurrentLine);
-            }
-            return annotations;
+            return _declarationFinder.FindAnnotations(module, line).Where(annotation => annotation.AnnotationType.HasFlag(AnnotationType.IdentifierAnnotation));
         }
     }
 }
diff --git a/Rubberduck.Parsing/VBA/DeclarationCaching/DeclarationFinder.cs b/Rubberduck.Parsing/VBA/DeclarationCaching/DeclarationFinder.cs
@@ -28,7 +28,7 @@ public class DeclarationFinder
         private readonly ConcurrentDictionary<QualifiedMemberName, ConcurrentBag<Declaration>> _newUndeclared;
         private readonly ConcurrentBag<UnboundMemberDeclaration> _newUnresolved;
         private List<UnboundMemberDeclaration> _unresolved;
-        private IDictionary<QualifiedModuleName, List<IAnnotation>> _annotations;
+        private IDictionary<(QualifiedModuleName module, int annotatedLine), List<IAnnotation>> _annotations;
         private IDictionary<Declaration, List<ParameterDeclaration>> _parametersByParent;
         private IDictionary<DeclarationType, List<Declaration>> _userDeclarationsByType;
         private IDictionary<QualifiedSelection, List<Declaration>> _declarationsBySelection;
@@ -92,7 +92,8 @@ private List<Action> CollectionConstructionActions(IReadOnlyList<Declaration> de
                         .ToList(),
                 () =>
                     _annotations = annotations
-                        .GroupBy(node => node.QualifiedSelection.QualifiedName)
+                        .Where(a => a.AnnotatedLine.HasValue)
+                        .GroupBy(a => (a.QualifiedSelection.QualifiedName, a.AnnotatedLine.Value))
                         .ToDictionary(),
                 () =>
                     _declarations = declarations
@@ -489,9 +490,9 @@ public IEnumerable<Declaration> FindMemberMatches(Declaration parent, string mem
                 : Enumerable.Empty<Declaration>();
         }
 
-        public IEnumerable<IAnnotation> FindAnnotations(QualifiedModuleName module)
+        public IEnumerable<IAnnotation> FindAnnotations(QualifiedModuleName module, int annotatedLine)
         {
-            return _annotations.TryGetValue(module, out var result) 
+            return _annotations.TryGetValue((module, annotatedLine), out var result) 
                 ? result 
                 : Enumerable.Empty<IAnnotation>();
         }
diff --git a/Rubberduck.Parsing/VBA/DeclarationResolving/DeclarationResolveRunnerBase.cs b/Rubberduck.Parsing/VBA/DeclarationResolving/DeclarationResolveRunnerBase.cs
@@ -140,11 +140,14 @@ protected void ResolveDeclarations(QualifiedModuleName module, IParseTree tree,
                 }
                 Logger.Debug($"Creating declarations for module {module.Name}.");
 
-                var annotations = _state.GetModuleAnnotations(module).ToList();
+                var annotationsOnWhiteSpaceLines = _state.GetModuleAnnotations(module)
+                    .Where(a => a.AnnotatedLine.HasValue)
+                    .GroupBy(a => a.AnnotatedLine.Value)
+                    .ToDictionary();
                 var attributes = _state.GetModuleAttributes(module);
                 var membersAllowingAttributes = _state.GetMembersAllowingAttributes(module);
 
-                var moduleDeclaration = NewModuleDeclaration(module, tree, annotations, attributes, projectDeclaration);
+                var moduleDeclaration = NewModuleDeclaration(module, tree, annotationsOnWhiteSpaceLines, attributes, projectDeclaration);
                 _state.AddDeclaration(moduleDeclaration);
 
                 var controlDeclarations = DeclarationsFromControls(moduleDeclaration);
@@ -153,7 +156,7 @@ protected void ResolveDeclarations(QualifiedModuleName module, IParseTree tree,
                     _state.AddDeclaration(declaration);
                 }
 
-                var declarationsListener = new DeclarationSymbolsListener(moduleDeclaration, annotations, attributes, membersAllowingAttributes);
+                var declarationsListener = new DeclarationSymbolsListener(moduleDeclaration, annotationsOnWhiteSpaceLines, attributes, membersAllowingAttributes);
                 ParseTreeWalker.Default.Walk(declarationsListener, tree);
                 foreach (var createdDeclaration in declarationsListener.CreatedDeclarations)
                 {
@@ -172,13 +175,13 @@ protected void ResolveDeclarations(QualifiedModuleName module, IParseTree tree,
         private ModuleDeclaration NewModuleDeclaration(
             QualifiedModuleName qualifiedModuleName,
             IParseTree tree,
-            ICollection<IAnnotation> annotations,
+            IDictionary<int, List<IAnnotation>> annotationsOnWhiteSpaceLines,
             IDictionary<(string scopeIdentifier, DeclarationType scopeType),
                 Attributes> attributes,
             Declaration projectDeclaration)
         {
             var moduleAttributes = ModuleAttributes(qualifiedModuleName, attributes);
-            var moduleAnnotations = FindModuleAnnotations(tree, annotations);
+            var moduleAnnotations = FindModuleAnnotations(tree, annotationsOnWhiteSpaceLines);
 
             switch (qualifiedModuleName.ComponentType)
             {
@@ -222,34 +225,30 @@ private static Attributes ModuleAttributes(QualifiedModuleName qualifiedModuleNa
             return moduleAttributes;
         }
 
-        private static IEnumerable<IAnnotation> FindModuleAnnotations(IParseTree tree, ICollection<IAnnotation> annotations)
+        private static IEnumerable<IAnnotation> FindModuleAnnotations(IParseTree tree, IDictionary<int, List<IAnnotation>> annotationsOnWhiteSpaceLines)
         {
-            if (annotations == null)
+            if (annotationsOnWhiteSpaceLines == null)
             {
                 return null;
             }
 
-            var potentialModuleAnnotations = annotations.Where(annotation =>
-                annotation.AnnotationType.HasFlag(AnnotationType.ModuleAnnotation));
+            var firstModuleBodyLine = FirstModuleBodyElementLine(tree);
 
-            var lastPossibleDeclarationsSectionLine = LastPossibleDeclarationsSectionLine(tree);
-
-            //There is no module body.
-            if (lastPossibleDeclarationsSectionLine == null)
+            //There is no module body and, thus, no restrictions on the placement of module annotations on whitespace lines.
+            if (firstModuleBodyLine == null)
             {
-                return potentialModuleAnnotations;
+                return annotationsOnWhiteSpaceLines.Values.SelectMany(annotationList => annotationList)
+                    .Where(annotation => annotation.AnnotationType.HasFlag(AnnotationType.ModuleAnnotation));
             }
 
-            var lastPossibleModuleAnnotationLine = lastPossibleDeclarationsSectionLine.Value;
-            var moduleAnnotations = potentialModuleAnnotations.Where(annotation => annotation.QualifiedSelection.Selection.EndLine <= lastPossibleModuleAnnotationLine);
+            var lastPossibleAnnotatedLine = firstModuleBodyLine.Value;
+            var moduleAnnotations = annotationsOnWhiteSpaceLines.Keys
+                .Where(line => (line <= lastPossibleAnnotatedLine))
+                .SelectMany(line => annotationsOnWhiteSpaceLines[line])
+                .Where(annotation => annotation.AnnotationType.HasFlag(AnnotationType.ModuleAnnotation));
             return moduleAnnotations;
         }
 
-        private static int? LastPossibleDeclarationsSectionLine(IParseTree tree)
-        {
-            return FirstModuleBodyElementLine(tree) - 1;
-        }
-
         private static int? FirstModuleBodyElementLine(IParseTree tree)
         {
             var startContext = (ParserRuleContext)tree;
diff --git a/Rubberduck.Parsing/VBA/DeclarationResolving/DeclarationSymbolsListener.cs b/Rubberduck.Parsing/VBA/DeclarationResolving/DeclarationSymbolsListener.cs
@@ -18,7 +18,7 @@ public class DeclarationSymbolsListener : VBAParserBaseListener
         private Declaration _currentScopeDeclaration;
         private Declaration _parentDeclaration;
 
-        private readonly ICollection<IAnnotation> _annotations;
+        private readonly IDictionary<int, List<IAnnotation>> _annotations;
         private readonly IDictionary<(string scopeIdentifier, DeclarationType scopeType), Attributes> _attributes;
         private readonly IDictionary<(string scopeIdentifier, DeclarationType scopeType), ParserRuleContext> _membersAllowingAttributes;
 
@@ -27,7 +27,7 @@ public class DeclarationSymbolsListener : VBAParserBaseListener
 
         public DeclarationSymbolsListener(
             Declaration moduleDeclaration,
-            ICollection<IAnnotation> annotations,
+            IDictionary<int, List<IAnnotation>> annotations,
             IDictionary<(string scopeIdentifier, DeclarationType scopeType),
             Attributes> attributes,
             IDictionary<(string scopeIdentifier, DeclarationType scopeType),
@@ -54,28 +54,12 @@ private IEnumerable<IAnnotation> FindAnnotations(int firstLine, AnnotationType a
                 return null;
             }
 
-            var annotations = new List<IAnnotation>();
-
-            // VBE 1-based indexing
-            for (var currentLine = firstLine - 1; currentLine >= 1; currentLine--)
+            if (_annotations.TryGetValue(firstLine, out var scopedAnnotations))
             {
-                //Annotation sections end at the first line without an annotation with the specified flag or identifier annotation.
-                //Identifier annotations are treated in a special way because identifier references can appear on the same line as other constructs that can be annotated.
-                if (!_annotations.Any(annotation => annotation.QualifiedSelection.Selection.StartLine <= currentLine
-                                                    && annotation.QualifiedSelection.Selection.EndLine >= currentLine
-                                                    && (annotation.AnnotationType.HasFlag(annotationTypeFlag)
-                                                        || annotation.AnnotationType.HasFlag(AnnotationType.IdentifierAnnotation))))
-                {
-                    break;
-                }
-
-                var annotationsStartingOnCurrentLine = _annotations.Where(a => a.QualifiedSelection.Selection.StartLine == currentLine
-                                                                               && a.AnnotationType.HasFlag(annotationTypeFlag));
-
-                annotations.AddRange(annotationsStartingOnCurrentLine);
+                return scopedAnnotations.Where(annotation => annotation.AnnotationType.HasFlag(annotationTypeFlag));
             }
 
-            return annotations;
+            return Enumerable.Empty<IAnnotation>();
         }
 
         private IEnumerable<IAnnotation> FindVariableAnnotations(int firstVariableLine)
diff --git a/RubberduckTests/Annotations/AnnotationResolutionTests.cs b/RubberduckTests/Annotations/AnnotationResolutionTests.cs
diff --git a/RubberduckTests/Inspections/DefTypeStatementInspectionTests.cs b/RubberduckTests/Inspections/DefTypeStatementInspectionTests.cs
diff --git a/RubberduckTests/Inspections/IllegalAnnotationsInspectionTests.cs b/RubberduckTests/Inspections/IllegalAnnotationsInspectionTests.cs

Original file line number	Diff line number	Diff line change
`@@ -9,5 +9,6 @@ public interface IAnnotation`
`9`	`9`	`QualifiedSelection QualifiedSelection { get; }`
`10`	`10`	`bool AllowMultiple { get; }`
`11`	`11`	`VBAParser.AnnotationContext Context { get; }`
	`12`	`+ int? AnnotatedLine { get; }`
`12`	`13`	`}`
`13`	`14`	`}`