Java: Add support for Annotation types stub generation#8695
Conversation
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Hopefully the following comments are helpful, and this is not too intrusive.
I am not a member of the project so feel free to consider these only as suggestions.
Marcono1234
left a comment
Marcono1234
left a comment
There was a problem hiding this comment.
Thanks a lot for considering the feedback!
There might be a few remaining issues.
Regarding annotation default values: There might not be any additional work needed because they seem to be included when an annotation is used (at least the last time I checked this), see also #6275.
atorralba
force-pushed
the
atorralba/stub-generator-annotation-types
branch
from
1f1e494 to
fdf6510
Compare
tamasvajk
left a comment
tamasvajk
left a comment
There was a problem hiding this comment.
Should there be a test for this? There's a similar query in C#, we added a not so human readable test in https://github.com/github/codeql/tree/main/csharp/ql/test/query-tests/Stubs/Minimal. It might be worth considering for Java too.
atorralba
force-pushed
the
atorralba/stub-generator-annotation-types
branch
from
fdf6510 to
7687c8c
Compare
Good point @tamasvajk, thanks! I had to create a See 7687c8c for details, let me know if something doesn't add up. |
tamasvajk
left a comment
tamasvajk
left a comment
There was a problem hiding this comment.
Looks reasonable to me. I added a question regarding stable ordering of elements, feel free to ignore it as it's not specific to annotations.
| } | ||
|
|
||
| private string stubAnnotations() { | ||
| result = concat(stubAnnotation(this.(AnnotationType).getAnAnnotation()) + "\n") |
There was a problem hiding this comment.
Do we need a stable order for the annotations? I see that we don't order the members in stubMembers either, but we do order by all concats in C#. Just wondering.
There was a problem hiding this comment.
This would seem wise, ordering by their fully-qualified names for example
| result = this.(AnnotationType).getAnAnnotation().getAValue().getType() or | ||
| result = this.(AnnotationType).getAnAnnotation().getAValue().(ArrayInit).getAnInit().getType() |
There was a problem hiding this comment.
Note that getAValue was recently deprecated because it's usually not useful -- use getAnArrayValue(_) instead, which unpacks array members
|
|
||
| language[monotonicAggregates] | ||
| private string stubAnnotation(Annotation a) { | ||
| if exists(a.getAValue()) |
atorralba
force-pushed
the
atorralba/stub-generator-annotation-types
branch
from
7687c8c to
f860ae8
Compare
|
Thanks for the review @tamasvajk @smowton and sorry for the delay. Comments addressed in f860ae8. |
| result = concat(stubAnnotation(this.(AnnotationType).getAnAnnotation()) + "\n") | ||
| result = | ||
| concat(Annotation an | | ||
| this.(AnnotationType).getAnAnnotation() = an | ||
| | | ||
| stubAnnotation(an), "\n" order by an.getType().getQualifiedName() |
There was a problem hiding this comment.
This is actually a behavior change: Previously it would add a \n after each annotation. Now it only adds it between annotations but the last one does not have a trailing \n (see also test output changes).
When the last annotation has values (and therefore ends with )) that still seems to produce valid Java code, but I assume when an annotation has no values this produces malformed Java code:
@MyAnnotationpublic void m() {
}There was a problem hiding this comment.
Thanks @Marcono1234, good catch. I addressed this in #10662.
Add line break after all stubbed annotations to avoid malformed code See https://github.com/github/codeql/pull/8695\#discussion_r985674245
Add line break after all stubbed annotations to avoid malformed code See https://github.com/github/codeql/pull/8695\#discussion_r985674245
5 participants
Adds support for Annotation types in our Java stub generator.
Due to default values for Annotation methods not being currently supported, the stubs generated may need manual correction if the original types have default values, e.g. https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/bind/annotation/RequestMapping.java#L85.