Make the annotation API consistent with the other APIs#1047
Merged
swissiety merged 1 commit intosoot-oss:developfrom Sep 12, 2024
Merged
Make the annotation API consistent with the other APIs#1047swissiety merged 1 commit intosoot-oss:developfrom
swissiety merged 1 commit intosoot-oss:developfrom
Conversation
Collaborator
marcus-h
commented
Aug 29, 2024
229ca38 to
09f1b85
Compare
JonasKlauke
approved these changes
Sep 9, 2024
Collaborator
JonasKlauke
left a comment
There was a problem hiding this comment.
Coreviewed by @swissiety
| } | ||
| } | ||
|
|
||
| ((JavaSootClassSource) classSource).resolveAnnotations().forEach(annotationUsages::add); |
Collaborator
There was a problem hiding this comment.
simplify. use collect(asList)
Collaborator
Author
There was a problem hiding this comment.
I just had a look at it: JavaSootClassSource.resolveAnnotations returns an
Iterable so we could
- return it directly (drawback: in case of the AsmClassSource,
resolveAnnotations always recomputes the AnnotationUsages from scratch (also
not ideal) so returning it directly is OK but if the class is backed up by
a different class source, this could be an issue (e.g., it could return
a mutable List etc., which is then modified by the user)) - convert the Iterable to a stream and then to a list (I don't see any benefit)
- keep it as is
Any preferences?:)
Collaborator
There was a problem hiding this comment.
lets cache it & return as Collections.unmodifiableList()?
swissiety
approved these changes
Sep 9, 2024
Collaborator
swissiety
left a comment
There was a problem hiding this comment.
<3 for removing not so well designed clutter
0708100 to
f5f7aaf
Compare
The old annotation API was pretty inconsistent with all other SootUp APIs because several methods expected a JavaView object. Examples: - JavaSootClass.getAnnotations(@nonnull Optional<JavaView> view) - JavaSootMethod.getAnnotations(@nonnull Optional<JavaView> view) For instance, in JavaSootClass.getAnnotations the JavaView is used to - retrieve the superclass' JavaSootClass in order to take inherited annotations into account - retrieve potential default values from an AnnotationType While this might come handy, this "conflicts" with existing SootUp APIs. For instance, JavaSootClass.getMethods() only returns the methods that are defined in the corresponding class and does not take methods from the superclass or interfaces into account. These methods have to be manually retrieved by first retrieving the superclass' type from the JavaSootClass, then the JavaSootClass that corresponds to the superclass' type is queried from the JavaView, and, finally, the getMethods() method is called on that JavaSootClass instance etc. Hence, we make the annotation API consistent with the other existing APIs. Unfortunately, this breaks the existing API. (Incompatible) API changes: - java.core.AnnotationUsage: * getAnnotation() returns a ClassType instead of an AnnotationType * getValuesWithDefaults() is removed without any replacement. For now, the merging of the actual values with the default values has to be done manually. - java.core.JavaAnnotationSootClass: * getDefaultValues() is introduced in order to retrieve the default values that are defined in the corresponding annotation interface. - java.core.JavaIdentifierFactory: * getAnnotationType(...) is removed without any replacement. - java.core.JavaPackageName: * getAnnotations(...) and a constructor that takes a list of AnnotationUsage instances are removed without any replacement (they are unused within SootUp's codebase). The potential annotations can be retrieved by querying the corresponding package-info file from the JavaView (for instance, see the newly introduced PackageAnnotationTest.testPackageAnnotation test). - java.core.JavaSootClass: * getAnnotations(@nonnull Optional<JavaView> view) is replaced by getAnnotations(), which does *not* take potential inherited annotations or default values into account. - java.core.JavaSootMethod, java.core.JavaSootField: * as in the JavaSootClass case above. - java.core.types.AnnotationType: This class is removed. An annotation interface type is represented by a (Java-) ClassType. - bytecode.frontend.AsmUtil.createAnnotationUsage(List<AnnotationNode> invisibleParameterAnnotation): If the annotationValue is an instance of AnnotationNode, the old code stored a list, which contains a single AnnotationUsage instance, in the paramMap. This is counterintuitive because this does not reflect the "structure" of the element's type in the annotation interface definition. Hence, the new code stores the AnnotationUsage instance directly in the paramMap. The new behavior is documented in the AnnotationUsageTest.testAnnotationWithNestedAnnotationDefaultValues test. The existing NestedAnnotationTest.testNestedAnnotation is adjusted accordingly. - bytecode.frontend.AsmMethodSource.resolveAnnotationsInDefaultValue(Object a): If the passed object "a" is an instance of AnnotationNode, the old code transformed it into a list containing a single AnnotationUsage instance. As above, this is counterintuitive because it does not reflect the element's type structure in the annotation interface definition. Hence, the new code just returns the AnnotationUsage itself without wrapping it in a list. The new behavior is documented in the AnnotationUsageTest.testContainerAnnotationDefaultValues test. Note: Some of the reformatting noise in JavaSootMethod was caused by running "mvn fmt:format"...
f5f7aaf to
f0767c8
Compare
This file contains hidden or 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
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.