OnErrorResumeNext Inspection can fail while walking ParseTrees

[Vogel612]

Stacktrace obtained from #3986:

2018-05-07 20:32:52.8434;WARN-2.2.6698.27995;Rubberduck.Inspections.Rubberduck.Inspections.Inspector+<FindIssuesAsync>d__7;System.ArgumentException: An item with the same key has already been added.
   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at Rubberduck.Inspections.Concrete.OnErrorStatementListener.ExitModuleBodyElement(ModuleBodyElementContext context) in C:\projects\rubberduck\Rubberduck.Inspections\Concrete\UnhandledOnErrorResumeNextInspection.cs:line 79
   at Rubberduck.Parsing.Grammar.VBAParser.ModuleBodyElementContext.ExitRule(IParseTreeListener listener) in C:\projects\rubberduck\Rubberduck.Parsing\obj\Release\VBAParser.cs:line 1988
   at Rubberduck.Parsing.VBA.CombinedParseTreeListener.ExitEveryRule(ParserRuleContext ctx) in C:\projects\rubberduck\Rubberduck.Parsing\VBA\CombinedParseTreeListener.cs:line 33
   at Antlr4.Runtime.Tree.ParseTreeWalker.ExitRule(IParseTreeListener listener, IRuleNode r)
   at Antlr4.Runtime.Tree.ParseTreeWalker.Walk(IParseTreeListener listener, IParseTree t)
   at Rubberduck.Inspections.Rubberduck.Inspections.Inspector.WalkTrees(CodeInspectionSettings settings, RubberduckParserState state, IEnumerable`1 inspections, ParsePass pass) in C:\projects\rubberduck\Rubberduck.Inspections\Inspector.cs:line 239
   at Rubberduck.Inspections.Rubberduck.Inspections.Inspector.<FindIssuesAsync>d__7.MoveNext() in C:\projects\rubberduck\Rubberduck.Inspections\Inspector.cs:line 95;System.ArgumentException: An item with the same key has already been added.
   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at Rubberduck.Inspections.Concrete.OnErrorStatementListener.ExitModuleBodyElement(ModuleBodyElementContext context) in C:\projects\rubberduck\Rubberduck.Inspections\Concrete\UnhandledOnErrorResumeNextInspection.cs:line 79
   at Rubberduck.Parsing.Grammar.VBAParser.ModuleBodyElementContext.ExitRule(IParseTreeListener listener) in C:\projects\rubberduck\Rubberduck.Parsing\obj\Release\VBAParser.cs:line 1988
   at Rubberduck.Parsing.VBA.CombinedParseTreeListener.ExitEveryRule(ParserRuleContext ctx) in C:\projects\rubberduck\Rubberduck.Parsing\VBA\CombinedParseTreeListener.cs:line 33
   at Antlr4.Runtime.Tree.ParseTreeWalker.ExitRule(IParseTreeListener listener, IRuleNode r)
   at Antlr4.Runtime.Tree.ParseTreeWalker.Walk(IParseTreeListener listener, IParseTree t)
   at Rubberduck.Inspections.Rubberduck.Inspections.Inspector.WalkTrees(CodeInspectionSettings settings, RubberduckParserState state, IEnumerable`1 inspections, ParsePass pass) in C:\projects\rubberduck\Rubberduck.Inspections\Inspector.cs:line 239
   at Rubberduck.Inspections.Rubberduck.Inspections.Inspector.<FindIssuesAsync>d__7.MoveNext() in C:\projects\rubberduck\Rubberduck.Inspections\Inspector.cs:line 95

Seems like we do something wrong in ExitModuleBodyElement in the OERNListener

[MDoerner]

The problem here is that the listener populates two dictionaries with additional information to be passed to the inspection results. These do not get cleared in ClearContexts. Consequently, a rerun of the inspection will fail on adding a context the second time.

To be honest, I have not been able to determine the actual use of the two dictionaries. The first one actually does not save any information one could not get from the context via an existing extension method.

[rkapka]

The dictionaries are assigned to the Properties dynamic object and accessed in the inspection's quick fix.

[MDoerner]

What is a bit of a concern for me is that the inspection does work for one quickfix while finding the results. The second dictionary actually dictates what labels the quickfix uses. In an ideal world the inspections would determine the offending contexts and the quickfixes would determine how to fix the results.

Regarding my comment about the first context, in the quickfix result.Properties.BodyElement should be equivalent to result.Context.GetAncestor<VBAParser.ModuleBodyElementContext>();

[rkapka]

I agree completely on the second point, this needs to be simplified using the extension method.

I also understand that the separation of concerns is violated here, but I can't think of a good alternative at the moment. I suppose I could find all existing labels inside the issue's body element and then deduce the new label name, but what if there are multiple issues in one procedure? If I understand the code in QuickFixProvider correctly, all fixes are applied before the module is rewritten, so two fixes in one procedure will end up with the same label name. This is just an assumption, though, which I'll have to verify.

[MDoerner]

You are right that the quickfixes might fix all instances at the same time, in case of fix all in module or all in projects. That is a problem for this quickfix indeed.

One alternative thing to provide together instead of the labels came to my mind. How about passing along all result contexts for the same module body element in a list attached to the property bag? Then the relative position of the statement could be inferred from that. In addition, the declaration finder on the state could be used to determine the maximally indexed already existing label with the intended name, which could be used to determine the offset for the error handler label indexes. However, that is just a thought.

[retailcoder]

Alternatively, the quickfix could be made unavailable at project and module levels, "forcing" the user to review each "issue" individually at a procedure scope. If making the quickfix operate at procedure level makes our lives easier and the quickfix more reliable, I'm all for it.