Stop using stale state in UnreachableCaseInspection

Also removes the special testing interface from ParseTreeValueVisitor.

Author: MDoerner

Rubberduck.CodeAnalysis/Inspections/Concrete/UnreachableCaseInspection/ParseTreeValueVisitor.cs

@@ -10,16 +10,9 @@
 namespace Rubberduck.Inspections.Concrete.UnreachableCaseInspection
 {
     public interface IParseTreeValueVisitor : IParseTreeVisitor<IParseTreeVisitorResults>
-    {
-        event EventHandler<ValueResultEventArgs> OnValueResultCreated;
-    }
-
-    public interface ITestParseTreeVisitor
-    {
-        void InjectValuedDeclarationEvaluator(Func<Declaration, (bool, string, string)> func);
-    }
+    {}
 
-    public class ParseTreeValueVisitor : IParseTreeValueVisitor, ITestParseTreeVisitor
+    public class ParseTreeValueVisitor : IParseTreeValueVisitor
     {
         private class EnumMember
         {
@@ -29,28 +22,31 @@ public EnumMember(VBAParser.EnumerationStmt_ConstantContext constContext, long i
                 Value = initValue;
                 HasAssignment = constContext.children.Any(ch => ch.Equals(constContext.GetToken(VBAParser.EQ, 0)));
             }
-            public VBAParser.EnumerationStmt_ConstantContext ConstantContext { set; get; }
+            public VBAParser.EnumerationStmt_ConstantContext ConstantContext { get; }
             public long Value { set; get; }
-            public bool HasAssignment { set; get; }
+            public bool HasAssignment { get; }
         }
 
-        private IParseTreeVisitorResults _contextValues;
-        private IParseTreeValueFactory _inspValueFactory;
-        private IReadOnlyList<VBAParser.EnumerationStmtContext> _enumStmtContexts;
+        private readonly IParseTreeVisitorResults _contextValues;
+        private readonly IParseTreeValueFactory _inspValueFactory;
+        private readonly IReadOnlyList<VBAParser.EnumerationStmtContext> _enumStmtContexts;
         private List<EnumMember> _enumMembers;
+        private Func<Declaration, (bool, string, string)> _valueDeclarationEvaluator;
 
-        public ParseTreeValueVisitor(IParseTreeValueFactory valueFactory, IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, Func<ParserRuleContext, (bool success, IdentifierReference idRef)> idRefRetriever)
+        public ParseTreeValueVisitor(
+            IParseTreeValueFactory valueFactory, 
+            IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, 
+            Func<ParserRuleContext, (bool success, IdentifierReference idRef)> idRefRetriever, 
+            Func<Declaration, (bool, string, string)> valueDeclarationEvaluator = null)
         {
             _inspValueFactory = valueFactory;
             IdRefRetriever = idRefRetriever;
             _contextValues = new ParseTreeVisitorResults();
-            OnValueResultCreated += _contextValues.OnNewValueResult;
             _enumStmtContexts = allEnums;
+            _valueDeclarationEvaluator = valueDeclarationEvaluator ?? GetValuedDeclaration;
         }
 
-        private Func<ParserRuleContext, (bool success, IdentifierReference idRef)> IdRefRetriever { set; get; } = null;
-
-        public event EventHandler<ValueResultEventArgs> OnValueResultCreated;
+        private Func<ParserRuleContext, (bool success, IdentifierReference idRef)> IdRefRetriever { get; }
 
         public virtual IParseTreeVisitorResults Visit(IParseTree tree)
         {
@@ -85,7 +81,7 @@ public virtual IParseTreeVisitorResults VisitErrorNode(IErrorNode node)
 
         private void StoreVisitResult(ParserRuleContext context, IParseTreeValue inspValue)
         {
-            OnValueResultCreated(this, new ValueResultEventArgs(context, inspValue));
+            _contextValues.AddIfNotPresent(context, inspValue);
         }
 
         private bool HasResult(ParserRuleContext context)
@@ -141,7 +137,6 @@ private void Visit(VBAParser.LExprContext context)
             }
             else
             {
-                var smplNameExprTypeName = string.Empty;
                 var smplName = context.GetDescendent<VBAParser.SimpleNameExprContext>();
                 if (TryGetIdentifierReferenceForContext(smplName, out IdentifierReference idRef))
                 {
@@ -286,20 +281,6 @@ private bool TryGetLExprValue(VBAParser.LExprContext lExprContext, out string ex
             return false;
         }
 
-        private Func<Declaration, (bool, string, string)> _valueDeclarationEvaluator;
-        private Func<Declaration, (bool, string, string)> ValuedDeclarationEvaluator
-        {
-            set
-            {
-                _valueDeclarationEvaluator = value;
-            }
-            get
-            {
-                return _valueDeclarationEvaluator ?? GetValuedDeclaration;
-            }
-        }
-
-
         private (bool IsType, string ExpressionValue, string TypeName) GetValuedDeclaration(Declaration declaration)
         {
             if (declaration is ValuedDeclaration valuedDeclaration)
@@ -321,7 +302,7 @@ private void GetContextValue(ParserRuleContext context, out string declaredTypeN
                 expressionValue = rangeClauseIdentifierReference.IdentifierName;
                 declaredTypeName = GetBaseTypeForDeclaration(declaration);
 
-                (bool IsValuedDeclaration, string ExpressionValue, string TypeName) = ValuedDeclarationEvaluator(declaration);
+                (bool IsValuedDeclaration, string ExpressionValue, string TypeName) = _valueDeclarationEvaluator(declaration);
 
                 if( IsValuedDeclaration)
                 {
@@ -334,7 +315,8 @@ private void GetContextValue(ParserRuleContext context, out string declaredTypeN
                         declaredTypeName = Tokens.String;
                         return;
                     }
-                    else if (long.TryParse(expressionValue, out _))
+
+                    if (long.TryParse(expressionValue, out _))
                     {
                         return;
                     }
@@ -437,9 +419,6 @@ private static bool IsBinaryOpEvaluationContext<T>(T context)
             return false;
         }
 
-        public void InjectValuedDeclarationEvaluator( Func<Declaration, (bool, string, string)> func)
-            => ValuedDeclarationEvaluator = func;
-
         private void LoadEnumMemberValues()
         {
             _enumMembers = new List<EnumMember>();

Rubberduck.CodeAnalysis/Inspections/Concrete/UnreachableCaseInspection/ParseTreeValueVisitorFactory.cs

@@ -0,0 +1,28 @@
+using System;
+using System.Collections.Generic;
+using Antlr4.Runtime;
+using Rubberduck.Parsing.Grammar;
+using Rubberduck.Parsing.Symbols;
+
+namespace Rubberduck.Inspections.Concrete.UnreachableCaseInspection
+{
+    public interface IParseTreeValueVisitorFactory
+    {
+        IParseTreeValueVisitor Create(IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, Func<ParserRuleContext, (bool success, IdentifierReference idRef)> idRefRetriever);
+    }
+
+    public class ParseTreeValueVisitorFactory : IParseTreeValueVisitorFactory
+    {
+        private readonly IParseTreeValueFactory _valueFactory;
+
+        public ParseTreeValueVisitorFactory(IParseTreeValueFactory valueFactory)
+        {
+            _valueFactory = valueFactory;
+        }
+
+        public IParseTreeValueVisitor Create(IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, Func<ParserRuleContext, (bool success, IdentifierReference idRef)> identifierReferenceRetriever)
+        {
+            return new ParseTreeValueVisitor(_valueFactory, allEnums, identifierReferenceRetriever);
+        }
+    }
+}

Rubberduck.CodeAnalysis/Inspections/Concrete/UnreachableCaseInspection/ParseTreeVisitorResults.cs

@@ -1,6 +1,7 @@
 using Antlr4.Runtime;
 using System;
 using System.Collections.Generic;
+using System.Linq;
 
 namespace Rubberduck.Inspections.Concrete.UnreachableCaseInspection
 {
@@ -12,7 +13,7 @@ public interface IParseTreeVisitorResults
         string GetToken(ParserRuleContext context);
         bool Contains(ParserRuleContext context);
         bool TryGetValue(ParserRuleContext context, out IParseTreeValue value);
-        void OnNewValueResult(object sender, ValueResultEventArgs e);
+        void AddIfNotPresent(ParserRuleContext context, IParseTreeValue value);
     }
 
     public class ParseTreeVisitorResults : IParseTreeVisitorResults
@@ -35,18 +36,15 @@ public IParseTreeValue GetValue(ParserRuleContext context)
 
         public List<ParserRuleContext> GetChildResults(ParserRuleContext parent)
         {
-            var results = new List<ParserRuleContext>();
-            if (!(parent is null) || Contains(parent))
+            if (parent is null)
             {
-                foreach (var child in parent.children)
-                {
-                    if (child is ParserRuleContext prCtxt  && Contains(prCtxt))
-                    {
-                        results.Add(prCtxt);
-                    }
-                }
+                return new List<ParserRuleContext>();
             }
-            return results;
+
+            return parent.children
+                .OfType<ParserRuleContext>()
+                .Where(Contains)
+                .ToList();
         }
 
         public string GetValueType(ParserRuleContext context)
@@ -69,11 +67,11 @@ public bool TryGetValue(ParserRuleContext context, out IParseTreeValue value)
             return _parseTreeValues.TryGetValue(context, out value);
         }
 
-        public void OnNewValueResult(object sender, ValueResultEventArgs e)
+        public void AddIfNotPresent(ParserRuleContext context, IParseTreeValue value)
         {
-            if (!_parseTreeValues.ContainsKey(e.Context))
+            if (!_parseTreeValues.ContainsKey(context))
             {
-                _parseTreeValues.Add(e.Context, e.Value);
+                _parseTreeValues.Add(context, value);
             }
         }
     }

Rubberduck.CodeAnalysis/Inspections/Concrete/UnreachableCaseInspection/UnreachableCaseInspection.cs

@@ -117,100 +117,95 @@ namespace Rubberduck.Inspections.Concrete.UnreachableCaseInspection
     public sealed class UnreachableCaseInspection : InspectionBase, IParseTreeInspection
     {
         private readonly IUnreachableCaseInspectorFactory _unreachableCaseInspectorFactory;
-        private readonly IParseTreeValueFactory _valueFactory;
+        private readonly IParseTreeValueVisitorFactory _parseTreeValueVisitorFactory;
+        private readonly UnreachableCaseInspectionListener _listener;
 
-        private enum CaseInspectionResult { Unreachable, InherentlyUnreachable, MismatchType, Overflow, CaseElse };
-
-        private static readonly Dictionary<CaseInspectionResult, string> ResultMessages = new Dictionary<CaseInspectionResult, string>()
+        public enum CaseInspectionResultType
         {
-            [CaseInspectionResult.Unreachable] = InspectionResults.UnreachableCaseInspection_Unreachable,
-            [CaseInspectionResult.InherentlyUnreachable] = InspectionResults.UnreachableCaseInspection_InherentlyUnreachable,
-            [CaseInspectionResult.MismatchType] = InspectionResults.UnreachableCaseInspection_TypeMismatch,
-            [CaseInspectionResult.Overflow] = InspectionResults.UnreachableCaseInspection_Overflow,
-            [CaseInspectionResult.CaseElse] = InspectionResults.UnreachableCaseInspection_CaseElse
-        };
+            Unreachable,
+            InherentlyUnreachable,
+            MismatchType,
+            Overflow,
+            CaseElse
+        }
 
-        public UnreachableCaseInspection(IDeclarationFinderProvider declarationFinderProvider) 
+        public UnreachableCaseInspection(IDeclarationFinderProvider declarationFinderProvider, IUnreachableCaseInspectionFactoryProvider factoryProvider) 
             : base(declarationFinderProvider)
         {
-            var factoryProvider = new UnreachableCaseInspectionFactoryProvider();
-
             _unreachableCaseInspectorFactory = factoryProvider.CreateIUnreachableInspectorFactory();
-            _valueFactory = factoryProvider.CreateIParseTreeValueFactory();
+            _parseTreeValueVisitorFactory = factoryProvider.CreateParseTreeValueVisitorFactory();
+            _listener = new UnreachableCaseInspectionListener();
         }
 
         public CodeKind TargetKindOfCode => CodeKind.CodePaneCode;
 
-        public IInspectionListener Listener { get; } =
-            new UnreachableCaseInspectionListener();
-
-        private ParseTreeVisitorResults ValueResults { get; } = new ParseTreeVisitorResults();
+        public IInspectionListener Listener => _listener;
 
         protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
         {
             var finder = DeclarationFinderProvider.DeclarationFinder;
+            var enumStmts = _listener.EnumerationStmtContexts();
+            var parseTreeValueVisitor = CreateParseTreeValueVisitor(enumStmts, GetIdentifierReferenceForContext);
 
             return finder.UserDeclarations(DeclarationType.Module)
                 .Where(module => module != null)
-                .SelectMany(module => DoGetInspectionResults(module.QualifiedModuleName, finder))
+                .SelectMany(module => DoGetInspectionResults(module.QualifiedModuleName, finder, parseTreeValueVisitor))
                 .ToList();
         }
 
         protected override IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module)
         {
             var finder = DeclarationFinderProvider.DeclarationFinder;
-            return DoGetInspectionResults(module, finder);
+            var enumStmts = _listener.EnumerationStmtContexts();
+            var parseTreeValueVisitor = CreateParseTreeValueVisitor(enumStmts, GetIdentifierReferenceForContext);
+            return DoGetInspectionResults(module, finder, parseTreeValueVisitor);
         }
 
-        private IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module, DeclarationFinder finder)
+        private IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module, DeclarationFinder finder, IParseTreeValueVisitor parseTreeValueVisitor)
         {
             var qualifiedSelectCaseStmts = Listener.Contexts(module)
                 // ignore filtering here to make the search space smaller
                 .Where(result => !result.IsIgnoringInspectionResultFor(finder, AnnotationName));
 
-            ParseTreeValueVisitor.OnValueResultCreated += ValueResults.OnNewValueResult;
-
             return qualifiedSelectCaseStmts
-                .SelectMany(ResultsForContext)
+                .SelectMany(context => ResultsForContext(context, finder, parseTreeValueVisitor))
                 .ToList();
         }
 
-        private IEnumerable<IInspectionResult> ResultsForContext(QualifiedContext<ParserRuleContext> qualifiedSelectCaseStmt)
+        private IEnumerable<IInspectionResult> ResultsForContext(QualifiedContext<ParserRuleContext> qualifiedSelectCaseStmt, DeclarationFinder finder, IParseTreeValueVisitor parseTreeValueVisitor)
         {
-            qualifiedSelectCaseStmt.Context.Accept(ParseTreeValueVisitor);
-            var selectCaseInspector = _unreachableCaseInspectorFactory.Create((VBAParser.SelectCaseStmtContext)qualifiedSelectCaseStmt.Context, ValueResults, _valueFactory, GetVariableTypeName);
+            var contextValues = qualifiedSelectCaseStmt.Context.Accept(parseTreeValueVisitor);
+            var selectCaseInspector = _unreachableCaseInspectorFactory.Create((VBAParser.SelectCaseStmtContext)qualifiedSelectCaseStmt.Context, contextValues, GetVariableTypeName);
 
-            selectCaseInspector.InspectForUnreachableCases();
+            var results = selectCaseInspector.InspectForUnreachableCases();
 
-            return selectCaseInspector
-                .UnreachableCases
-                .Select(uc => CreateInspectionResult(qualifiedSelectCaseStmt, uc, ResultMessages[CaseInspectionResult.Unreachable]))
-                .Concat(selectCaseInspector
-                    .MismatchTypeCases
-                    .Select(mm => CreateInspectionResult(qualifiedSelectCaseStmt, mm, ResultMessages[CaseInspectionResult.MismatchType])))
-                .Concat(selectCaseInspector
-                    .OverflowCases
-                    .Select(mm => CreateInspectionResult(qualifiedSelectCaseStmt, mm, ResultMessages[CaseInspectionResult.Overflow])))
-                .Concat(selectCaseInspector
-                    .InherentlyUnreachableCases
-                    .Select(mm => CreateInspectionResult(qualifiedSelectCaseStmt, mm, ResultMessages[CaseInspectionResult.InherentlyUnreachable])))
-                .Concat(selectCaseInspector
-                    .UnreachableCaseElseCases
-                    .Select(ce => CreateInspectionResult(qualifiedSelectCaseStmt, ce, ResultMessages[CaseInspectionResult.CaseElse])))
+            return results
+                .Select(resultTpl => CreateInspectionResult(qualifiedSelectCaseStmt, resultTpl.context, resultTpl.resultType))
                 .ToList();
         }
 
-        private IParseTreeValueVisitor _parseTreeValueVisitor;
-        public IParseTreeValueVisitor ParseTreeValueVisitor
+        private IInspectionResult CreateInspectionResult(QualifiedContext<ParserRuleContext> selectStmt, ParserRuleContext unreachableBlock, CaseInspectionResultType resultType)
         {
-            get
+            return CreateInspectionResult(selectStmt, unreachableBlock, ResultMessage(resultType));
+        }
+
+        //This cannot be a dictionary because the strings have to change after a change in the selected language.
+        private static string ResultMessage(CaseInspectionResultType resultType)
+        {
+            switch (resultType)
             {
-                if (_parseTreeValueVisitor is null)
-                {
-                    var listener = (UnreachableCaseInspectionListener)Listener;
-                    _parseTreeValueVisitor = CreateParseTreeValueVisitor(_valueFactory, listener.EnumerationStmtContexts(), GetIdentifierReferenceForContext);
-                }
-                return _parseTreeValueVisitor;
+                case CaseInspectionResultType.Unreachable:
+                    return InspectionResults.UnreachableCaseInspection_Unreachable;
+                case CaseInspectionResultType.InherentlyUnreachable:
+                    return InspectionResults.UnreachableCaseInspection_InherentlyUnreachable;
+                case CaseInspectionResultType.MismatchType:
+                    return InspectionResults.UnreachableCaseInspection_TypeMismatch;
+                case CaseInspectionResultType.Overflow:
+                    return InspectionResults.UnreachableCaseInspection_Overflow;
+                case CaseInspectionResultType.CaseElse:
+                    return InspectionResults.UnreachableCaseInspection_CaseElse;
+                default:
+                    throw new ArgumentOutOfRangeException(nameof(resultType), resultType, null);
             }
         }
 
@@ -221,8 +216,8 @@ private IInspectionResult CreateInspectionResult(QualifiedContext<ParserRuleCont
                         new QualifiedContext<ParserRuleContext>(selectStmt.ModuleName, unreachableBlock));
         }
 
-        public static IParseTreeValueVisitor CreateParseTreeValueVisitor(IParseTreeValueFactory valueFactory, IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, Func<ParserRuleContext, (bool success, IdentifierReference idRef)> func)
-            => new ParseTreeValueVisitor(valueFactory, allEnums, func);
+        public IParseTreeValueVisitor CreateParseTreeValueVisitor(IReadOnlyList<VBAParser.EnumerationStmtContext> allEnums, Func<ParserRuleContext, (bool success, IdentifierReference idRef)> func)
+            => _parseTreeValueVisitorFactory.Create(allEnums, func);
 
         //Method is used as a delegate to avoid propagating RubberduckParserState beyond this class
         private (bool success, IdentifierReference idRef) GetIdentifierReferenceForContext(ParserRuleContext context)