Skip to content

Make the annotation API consistent with the other APIs#1047

Merged
swissiety merged 1 commit intosoot-oss:developfrom
marcus-h:annotations
Sep 12, 2024
Merged

Make the annotation API consistent with the other APIs#1047
swissiety merged 1 commit intosoot-oss:developfrom
marcus-h:annotations

Conversation

@marcus-h
Copy link
Copy Markdown
Collaborator

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"...

@marcus-h marcus-h force-pushed the annotations branch 2 times, most recently from 229ca38 to 09f1b85 Compare August 29, 2024 16:16
Copy link
Copy Markdown
Collaborator

@JonasKlauke JonasKlauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coreviewed by @swissiety

}
}

((JavaSootClassSource) classSource).resolveAnnotations().forEach(annotationUsages::add);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplify. use collect(asList)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?:)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets cache it & return as Collections.unmodifiableList()?

@swissiety swissiety self-requested a review September 9, 2024 11:45
Copy link
Copy Markdown
Collaborator

@swissiety swissiety left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3 for removing not so well designed clutter

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"...
@swissiety swissiety merged commit 2927fcd into soot-oss:develop Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants