Java: Improve and add predicates and classes for annotations #6246

Marcono1234 · 2021-07-10T22:46:41Z

Tries to improve the CodeQL classes and predicates for working with annotations.
The most important changes are in:

java/ql/src/semmle/code/java/Annotation.qll
java/ql/src/semmle/code/java/JDKAnnotations.qll

smowton

Looks broadly good. Three top-level suggestions:

Maintain backward compatibility: let the existing getAnAnnotation etc retain the current behaviour and introduce new methods that include inheritance
Some simple changes (e.g. introducing getReferencedType, getCheckedType) are noisy and not very related to annotations. Let these have their own PR.
Let's have some simple tests to check the right methods do and don't see inheritance

smowton · 2021-07-13T09:10:43Z

java/ql/src/semmle/code/java/Annotation.qll

+   * gets the enum constant used as value for that element. This includes default values in
+   * case no explicit value is specified.
+   */
+  EnumConstant getValueEnumConstant(string name) { result = getValue(name).(FieldRead).getField() }


This isn't really specific to annotations. Perhaps provide a class EnumConstantRead extends FieldRead with relevant convenience methods?

This isn't really specific to annotations.

It is, isn't it? Enums are one of the value types an annotation type element can have.
Consider this example for @Target:

@Target(ElementType.ANNOTATION_TYPE) public @interface MetaAnnotationType { ... }

The added CodeQL predicate would allow easily getting ElementType.ANNOTATION_TYPE here.

How would a EnumConstantRead help for this use case? Or do you mean something different?

I mean that the desire to interpret a FieldRead as an enum constant read goes far beyond annotations, so we should provide something like

class EnumConstantRead extends FieldRead { EnumConstantRead() { this.getField() instanceof EnumConstant } EnumConstant getEnumConstant() { result = this.getField() } }

Then use it in this method.

Ah, ok I thought you meant this predicate should be removed.

Though I am not sure if EnumConstantRead adds that much value; it probably only saves you one EnumConstant cast for the result of getField() in most cases.

java/ql/src/semmle/code/java/Annotation.qll

smowton · 2021-07-13T09:12:34Z

java/ql/src/semmle/code/java/Annotation.qll

+   * gets the referenced type used as value for that element. This includes default values
+   * in case no explicit value is specified.
+   */
+  Type getValueClass(string name) { result = getValue(name).(TypeLiteral).getReferencedType() }


Rename getReferencedType for consistency

Classes are only one possible value type of annotation type elements, I chose getValue... for consistency with the other predicates retrieving an annotation value.

Have renamed it to getTypeValue.

java/ql/src/semmle/code/java/Annotation.qll

smowton · 2021-07-13T09:24:26Z

java/ql/src/semmle/code/java/Annotation.qll

+   */
+  // Note: Cannot use a predicate with string as result because annotation type without
+  // explicit @Target can be applied to all targets, requiring to hardcode element types here
+  bindingset[elementType]


If there is a small set of possible values you can do something like class AnnotationTargetType extends string { AnnotationTargetType() { this = ["CLASS", "METHOD", ...] } } and drop the bindingset

Yes that would be doable, but I wanted to avoid this (see "Note: .." comment a few lines above) because the set of possible target types would have to be adjusted then whenever a new constant is added to ElementType (recents ones were MODULE and RECORD_COMPONENT).

Additionally it might cause confusing results when a Java database is not using the latest Java version. Consider a Java 8 project for which this predicate claims that RECORD_COMPONENT (added for Java 16) is a target.
But maybe that is not actually a big issue, especially since this predicate already relies on JDK-8231435 which was fixed only for Java 14.

I would prefer not using bindingset here as well.

java/ql/src/semmle/code/java/JDKAnnotations.qll

smowton · 2021-07-13T09:26:54Z

java/ql/src/semmle/code/java/JDKAnnotations.qll

-      result = this.getAValue() or
-      result = this.getAValue().(ArrayInit).getAnInit()
-    )
+    result = this.getAValue(_)


Keep a no-arg getAValue method, since we seem to use it a lot?

I did not remove getAValue(). But as mentioned in https://github.com/github/codeql/pull/6246/files#r668908394, getAValue() and getAValue(string) are actually for different use cases. The first one is for single values, the second one is for single and array values.

But while looking at it, I think the not result instanceof ArrayInit and in the line above can now be removed with this change.

Actually I have marked getAValue() as deprecated now because it is error-prone. It could by accident match the wrong annotation type element, especially when in the future new elements are added to an annotation type.

Marcono1234 · 2021-07-13T16:40:19Z

Maintain backward compatibility: let the existing getAnAnnotation etc retain the current behaviour and introduce new methods that include inheritance

getAnAnnotation() behavior did not change (at least not on purpose). It previously considered inheritance (as defined by @Inherited) already, but that is only implemented for CodeQL's Class since it only applies to Java classes. The change here is that this documented already for Annotatable to make it more clear for the user.

However, hasAnnotation() was changed, previously it did not consider inherited annotations. But keeping old behavior (not considering inherited annotations) would probably be confusing because the similarly named predicates getAnAnnotation() and hasAnnotation(string, string) did and still do consider inherited annotations.

Some simple changes (e.g. introducing getReferencedType, getCheckedType) are noisy and not very related to annotations. Let these have their own PR.

Fair point, I will create a separate pull request for them.

smowton · 2021-07-14T11:30:15Z

Agreed that hasAnnotation deviating from getAnAnnotation is likely a bug and we probably don't want to preserve the old behaviour. Deprecating the confusing getAValue if it doesn't yield single values also sounds sensible.

Marcono1234 · 2022-03-27T22:42:41Z

Sorry for the long delay. Have rebased the changes onto main and addressed some of the review comments. The pull request is still not completely ready, but I hope it is going in the right direction.

smowton

Looks plausible, just some doc fixes here

java/ql/lib/semmle/code/java/Annotation.qll

smowton · 2022-03-31T09:56:55Z

java/ql/lib/semmle/code/java/Annotation.qll

+  }
+
+  /**
+   * If the annotation element with the specified `name` has a Java `Class` as value type,


Suggested change

* If the annotation element with the specified `name` has a Java `Class` as value type,

* If the annotation element with the specified `name` is of type `java.lang.Class`,

Have chosen now the following to be consistent with the documentation of the other predicates:

If the value type of the annotation element with the specified name is java.lang.Class

I hope that is alright.

java/ql/lib/semmle/code/java/Annotation.qll

smowton · 2022-03-31T10:10:45Z

java/ql/lib/semmle/code/java/Annotation.qll

+    then elementType = getADeclaredAnnotation().(TargetAnnotation).getATargetElementType()
+    else
+      // No Target annotation means "applicable to all contexts" since JDK 14, see https://bugs.openjdk.java.net/browse/JDK-8231435
+      // The compiler does not completely implement that, but pretend it did


Expand on this?

It think I was referring to some bugs with module declarations, such as JDK-8254023; I will expand the comment.

However, the behavior for a missing @Target annotation changed again in Java 17, now it only includes declaration contexts, but excludes type usage contexts (JDK-8261610). I will mention this in the comment as well.
I will probably still keep the predicate as isATargetType for now but exclude TYPE_USE if no @Target annotation is present, this seems slightly less error-prone than a getATargetType() predicate which is more likely to become outdated as the JDK evolves and new declaration contexts are added.

Or what do you think?

How about naming the predicate mayBeTargetType, and note in qldoc that the meaning of no-@Target has changed over time, with this predicate taking the broadest interpretation?

mayBeTargetType (or rather mayBeATargetType?) sounds a bit weird to me, but if you want I can rename it nonetheless.
Though extending the documentation sounds like a good idea, but I think I will rather mention "latest Java version" or similar, since "broadest" might sound like applicable in all contexts (including type contexts), which is no longer the case since Java 17.

getALikelyTargetType?

getA... would have to be a predicate with result then, right? That is not possible with the current implementation.
My main concerns with mayBeTargetType is that personally I find that "may be" sounds a bit weird (and too close to "maybe", especially as predicate / method prefix), but this is probably rather subjective; and that this name expresses some uncertainty (I think), even though the predicate is fairly reliable (unless you provide it with strings which are not ElementType values).

smowton · 2022-03-31T10:11:09Z

java/ql/lib/semmle/code/java/Annotation.qll

+  }
+
+  /** Holds if this annotation type is annotated with the meta-annotation `@Repeatable`. */
+  predicate isRepeatable() { getADeclaredAnnotation() instanceof RepeatableAnnotation }


getADeclared... this isn't inheritable?

Yes getADeclaredAnnotation() does not include inherited annotations, but @Repeatable is not inheritable, so this is correct, isn't it?

I didn't know, was just asking you to ensure that's what you intended. Sounds like it's fine.

Marcono1234 · 2022-04-02T17:16:17Z

I think this pull request is now complete and ready for a full review. Thanks already for the previous comments!

Marcono1234 · 2022-04-03T18:01:27Z

java/ql/lib/change-notes/2022-04-01-annotation-deprecations.md

+* The predicate `Annotation.getAValue()` has been deprecated because it might lead to obtaining the value of the wrong annotation element by accident. `getValue(string)` (or one of the value type specific predicates) should be used to explicitly specify the name of the annotation element.
+* The predicate `Annotation.getAValue(string)` has been renamed to `getAnArrayValue(string)`.
+* The predicate `SuppressWarningsAnnotation.getASuppressedWarningLiteral()` has been deprecated because it unnecessarily restricts the result type; `getASuppressedWarning()` should be used instead.
+* The predicates `TargetAnnotation.getATargetExpression()` and `RetentionAnnotation.getRetentionPolicyExpression()` have been deprecated because getting the enum constant read expression is rarely useful, instead the corresponding predicates for getting the name of the referenced enum constants should be used.


Hmm, maybe there are use cases where one wants to get the location of the expression, e.g. to include it in the query result set. But those use cases might still be rare and rather apply to custom annotations and not these JDK default ones.

aschackmull

LGTM.

aschackmull

Missing autoformat.

java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll

aschackmull · 2022-04-25T12:00:38Z

--- expected
+++ actual
@@ -1,1 +1,20 @@
-
+[VALUE_NOT_IN_TYPE] predicate hasLocation(@locatable locatableid, @location id): Value 28381 of field locatableid is not in type @locatable. Appears in tuple (28381,28382)
+	Relevant element: locatableid=28381
+		Full ID for 28381: @"annotation;(27870);(452)". The ID may expand to @"annotation;{@"annotation;{@"field;{@"class;AnnotationValues"};arrayValuesSingleExpr"};{@"class;AnnotationValues$ArrayValues"}"};{@"callable;{@"class;AnnotationValues$ArrayValues"}.annotationValues(){@"array;1;{@"class;AnnotationValues$CustomAnnotation"}"}"}"
+[VALUE_NOT_IN_TYPE] predicate annotValue(@annotation parentid, @method id2, @expr value): Value 28381 of field parentid is not in type @annotation. Appears in tuple (28381,517,28383)
+	Relevant element: parentid=28381
+		Full ID for 28381: @"annotation;(27870);(452)". The ID may expand to @"annotation;{@"annotation;{@"field;{@"class;AnnotationValues"};arrayValuesSingleExpr"};{@"class;AnnotationValues$ArrayValues"}"};{@"callable;{@"class;AnnotationValues$ArrayValues"}.annotationValues(){@"array;1;{@"class;AnnotationValues$CustomAnnotation"}"}"}"
+	Relevant element: id2=517
+		Full ID for 517: @"callable;(448).value()(43)". The ID may expand to @"callable;{@"class;AnnotationValues$CustomAnnotation"}.value(){@"class;java.lang.String"}"
+[VALUE_NOT_IN_TYPE] predicate annotValue(@annotation parentid, @method id2, @expr value): Value 28381 of field value is not in type @expr. Appears in tuple (27870,452,28381)
+	Relevant element: value=28381
+		Full ID for 28381: @"annotation;(27870);(452)". The ID may expand to @"annotation;{@"annotation;{@"field;{@"class;AnnotationValues"};arrayValuesSingleExpr"};{@"class;AnnotationValues$ArrayValues"}"};{@"callable;{@"class;AnnotationValues$ArrayValues"}.annotationValues(){@"array;1;{@"class;AnnotationValues$CustomAnnotation"}"}"}"
+	Relevant element: parentid=27870
+		Full ID for 27870: @"annotation;(27869);(444)". The ID may expand to @"annotation;{@"field;{@"class;AnnotationValues"};arrayValuesSingleExpr"};{@"class;AnnotationValues$ArrayValues"}"
+	Relevant element: id2=452
+		Full ID for 452: @"callable;(444).annotationValues()(449)". The ID may expand to @"callable;{@"class;AnnotationValues$ArrayValues"}.annotationValues(){@"array;1;{@"class;AnnotationValues$CustomAnnotation"}"}"
+[VALUE_NOT_IN_TYPE] predicate exprs(@expr id, int kind, @type typeid, @exprparent parent, int idx): Value 28381 of field parent is not in type @exprparent. Appears in tuple (28383,22,43,28381,-1)
+	Relevant element: parent=28381
+		Full ID for 28381: @"annotation;(27870);(452)". The ID may expand to @"annotation;{@"annotation;{@"field;{@"class;AnnotationValues"};arrayValuesSingleExpr"};{@"class;AnnotationValues$ArrayValues"}"};{@"callable;{@"class;AnnotationValues$ArrayValues"}.annotationValues(){@"array;1;{@"class;AnnotationValues$CustomAnnotation"}"}"}"
+	Relevant element: typeid=43
+		Full ID for 43: @"class;java.lang.String"
[2/8] [36/133 eval 254ms] FAILED(EXTRACTION) /home/runner/work/semmle-code/semmle-code/ql/java/ql/test/library-tests/annotations/DB-CHECK

aschackmull · 2022-04-25T12:02:42Z

@smowton Looks like we hit an extractor error here.

…s result

…tions Additionally changes `hasAnnotation()` to consider inherited annotations for consistency.

Predicate name could lead to confusion with non-array predicate getAValue()

As suggested by smowton during review.

…ppingGetMethod

As mentioned by smowton during review, the predicate only has a single result due to being restricted by the index and therefore its name should not start with "getA...". Also remove deprecated `getAValue(string, int)` because it never existed on the `main` branch.

Autoformat

…ion handling has been fixed

smowton · 2022-09-16T14:58:39Z

I just merged a fix to the Java extractor that resolves the database inconsistency seen above. I've rebased this PR on latest main and added a commit amending the test expectations to take the extractor fix into account.

@Marcono1234 I don't think this should impact your work, but the modified extractor records an annotValue for nested annotations that always uses an array constructor in the case of an annotation-array-typed field. For example, if we have an annotation @interface Anno1 { Anno2[] nestedAnnotations; } and a usage @Anno1(nestedAnnotations = @Anno2("value")), then the annotValue table will record Anno1 having a value { @Anno2 ... }, i.e. using an array constructor even though that was implicit at source level. This differs from primitive arrays, where @interface Anno1 { String[] strings; } ... @Anno1(strings = "value") would yield two annotValue entries, one pointing at the array { "value" } and one pointing at the raw value "value".

Marcono1234 · 2022-09-16T20:15:46Z

Thanks! I guess that is fine and I assume you had your reasons for implementing it like this in the extractor.

would yield two annotValue entries, one pointing at the array { "value" } and one pointing at the raw value "value"

One would be the source expression and one the non-source expression, right?

Is the "Java Language Tests" check failure due to a failing test, or have your extractor changes simply not been included in the CodeQL version running the "Java Language Tests" yet?

smowton · 2022-09-20T09:13:56Z

It failed because the deprecation for Annotation.getAValue appears in test output, which I hadn't realised! I'll push a commit to rephrase that test.

smowton

Proxying Schack's previous approval

github-actions bot added the Java label Jul 10, 2021

smowton reviewed Jul 13, 2021

View reviewed changes

Marcono1234 force-pushed the marcono1234/annotation-improvements branch from a82a672 to 5b5d837 Compare March 27, 2022 22:27

smowton reviewed Mar 31, 2022

View reviewed changes

github-actions bot added the documentation label Apr 2, 2022

Marcono1234 marked this pull request as ready for review April 2, 2022 17:16

Marcono1234 requested a review from a team as a code owner April 2, 2022 17:16

Marcono1234 commented Apr 3, 2022

View reviewed changes

Marcono1234 force-pushed the marcono1234/annotation-improvements branch from 54985a6 to 31ea7a5 Compare April 14, 2022 20:55

aschackmull previously approved these changes Apr 20, 2022

View reviewed changes

aschackmull reviewed Apr 25, 2022

View reviewed changes

java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll Outdated Show resolved Hide resolved

java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll Outdated Show resolved Hide resolved

aschackmull dismissed their stale review via 209686d April 25, 2022 11:07

aschackmull previously approved these changes Apr 25, 2022

View reviewed changes

Marcono1234 added 11 commits September 16, 2022 15:49

Java: Add Annotation value convenience predicates

536f5c7

Java: Clarify that Annotation value predicates have default value a…

afb7462

…s result

Java: Clarify that Annotatable predicates consider inherited annota…

f69b6ee

…tions Additionally changes `hasAnnotation()` to consider inherited annotations for consistency.

Java: Add convenience predicates for AnnotationType

02c8fe9

Java: Add classes and predicates for @Repeatable

c226758

Java: Rename Annotation value predicates

b96061a

Java: Rename Annotation.getAValue predicates for array values

fd5fdd8

Predicate name could lead to confusion with non-array predicate getAValue()

Java: Improve Annotation.getAnAssociatedAnnotation

47e3895

As suggested by smowton during review.

Java: Add convenience array value Annotation predicates

998aa95

Java: Fix incorrect annotation handling for SpringControllerRequestMa…

e3c1b96

…ppingGetMethod

Java: Deprecate error-prone and rarely used annotation predicates

4ef2d15

Marcono1234 and others added 7 commits September 16, 2022 15:49

Java: Deprecate RetentionAnnotation.getRetentionPolicyExpression()

659a3a7

Java: Address Annotation review comments and add change note

8c9bdeb

Java: Add annotation tests

37b1891

Java: Extend AnnotationType.isATargetType documentation

c8b9229

Apply suggestions from code review

1945f18

Autoformat

Update test expectations now that the Java extractor's nested annotat…

0ab5d46

…ion handling has been fixed

smowton dismissed aschackmull’s stale review via 0ab5d46 September 16, 2022 14:51

smowton force-pushed the marcono1234/annotation-improvements branch from 209686d to 0ab5d46 Compare September 16, 2022 14:51

Avoid deprecated Annotation.getAValue

14fa6d4

smowton approved these changes Sep 20, 2022

View reviewed changes

smowton merged commit f826342 into github:main Sep 20, 2022

Marcono1234 deleted the marcono1234/annotation-improvements branch September 20, 2022 19:50

Marcono1234 mentioned this pull request Jul 21, 2024

Java: Update Annotation predicate examples in language guide #17026

Merged

	* If the annotation element with the specified `name` has a Java `Class` as value type,
	* If the annotation element with the specified `name` is of type `java.lang.Class`,

Java: Improve and add predicates and classes for annotations #6246

Java: Improve and add predicates and classes for annotations #6246

Uh oh!

Conversation

Marcono1234 commented Jul 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Mar 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 commented Jul 13, 2021

Uh oh!

smowton commented Jul 14, 2021

Uh oh!

Marcono1234 commented Mar 27, 2022

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 commented Jul 10, 2021 •

edited

Loading

Marcono1234 Mar 27, 2022 •

edited

Loading

Marcono1234 Apr 13, 2022 •

edited

Loading