New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor parse tree inspections #5404
Merged
retailcoder
merged 41 commits into
rubberduck-vba:next
from
MDoerner:RefactorParseTreeInspections
Mar 8, 2020
Merged
Changes from all commits
Commits
Show all changes
41 commits
Select commit
Hold shift + click to select a range
137e3dc
Redesign ParseTreeInspection base class
MDoerner 7d620a8
Remove declaration members and state from the InspectionBase
MDoerner f1b6bb0
Make ImplicitByRef- and RedundantByRefInspection declaration inspections
MDoerner 46d3707
Move DeclarationExtensions to JunkDrawer
MDoerner 0c7da5a
Make IllegalAnnotationInspection work for individual modules
MDoerner 2fc4217
Reduce constructor requirements on inspections
MDoerner 117f00f
Treat IUnknown like Object in incompatible Set type inspections
MDoerner 15d6320
Make ObsoleteCallStatementInspection more precise
MDoerner c4e0fd8
Turn ProcedureCanBeWrittenAsFunctionInspection into a declaration ins…
MDoerner a7ccb69
Make UnreachableCaseInspection able to run by module
MDoerner b24cfcb
Make it possible to run inspections for single modules
MDoerner 03bf899
Fix an NRE in UnreachableCaseInspector
MDoerner 9fed0b3
Add RequiredArguments to IAnnotation
MDoerner 8c2fae8
Make MissingAnnotationArguments use only the annotations
MDoerner d21dacf
Unquote the replacement documentation on ObsoleteAnnotation
MDoerner 1c8f7a4
Add SuperfluousAnnotationArgumentsInspection
MDoerner e280deb
Stop using stale state in UnreachableCaseInspection
MDoerner ca77a8d
Allow to clear inspection listener contexts by module
MDoerner 04aa262
Stop using inspector for parse tree inspections
MDoerner e5efb48
Pass declaration finder along in UnreacableCaseInspection
MDoerner deea59f
Refactor ParseTreeValueVisitor
MDoerner 5eda095
Make the enum members part of the ParseTreeVisitorResults
MDoerner 5689b95
Get enumeration statements from enumeration members instead of the li…
MDoerner fbdcf00
Remove most state from UnreachableCaseInspector
MDoerner 13e1547
Make inspection listener base and parse tree inspection base generic
MDoerner 42a602a
Let IdentifierReferenceInspectionResult take a DeclarationFinder inst…
MDoerner ab922fd
Let hand down the declaration finder in InspectionBase to the impleme…
MDoerner b26b8f4
Make ParseTreeValueVisitor.VisitChildren pure
MDoerner 6a17bf7
Make UnreachableCaseInspector.InspectForUnreachableCases pure
MDoerner af2b026
Make all concrete inspection listeners private
MDoerner 0a517ae
Make empty block inspections derive from the same base providing the …
MDoerner 32e6201
Make inspection and quick fix implementation internal
MDoerner 932ac85
Move inspection and quick fix interfaces into CodeAnalysis
MDoerner 3cdd860
Move remainder of inspections API to CodeAnalysis
MDoerner e7c76e3
Adjust inspection and quick fix namespaces
MDoerner b527bb3
Remove experimental attribute from empty block inspections
MDoerner 280bcc6
Fix wrong hasResult attributes in inspection xml-docs
MDoerner 3b49f91
Fix inspection xml-doc analyzers
MDoerner 0745617
Add inspection xml-doc analyzer for duplicate names
MDoerner 5d3b03b
Add missing module elements to inspection xml-doc examples
MDoerner 98ea837
Add analyzer for hostApp in inspections xml-doc
MDoerner File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
46 changes: 19 additions & 27 deletions
46
Rubberduck.CodeAnalysis/Inspections/Abstract/DeclarationInspectionBaseBase.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,60 +1,52 @@ | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Rubberduck.Parsing.Inspections.Abstract; | ||
| using Rubberduck.Parsing.Symbols; | ||
| using Rubberduck.Parsing.VBA; | ||
| using Rubberduck.Parsing.VBA.DeclarationCaching; | ||
| using Rubberduck.VBEditor; | ||
|
|
||
| namespace Rubberduck.Inspections.Abstract | ||
| namespace Rubberduck.CodeAnalysis.Inspections.Abstract | ||
| { | ||
| public abstract class DeclarationInspectionBaseBase : InspectionBase | ||
| /// <summary> | ||
| /// This is a base class for the other declaration inspection base classes. It should not be implemented directly by concrete inspections. | ||
| /// </summary> | ||
| internal abstract class DeclarationInspectionBaseBase : InspectionBase | ||
| { | ||
| protected readonly DeclarationType[] RelevantDeclarationTypes; | ||
| protected readonly DeclarationType[] ExcludeDeclarationTypes; | ||
| private readonly DeclarationType[] _relevantDeclarationTypes; | ||
| private readonly DeclarationType[] _excludeDeclarationTypes; | ||
|
|
||
| protected DeclarationInspectionBaseBase(RubberduckParserState state, params DeclarationType[] relevantDeclarationTypes) | ||
| : base(state) | ||
| protected DeclarationInspectionBaseBase(IDeclarationFinderProvider declarationFinderProvider, params DeclarationType[] relevantDeclarationTypes) | ||
| : base(declarationFinderProvider) | ||
| { | ||
| RelevantDeclarationTypes = relevantDeclarationTypes; | ||
| ExcludeDeclarationTypes = new DeclarationType[0]; | ||
| _relevantDeclarationTypes = relevantDeclarationTypes; | ||
| _excludeDeclarationTypes = new DeclarationType[0]; | ||
| } | ||
|
|
||
| protected DeclarationInspectionBaseBase(RubberduckParserState state, DeclarationType[] relevantDeclarationTypes, DeclarationType[] excludeDeclarationTypes) | ||
| : base(state) | ||
| protected DeclarationInspectionBaseBase(IDeclarationFinderProvider declarationFinderProvider, DeclarationType[] relevantDeclarationTypes, DeclarationType[] excludeDeclarationTypes) | ||
| : base(declarationFinderProvider) | ||
| { | ||
| RelevantDeclarationTypes = relevantDeclarationTypes; | ||
| ExcludeDeclarationTypes = excludeDeclarationTypes; | ||
| _relevantDeclarationTypes = relevantDeclarationTypes; | ||
| _excludeDeclarationTypes = excludeDeclarationTypes; | ||
| } | ||
|
|
||
| protected abstract IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module, DeclarationFinder finder); | ||
|
|
||
| protected override IEnumerable<IInspectionResult> DoGetInspectionResults() | ||
| protected override IEnumerable<IInspectionResult> DoGetInspectionResults(DeclarationFinder finder) | ||
| { | ||
| var finder = DeclarationFinderProvider.DeclarationFinder; | ||
|
|
||
| return finder.UserDeclarations(DeclarationType.Module) | ||
| .Concat(finder.UserDeclarations(DeclarationType.Project)) | ||
| .Where(declaration => declaration != null) | ||
| .SelectMany(declaration => DoGetInspectionResults(declaration.QualifiedModuleName, finder)) | ||
| .ToList(); | ||
| } | ||
|
|
||
| protected virtual IEnumerable<IInspectionResult> DoGetInspectionResults(QualifiedModuleName module) | ||
| { | ||
| var finder = DeclarationFinderProvider.DeclarationFinder; | ||
| return DoGetInspectionResults(module, finder); | ||
| } | ||
|
|
||
| protected virtual IEnumerable<Declaration> RelevantDeclarationsInModule(QualifiedModuleName module, DeclarationFinder finder) | ||
| { | ||
| var potentiallyRelevantDeclarations = RelevantDeclarationTypes.Length == 0 | ||
| var potentiallyRelevantDeclarations = _relevantDeclarationTypes.Length == 0 | ||
| ? finder.Members(module) | ||
| : RelevantDeclarationTypes | ||
| : _relevantDeclarationTypes | ||
| .SelectMany(declarationType => finder.Members(module, declarationType)) | ||
| .Distinct(); | ||
| return potentiallyRelevantDeclarations | ||
| .Where(declaration => !ExcludeDeclarationTypes.Contains(declaration.DeclarationType)); | ||
| .Where(declaration => !_excludeDeclarationTypes.Contains(declaration.DeclarationType)); | ||
| } | ||
| } | ||
| } | ||
15 changes: 7 additions & 8 deletions
15
Rubberduck.CodeAnalysis/Inspections/Abstract/DeclarationInspectionMultiResultBase.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aDeclarationInspectionBase.csfile.I'm pretty sure we could have
DeclarationInspectionBaseandDeclarationInspectionBase<T>in the sameDeclarationInspectionBase.csfile, and avoid needing to name our abstract classesThingBaseBase😜@MDoerner what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit that I am not consistent on that guideline myself. But FWIW, in this particular case, I had files
RefactoringDialogBaseandRefactoringDialogBaseGeneric. Note that in the generic file, the class is justRefactoringDialogBase<>; no need forBaseBase.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but then you sacrifice the consistency of
classname.cs! #NamingIsHardThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in this particular example, this isn't generic. It's basically a
InspectionBaseimplementing a particular implementation for declaration inspections. In which case, I'm not sure it should beDeclarationInspectionBaseBase; simply justDeclarationInspectionBaseis 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
InspectionBaseis irrelevant for the name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the generic and non-generic versions of the base classes are already in the same file for each kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do something about this class or not? Options are
DeclarationInspectionBasetoDeclaratiinInspectionSingleResultBaseandDeclarationInspectionBaseBasetoDeclarationInspectionBase. That removes theBaseBasebut opens room for confusion since this class should exclusively be implemented by base classes and not concrete inspection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment makes me realize that I had the
BaseBasebackward; I interpreted it as aBaseclass based on anotherBaseclass but you're meaning aBaseclass that needs to be implemented by otherBaseclasses. I had to look at theDeclarationInspectionBaseto see that it inheritsDeclarationInspectionBaseBase. Obviously that ambiguity is not what we want.The only suggestion I can think of is
PartialBase, in similar fashion to thepartialclass 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 theBaseBaseexplaining that it is meant to be implemented by other base classes and not directly.