Refactor parse tree inspections #5404

MDoerner · 2020-02-27T22:42:59Z

This PR introduces a base class for the IInspectionListener implementations and extends the common code in the ParseTreeInspectionBase.

In addition, the InspectionBase gets modified in two ways. First, a new method is introduced to run inspections separately per module. Second, the State property gets removed, allowing the inspections to depend only on IDeclarationFinderProvider instead.

Furthermore, the three inspections ImplicitByRefModifierInspection, RedundantByRefModifierInspection and ProcedureCanBeWrittenAsFunctionInspection get turned into declaration inspections, since this very much simplifies them.

Since a new member RequiredArguments is added to IAnnotation, the MissingAnnotationArgumentInspection can be made more extensible and simple. It also only looks at annotations provided by the declaration finder instead of parse trees now.

There are also a few more small fixes and refactorings.

This PR is still WIP, since I have to clean up some FIXMEs in UnreacahbleCaseInspection.

Still, I wanted to get this PR out to get feedback and to pose the question whether we should always provide a declaration finder as argument to the abstract function DoGetInspectionResults in InspectionBase since most inspections need one and get it immediately in that method—with the exception of the parse tree inspections—and since, moreover, the enclosing method GetInspectionResults uses one anyway to determine whether results are ignored.

Also adds a base for inspection listeners.

The DeclarationFinderProvider remains protected for now.

Require an IDeclarationFinderProvider instead of a RubberduckParserState.

Also adds the missing configurationd for the references libraries in tests.

Also removes the use of a CodeModule.

…pection

Also refactors the class a bit and fixes several possible multiple enumeration issues.

It specifies the minimal number of arguments for the annotation.

Also adds an overload of DeclarationFinder.FindAnnotations providing all annotations in a module. IllegalAnnotationInspection now uses this in favor of RubberduckParserState.GetAnnotations, which allows to remove the dependency on the state.

codecov · 2020-02-27T22:43:03Z

Codecov Report

Merging #5404 into next will increase coverage by 0.6%.
The diff coverage is 84.91%.

@@            Coverage Diff            @@
##             next    #5404     +/-   ##
=========================================
+ Coverage   60.91%   61.51%   +0.6%     
=========================================
  Files        1175     1195     +20     
  Lines       40332    40570    +238     
  Branches     8536     8644    +108     
=========================================
+ Hits        24568    24955    +387     
+ Misses      13824    13788     -36     
+ Partials     1940     1827    -113

Impacted Files	Coverage Δ
...rduck.Core/UI/Settings/GeneralSettingsViewModel.cs	`57.47% <ø> (ø)`	⬆️
...Concrete/UnreachableCaseInspection/FilterLimits.cs	`67.39% <ø> (ø)`	⬆️
...sis/QuickFixes/Concrete/RemoveAttributeQuickFix.cs	`58.82% <ø> (ø)`
...eAnalysis/Settings/CodeInspectionConfigProvider.cs	`94.12% <ø> (ø)`	⬆️
...ncrete/UnreachableCaseInspection/ParseTreeValue.cs	`85.45% <ø> (+1.82%)`	⬆️
...Parsing/Preprocessing/CompilationArgumentsCache.cs	`100% <ø> (ø)`	⬆️
...ctions/Results/QualifiedContextInspectionResult.cs	`100% <ø> (ø)`	⬆️
...QuickFixes/Concrete/ExpandDefaultMemberQuickFix.cs	`78.26% <ø> (ø)`
...ickFixes/Concrete/AdjustAttributeValuesQuickFix.cs	`50% <ø> (ø)`
...rters/CodeInspectionSeverityEnumToTextConverter.cs	`0% <ø> (ø)`	⬆️
... and 403 more

Also add allowed number of arguments to IAnnotation interface.

Also removes the special testing interface from ParseTreeValueVisitor.

The inspector swallows exceptions, which we really do not want in tests.

It now correctly depends on the module and passes along the found results instead of aggregating them in an instance variable.

This allows to remove the corresponding state from the ParseTreeValueVisitor.

…stener in UnreachableCaseInspection

bclothier

Great work as always; I skimmed over minor changes but have the following questions.

bclothier · 2020-03-03T23:32:50Z

Rubberduck.CodeAnalysis/Inspections/Abstract/ParseTreeInspectionBase.cs

        public virtual CodeKind TargetKindOfCode => CodeKind.CodePaneCode;
    }
+
+    public class InspectionListenerBase : VBAParserBaseListener, IInspectionListener


I think we might want to consider sticking to the rule of one type per one file. Maybe even consider having all bases in a folder for easy grouping.

Except for generic abstract classes inherited from a non-generic abstract class? 😉

For parse tree inspections, we have always put the listeners into the same file.

I think, in many cases, we can actually make them private classes.

I do agree with inlining the listeners in the implementation since most of the time they will be specific to only one inspection so it makes sense to make it a nested & private class. We do have to consider the discoverability aspect for the abstract classes, however. If I'm new to the code, I cannot see that there are both ParseTreeInspectionBase and InspectionListenerBase by viewing the filesystem alone. I'd have to use something like Find Implementation on the IInspectionListener to see that I need to create my listener based on the InspectionListenerBase. I am not sure off the cuff if the Find Implementation won't give cluttered results with all other concrete implementations.

bclothier · 2020-03-03T23:48:42Z

Rubberduck.CodeAnalysis/Inspections/Concrete/BooleanAssignedInIfElseInspection.cs

-                                                       string.Format(InspectionResults.BooleanAssignedInIfElseInspection,
-                                                            (((VBAParser.IfStmtContext)result.Context).block().GetDescendent<VBAParser.LetStmtContext>()).lExpression().GetText().Trim()),
-                                                       result));
+            var literalText = ((VBAParser.IfStmtContext) context.Context)


The way it is, it requires upcasting from general ParserContext to a specific context. Given that we had the specific context available to us when listening, are we better off constructing the strings and saving it in the contexts dictionary (as a tuple, perhaps), thus simplifying the ResultDescription and cut out potential runtime errors resulting from incorrect upcasting in all and future implementations of the parse tree inspections?

That's not a crazy idea at all. Actually there's something odd about losing all benefits of generics here... it's almost like.. like it wants to be string ResultDescription<TContext>(QualifiedContext<TContext> context) where TContext : ParserRuleContext? Or that's.. probably a rabbit hole in generics wonderland...

I already have an idea how to wire up a generic version. However, this will need rework on all parse tree inspections and on the interface and its users as well.

Since the latest push, the parse tree inspection base classes and the listeners are generic with respect to the context type.

I think the latest changes is a vast improvement, nicely done! The only nitpick I have is that a select number of inspections use the general ParserRuleContext as the TContext due to the fact that they work with more than one contexts. Ideally, it would be possible to statically enumerate valid contexts they can have but I don't see a solution for that short of adding marker interfaces to various parser rule contexts. For this PR, I'm happy with the direction you have taken.

bclothier · 2020-03-03T23:50:33Z

Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyCaseBlockInspection.cs

@@ -45,18 +43,16 @@ namespace Rubberduck.Inspections.Concrete
    [Experimental(nameof(ExperimentalNames.EmptyBlockInspections))]


Unrelated but is this still necessary? The same question applies to other empty whatever inspections.

I can remove the experimental attributes, if you want.

I think we should change the severity to Hint, though.

@MDoerner agreed, Hint works. Were the attributes removed?

Should I remove them?

Yeah, now's a good time :)

retailcoder · 2020-03-04T01:33:28Z

Rubberduck.CodeAnalysis/Inspections/Abstract/DeclarationInspectionBaseBase.cs

@@ -13,15 +13,15 @@ public abstract class DeclarationInspectionBaseBase : InspectionBase
        protected readonly DeclarationType[] RelevantDeclarationTypes;
        protected readonly DeclarationType[] ExcludeDeclarationTypes;

-        protected DeclarationInspectionBaseBase(RubberduckParserState state, params DeclarationType[] relevantDeclarationTypes)
-            : base(state)
+        protected DeclarationInspectionBaseBase(IDeclarationFinderProvider declarationFinderProvider, params DeclarationType[] relevantDeclarationTypes)


This is why I'm less certain than before about the one class: one file rule/guideline, @bclothier: I'm thinking the only reason this class needs a silly name here is because of one class: one file, because DeclarationInspectionBase<T> wants to go into a DeclarationInspectionBase.cs file.
I'm pretty sure we could have DeclarationInspectionBase and DeclarationInspectionBase<T> in the same DeclarationInspectionBase.cs file, and avoid needing to name our abstract classes ThingBaseBase 😜

@MDoerner what do you think?

I'll admit that I am not consistent on that guideline myself. But FWIW, in this particular case, I had files RefactoringDialogBase and RefactoringDialogBaseGeneric. Note that in the generic file, the class is just RefactoringDialogBase<>; no need for BaseBase.

Ah, but then you sacrifice the consistency of classname.cs! #NamingIsHard

Also, in this particular example, this isn't generic. It's basically a InspectionBase implementing a particular implementation for declaration inspections. In which case, I'm not sure it should be DeclarationInspectionBaseBase; simply just DeclarationInspectionBase is sufficient. The fact that it's based on another abstract class (InspectionBase) is not really material and shouldn't factor into the naming, IMO.

Yes, agreed on #NamingIsHard.

I have to admit that I don't really like the name myself. It is called that way because it is the base class for the other declaration inspection base classes. Apart from the ones you already mentioned there are also the two multi result base classes.
That it is based on InspectionBase is irrelevant for the name.

Btw, the generic and non-generic versions of the base classes are already in the same file for each kind.

Should I do something about this class or not? Options are

Rename DeclarationInspectionBase to DeclaratiinInspectionSingleResultBase and DeclarationInspectionBaseBase to DeclarationInspectionBase. That removes the BaseBase but opens room for confusion since this class should exclusively be implemented by base classes and not concrete inspection.

Inline the class into all three implementation inspection base classes.

Leave it as-is.

Your comment makes me realize that I had the BaseBase backward; I interpreted it as a Base class based on another Base class but you're meaning a Base class that needs to be implemented by other Base classes. I had to look at the DeclarationInspectionBase to see that it inherits DeclarationInspectionBaseBase. Obviously that ambiguity is not what we want.

The only suggestion I can think of is PartialBase, in similar fashion to the partial class to suggest that it's not a complete definition, but I am not sure that is any improvement. At very least, there should be a xml docs on the BaseBase explaining that it is meant to be implemented by other base classes and not directly.

Rubberduck.CodeAnalysis/Inspections/Concrete/BooleanAssignedInIfElseInspection.cs

retailcoder · 2020-03-04T02:07:59Z

Still, I wanted to get this PR out to get feedback and to pose the question whether we should always provide a declaration finder as argument to the abstract function DoGetInspectionResults in InspectionBase since most inspections need one and get it immediately in that method—with the exception of the parse tree inspections—and since, moreover, the enclosing method GetInspectionResults uses one anyway to determine whether results are ignored.

Providing the finder as an argument of the templated method could eliminate the need to expose it in some protected manner, assuming it's only accessed in DoGetInspectionResults in derived types. That makes the encapsulation more complete, and the resulting API is much friendlier IMO; if I'm new to the code base and look at how inspections work, everything is easier to follow when you get parameters and use them to implement a procedure - it's almost like handling a Change event in a Worksheet module!

retailcoder

The amount of tech debt being paid here is phenomenal - is the draft/WIP status still valid? Let's merge this!

MDoerner · 2020-03-04T08:01:56Z

The draft status is still valid. Based on comments, I will make both the listener base and the parse tree inspection base generic and wire them up correctly. Moreover, I am not done with UnreachableCaseInspection yet.

…ead of the provider

…ntations as method parameter Before, the provider was protected and that was used, getting a new one in all base classes for the different kinds of inspections.

Also turns the ParseTreeValueVisitor in UnreachableCaseInspection into a constructor injected field.

Also turns the inspector in UnreachableCaseInspection into a constructor injected field.

MDoerner · 2020-03-06T23:55:54Z

I do not really like that ParseTreeValueVisitor still takes an optional constructor parameter solely for testing purposes, but I think I am done with what I wanted to do in this PR.

bclothier · 2020-03-07T00:23:08Z

Rubberduck.CodeAnalysis/Inspections/Concrete/EmptyBlockInspectionListenerBase.cs

@@ -7,7 +7,7 @@

 namespace Rubberduck.Inspections.Concrete
 {
-    public class EmptyBlockInspectionListenerBase<TContext> : InspectionListenerBase<TContext>
+    internal class EmptyBlockInspectionListenerBase<TContext> : InspectionListenerBase<TContext>


Would it be cleaner to have an abstract EmptyBlockInspectionBase<TContext> with a protected EmptyBlockInspectionListenerBase<TContext> ContextListener { get; }? That way the listener becomes private to the EmptyBlockInspectionBase.

The Base suffix says it already, but having this an abstract class would make it compiler-enforced that it's not intended to be instantiated directly, too.

…listener base

Also fixes wrong InternalsVisibleTo attributes.

Also sets the severity to Hint.

Also adds analyzers for the type attribute of the module element.

Also makes this mandatory from now on.

MDoerner · 2020-03-08T10:47:45Z

PSA: once this PR gets merged, everybody might have to rebuild the RubberduckMeta solution because this PR fixes the inspection XML-doc analyzer. Otherwise, you might get false positives for missing name tags.
(Previously, the analyzer did not work at all, because the inspections were not in the expected namespace.)

bclothier

I know the PR was merged already but few minor nitpicks maybe to address in future PRs.

bclothier · 2020-03-08T05:17:17Z

RubberduckCodeAnalysis/Resources.resx

+    <value>Duplicate value in 'name' attribute</value>
+  </data>
+  <data name="DuplicateNameAttributeDescription" xml:space="preserve">
+    <value>The 'name' attribute is used to uniquely identify an element. Consequently, it has to be unique.</value>


Suggested change

<value>The 'name' attribute is used to uniquely identify an element. Consequently, it has to be unique.</value>

<value>The 'name' attribute must uniquely identify an element.</value>

bclothier · 2020-03-08T05:25:03Z

RubberduckTestsCodeAnalysis/InspectionXmlDocAnalyzerTests.cs

+    /// End Sub
+    /// ]]>
+    /// </example>
+    [RequiredHost(""NotExcel"")]


I have to point out that the way it is, there's coupling between the xml-docs and the attribute. It would be better for the markup generator to actually read the attributes instead of expecting a hostApp (or any elements), which means we only have one source of truth and no need to keep thing synchronized.

I believe MSBuild is the markup generator here... it reads XML-docs though, not attributes. That coupling is hopelessly necessary, unfortunately.

bclothier · 2020-03-08T05:27:04Z

RubberduckCodeAnalysis/Resources.resx

+    <value>Invalid value for 'type' attribute.</value>
+  </data>
+  <data name="InvalidTypeAttributeDescription" xml:space="preserve">
+    <value>The values of the 'type' attribute are restricted to the value 'Standard Module', 'Class Module', 'Document' and 'User Form'.</value>


Nitpick: Isn't UserForm more typical?

MDoerner added 15 commits February 26, 2020 13:06

Redesign ParseTreeInspection base class

137e3dc

Also adds a base for inspection listeners.

Remove declaration members and state from the InspectionBase

7d620a8

The DeclarationFinderProvider remains protected for now.

Make ImplicitByRef- and RedundantByRefInspection declaration inspections

f1b6bb0

Move DeclarationExtensions to JunkDrawer

46d3707

Make IllegalAnnotationInspection work for individual modules

0c7da5a

Reduce constructor requirements on inspections

2fc4217

Require an IDeclarationFinderProvider instead of a RubberduckParserState.

Treat IUnknown like Object in incompatible Set type inspections

117f00f

Also adds the missing configurationd for the references libraries in tests.

Make ObsoleteCallStatementInspection more precise

15d6320

Also removes the use of a CodeModule.

Turn ProcedureCanBeWrittenAsFunctionInspection into a declaration ins…

c4e0fd8

…pection

Make UnreachableCaseInspection able to run by module

a7ccb69

Make it possible to run inspections for single modules

b24cfcb

Fix an NRE in UnreachableCaseInspector

03bf899

Also refactors the class a bit and fixes several possible multiple enumeration issues.

Add RequiredArguments to IAnnotation

9fed0b3

It specifies the minimal number of arguments for the annotation.

Unquote the replacement documentation on ObsoleteAnnotation

d21dacf

MDoerner added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Feb 27, 2020

MDoerner added 9 commits February 28, 2020 01:38

Add SuperfluousAnnotationArgumentsInspection

1c8f7a4

Also add allowed number of arguments to IAnnotation interface.

Stop using stale state in UnreachableCaseInspection

e280deb

Also removes the special testing interface from ParseTreeValueVisitor.

Allow to clear inspection listener contexts by module

ca77a8d

Stop using inspector for parse tree inspections

04aa262

The inspector swallows exceptions, which we really do not want in tests.

Pass declaration finder along in UnreacableCaseInspection

e5efb48

Refactor ParseTreeValueVisitor

deea59f

It now correctly depends on the module and passes along the found results instead of aggregating them in an instance variable.

Make the enum members part of the ParseTreeVisitorResults

5eda095

This allows to remove the corresponding state from the ParseTreeValueVisitor.

Get enumeration statements from enumeration members instead of the li…

5689b95

…stener in UnreachableCaseInspection

Remove most state from UnreachableCaseInspector

fbdcf00

bclothier reviewed Mar 4, 2020

View reviewed changes

retailcoder reviewed Mar 4, 2020

View reviewed changes

Rubberduck.CodeAnalysis/Inspections/Concrete/BooleanAssignedInIfElseInspection.cs Show resolved Hide resolved

retailcoder approved these changes Mar 4, 2020

View reviewed changes

MDoerner added 6 commits March 5, 2020 20:27

Make inspection listener base and parse tree inspection base generic

13e1547

Let IdentifierReferenceInspectionResult take a DeclarationFinder inst…

42a602a

…ead of the provider

Let hand down the declaration finder in InspectionBase to the impleme…

ab922fd

…ntations as method parameter Before, the provider was protected and that was used, getting a new one in all base classes for the different kinds of inspections.

Make ParseTreeValueVisitor.VisitChildren pure

b26b8f4

Also turns the ParseTreeValueVisitor in UnreachableCaseInspection into a constructor injected field.

Make UnreachableCaseInspector.InspectForUnreachableCases pure

6a17bf7

Also turns the inspector in UnreachableCaseInspection into a constructor injected field.

Make all concrete inspection listeners private

af2b026

MDoerner removed the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Mar 6, 2020

MDoerner marked this pull request as ready for review March 6, 2020 23:54

bclothier reviewed Mar 7, 2020

View reviewed changes

bclothier approved these changes Mar 7, 2020

View reviewed changes

MDoerner added 6 commits March 7, 2020 15:59

Make empty block inspections derive from the same base providing the …

0a517ae

…listener base

Make inspection and quick fix implementation internal

32e6201

Also fixes wrong InternalsVisibleTo attributes.

Move inspection and quick fix interfaces into CodeAnalysis

932ac85

Move remainder of inspections API to CodeAnalysis

3cdd860

Adjust inspection and quick fix namespaces

e7c76e3

Remove experimental attribute from empty block inspections

b527bb3

Also sets the severity to Hint.

MDoerner force-pushed the RefactorParseTreeInspections branch from fb9b8bb to b527bb3 Compare March 7, 2020 19:49

MDoerner added 5 commits March 7, 2020 22:16

Fix wrong hasResult attributes in inspection xml-docs

280bcc6

Fix inspection xml-doc analyzers

3b49f91

Also adds analyzers for the type attribute of the module element.

Add inspection xml-doc analyzer for duplicate names

0745617

Add missing module elements to inspection xml-doc examples

5d3b03b

Also makes this mandatory from now on.

Add analyzer for hostApp in inspections xml-doc

98ea837

retailcoder merged commit 5d7189e into rubberduck-vba:next Mar 8, 2020

MDoerner mentioned this pull request Mar 8, 2020

Inspection false positive "Argument with incompatible object type" for IUnknown #5394

Closed

bclothier reviewed Mar 8, 2020

View reviewed changes

		@@ -45,18 +43,16 @@ namespace Rubberduck.Inspections.Concrete
		[Experimental(nameof(ExperimentalNames.EmptyBlockInspections))]

	<value>The 'name' attribute is used to uniquely identify an element. Consequently, it has to be unique.</value>
	<value>The 'name' attribute must uniquely identify an element.</value>

Refactor parse tree inspections #5404

Refactor parse tree inspections #5404

Conversation

MDoerner commented Feb 27, 2020

codecov bot commented Feb 27, 2020 • edited

Codecov Report

bclothier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retailcoder Mar 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retailcoder commented Mar 4, 2020

retailcoder left a comment

Choose a reason for hiding this comment

MDoerner commented Mar 4, 2020

MDoerner commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDoerner commented Mar 8, 2020

bclothier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 27, 2020 •

edited

retailcoder Mar 4, 2020 •

edited