Redesign Annotation Processing #5107
Merged
Conversation
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 redesigns annotation processing to no longer rely on the AnnotationTypes enum to identify an annotation, allowing us to separate the scoping of an annotation from it's type. Additionally this explicitly stores metainformation on annotations in Attributes on the specific annotation type. This metainformation is used to automatically register annotations for the code pane parsing process. These changes have far-reaching implications for how annotations are used, most of them addressed in this commit. The parsing of annotations through the VBA API is not correctly dealt with.
This moves static information on annotations into implementations of IAnnotation and introduces a ParseTreeAnnotation class to correlate parser contexts to annotations. With that change we get a clearer separation between the annotation and it's use. For workability, a handful of extension methods have been added, some have been adapted. Additionally a large number of predicates and filterings have been adjusted to the new API.
Vogel612
force-pushed
the
refactor/annotation-processing
branch
from
c24e63e
to
5ac876e
Compare
bclothier
reviewed
Sep 1, 2019
MDoerner
reviewed
Sep 2, 2019
There was a problem hiding this comment.
First of all: good work!
I have looked at everything except the tests so far and I have to say that I like the new design, with a few exceptions:
- I do not really like extension methods for
ParseTreeAnnotation. They hide the implementation away, but still provide the impression that all such annotations might have attributes. I would prefer, if the check whether the contained annotation is an attribute annotation remained a responsibility of the caller. - I am not really happy about constructor on
ParseTreeAnnotationonly for tests. As @bclothier mentioned, this can be avoided by extracting an interface and mocking that in the tests. - The IoC setup seems to be not in sync with the implementation. (Have you loaded the current version in some host?) With the new constructor setup, two simple conventions could replace the explicit reflection.
Rubberduck.CodeAnalysis/Inspections/Concrete/ObsoleteMemberUsageInspection.cs
Outdated
Show resolved
Hide resolved
Rubberduck.CodeAnalysis/Inspections/Concrete/DuplicatedAnnotationInspection.cs
Show resolved
Hide resolved
Rubberduck.CodeAnalysis/QuickFixes/RemoveDuplicatedAnnotationQuickFix.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public static class AttributeAnnotationExtensions | ||
| { | ||
| public static string Attribute(this ParseTreeAnnotation annotationInstance) |
There was a problem hiding this comment.
I do not really like this extension method. For one, since there is only one ParseTreeAnnotation, it might as well be right on the interface.
Moreover, with this extension method we say that every ParseTreeAnnotation has an attribute, but it might be null, instead of saying that you can get an attribute from the annotation, if it implements the correct interface.
There was a problem hiding this comment.
yea, it feels dirty... Rewritten to be an extension of IAttributeAnnotation
Rubberduck.Parsing/Annotations/Implementations/FlexibleAttributeValueAnnotationBase.cs
Outdated
Show resolved
Hide resolved
Rubberduck.Parsing/VBA/ReferenceManagement/IdentifierReferenceResolver.cs
Outdated
Show resolved
Hide resolved
MDoerner
reviewed
Sep 2, 2019
|
|
||
| Assert.IsInstanceOf<MemberAttributeAnnotation>(inspectionResult.Properties.Annotation.Annotation); | ||
| Assert.AreEqual("VB_UserMemId", inspectionResult.Properties.AttributeName); | ||
| Assert.AreEqual("-4", ((ParseTreeAnnotation)inspectionResult.Properties.Annotation).AttributeValues()[0]); |
There was a problem hiding this comment.
Not for this PR, would it make sense to add specific subtypes for the different inspection result types for those cases in which additional information needs to be transferred to the user of the result?
| @@ -153,7 +156,9 @@ public void KnownMemberAttributeWithoutAnnotationWhileOtherAttributeWithAnnotati | |||
|
|
|||
| protected override IQuickFix QuickFix(RubberduckParserState state) | |||
| { | |||
| return new AddAttributeAnnotationQuickFix(new AnnotationUpdater(), new AttributeAnnotationProvider()); | |||
| // FIXME actually inject the annotations here... | |||
| return new AddAttributeAnnotationQuickFix(new AnnotationUpdater(), | |||
There was a problem hiding this comment.
Can we have this on its own line as well?
There was a problem hiding this comment.
not quite sure what you mean here?
There was a problem hiding this comment.
We usually have the first argument also on a new line, IIRC.
This means an interface IParseTreeAnnotation has been extracted. Additionally the CW registration has been adjusted, multiple minor fixes and tweaks to usage sites have been done. IAnnotation now exposes a method to allow dealing with annotation arguments as they are added to the ParseTreeAnnotation itself. By default that just passes the arguments through.
bclothier
approved these changes
Sep 6, 2019
Vogel612
force-pushed
the
refactor/annotation-processing
branch
from
711e3fc
to
4a7b243
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I've split the current
IAnnotationinto anIAnnotationfor dealing with the static information and aParseTreeAnnotationcombining anIAnnotationand the non-static information.This cleanly separates what kinds of annotations there are and what an annotation usage is. In addition this removes the special casing for the ExcelHotKeyAnnotation in the AnnotationAttributeProvider