Don't write irrelevant annotations in .ajava files#6254
Don't write irrelevant annotations in .ajava files#6254mernst wants to merge 19 commits intotypetools:masterfrom
Conversation
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
Overall this is a nice change, but I have a few concerns that I think need to be addressed before it's ready to merge.
| } else { | ||
| TypeMirror relevantType = TypesUtils.typeFromClass(clazz, types, elements); | ||
| relevantJavaTypes.add(types.erasure(relevantType)); | ||
| TypeMirror erased = types.erasure(relevantType); |
There was a problem hiding this comment.
relevantJavaTypesNames, at least, is only used by WPI (i.e., in this PR). That means that most (all?) of this computation is wasted for non-WPI runs of the checker, which makes me a bit uncomfortable: it does not seem reasonable to slow down the checker in the non-WPI case to speed it up in the WPI case. Can we guard some of this work behind a check that WPI is actually running?
I think this will also complicate the specification of relevantJavaTypeNames, which will need to say something like "always null if WPI is not enabled"
There was a problem hiding this comment.
There are 71 instances of extends BaseTypeChecker in the Checker Framework (excluding checkers used only for testing). Of those:
58 have 0 classes in a @RelevantJavaTypes annotation
5 have 1 class in a @RelevantJavaTypes annotation
2 have 3 classes in a @RelevantJavaTypes annotation
1 has 6 classes in a @RelevantJavaTypes annotation
1 has 8 classes in a @RelevantJavaTypes annotation
4 have 10 classes in a @RelevantJavaTypes annotation
So, I don't think this is a heavy cost, either in time to compute the strings nor in space to store the strings.
But if @smillst agrees with the efficiency concern, I will implement it.
| DependentTypesViewpointAdaptationTest this, DependentTypesViewpointAdaptationTest other) { | ||
| // :: warning: (assignment) | ||
| @SameLen("this") DependentTypesViewpointAdaptationTest myOther = other; | ||
| DependentTypesViewpointAdaptationTest myOther = other; |
There was a problem hiding this comment.
Why were these annotations removed?
If DependentTypesViewpointAdapationTest isn't being considered a relevant Java type for @SameLen at this location, I think that's a problem with either @SameLen's definition of its relevant types or with the implementation in this PR, not with this test.
There was a problem hiding this comment.
SameLenChecker.java contains:
@RelevantJavaTypes({CharSequence.class, Object[].class, Object.class})Is that incorrect? Why is @SameLen applicable to an arbitrary class?
There was a problem hiding this comment.
The @RelevantJavaTypes annotation is probably incorrect. The pattern that this test is testing is documented in the manual here: https://checkerframework.org/manual/#index-annotating-fixed-size
| int dotIndex = typeString.lastIndexOf('.'); | ||
| if (dotIndex != -1) { | ||
| // It's a fully-qualified name. Add the simple name as well. | ||
| // TODO: This might not handle a user writing a nested class like "Map.Entry". |
There was a problem hiding this comment.
Given the TODO comments here and about handling type variables (at WholeProgramInferenceJavaParserStorage line 1100 or so), I think it's clear we need a test case for this change where an annotation's relevant Java types include at least one class like Map.Entry: it has a type parameter, and it is an inner class (and is usually referenced that way).
Probably the easiest way to test that would be to add another annotation to the AinferTestChecker with the appropriate relevant type, and a corresponding test that ensures it can be inferred in the appropriate place.
…JavaTypes into dont-insert-irrelevant
…JavaTypes into dont-insert-irrelevant
📝 WalkthroughWalkthroughThis pull request makes three changes: removes a trailing blank line from a test resource file, adds annotation relevance filtering logic to WholeProgramInferenceJavaParserStorage that skips printing non-relevant qualifiers based on type context, and enhances GenericAnnotatedTypeFactory with a new field (relevantJavaTypeNames) and public methods (arraysAreRelevant(), nonprimitivesAreRelevant(), and isRelevant(String type)) to track and query relevant Java types by their string names in addition to existing TypeMirror tracking. Suggested reviewers
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`:
- Around line 1128-1134: The current ClassOrInterfaceType check in
WholeProgramInferenceJavaParserStorage uses only string name matching
(simpleName/scopedName) against gatf.isRelevant, which misses relevance
determined by subtype/supertype relations; change the logic in the block
handling parentNode instanceof ClassOrInterfaceType (the classType variable) to
resolve the declared type and call the type-aware relevance method (e.g.,
gatf.isRelevant(resolvedType) or gatf.isRelevant(TypeMirror)) instead of string
comparisons—use the JavaParser symbol/resolution API to obtain a
ResolvedReferenceType/TypeMirror for classType, catch resolution exceptions and
fall back to the existing name checks if resolution fails, and ensure you
reference WholeProgramInferenceJavaParserStorage, ClassOrInterfaceType, and gatf
in the fix.
- Around line 1123-1160: annotationIsRelevant currently calls
anno.getParentNode().get() (which can throw NoSuchElementException) and uses
throw new Error(...) for unknown parent cases; change it to be conservative by
replacing anno.getParentNode().get() with a safe Optional check (e.g.,
anno.getParentNode().orElse(null)) and if parentNode is null return true, change
the Parameter non-varargs branch (currently throwing) to return true instead of
throwing, and replace the final throw new Error(...) with a return true so any
unrecognized parent types are treated as relevant rather than aborting AJAVA
writing.
In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`:
- Around line 406-408: The variable nonprimitivesAreRelevantTemp is being set
with inverted logic when checking clazz.isPrimitive(): instead of setting it
true for primitive classes, set nonprimitivesAreRelevantTemp = false when
clazz.isPrimitive() and true otherwise (i.e., flip the assignment around the
clazz.isPrimitive() check) so the field nonprimitivesAreRelevant correctly
reflects that non-primitives are relevant; apply the same fix to the other
occurrence updating nonprimitivesAreRelevantTemp at the second site (the other
clazz.isPrimitive() branch).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
annotation-file-utilities/src/test/resources/annotations/tests/classfile/all-annotations.jaifframework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.javaframework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java
💤 Files with no reviewable changes (1)
- annotation-file-utilities/src/test/resources/annotations/tests/classfile/all-annotations.jaif
| Node parentNode = anno.getParentNode().get(); | ||
|
|
||
| if (parentNode instanceof ArrayType) { | ||
| return gatf.arraysAreRelevant(); | ||
| } | ||
| if (parentNode instanceof ClassOrInterfaceType) { | ||
| ClassOrInterfaceType classType = (ClassOrInterfaceType) parentNode; | ||
| String simpleName = classType.getName().toString(); | ||
| String scopedName = classType.getNameWithScope(); | ||
| // TODO: Do I need to remove type parameters? | ||
| return gatf.isRelevant(simpleName) || gatf.isRelevant(scopedName); | ||
| } | ||
| if (parentNode instanceof IntersectionType) { | ||
| return true; // TODO | ||
| } | ||
| if (parentNode instanceof Parameter) { | ||
| Parameter param = (Parameter) parentNode; | ||
| if (param.isVarArgs()) { | ||
| return gatf.arraysAreRelevant(); | ||
| } else { | ||
| throw new Error("Unexpected type annotation on non-varargs Parameter: " + param); | ||
| } | ||
| } | ||
| if (parentNode instanceof PrimitiveType) { | ||
| return gatf.isRelevant(parentNode.toString()); | ||
| } | ||
| if (parentNode instanceof UnionType) { | ||
| return true; // TODO | ||
| } | ||
| if (parentNode instanceof VarType) { | ||
| return gatf.nonprimitivesAreRelevant(); | ||
| } | ||
| if (parentNode instanceof WildcardType) { | ||
| return true; // TODO | ||
| } | ||
|
|
||
| throw new Error("What parent? " + parentNode.getClass().getSimpleName() + " " + parentNode); | ||
| } |
There was a problem hiding this comment.
Avoid hard failures in relevance filtering during AJAVA writing.
At Line 1123 and Line 1159, annotationIsRelevant can throw and abort file generation (anno.getParentNode().get() and throw new Error(...)). This method is documented as conservative; unknown contexts should default to “relevant” instead of crashing.
💡 Suggested fix
- Node parentNode = anno.getParentNode().get();
+ Optional<Node> maybeParentNode = anno.getParentNode();
+ if (!maybeParentNode.isPresent()) {
+ // Be conservative: if context is unknown, keep the annotation.
+ return true;
+ }
+ Node parentNode = maybeParentNode.get();
...
- } else {
- throw new Error("Unexpected type annotation on non-varargs Parameter: " + param);
- }
+ }
+ // Conservative fallback for non-varargs parameter-level placement.
+ return true;
...
- throw new Error("What parent? " + parentNode.getClass().getSimpleName() + " " + parentNode);
+ // Conservative fallback for unhandled JavaParser parent node kinds.
+ return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`
around lines 1123 - 1160, annotationIsRelevant currently calls
anno.getParentNode().get() (which can throw NoSuchElementException) and uses
throw new Error(...) for unknown parent cases; change it to be conservative by
replacing anno.getParentNode().get() with a safe Optional check (e.g.,
anno.getParentNode().orElse(null)) and if parentNode is null return true, change
the Parameter non-varargs branch (currently throwing) to return true instead of
throwing, and replace the final throw new Error(...) with a return true so any
unrecognized parent types are treated as relevant rather than aborting AJAVA
writing.
| if (parentNode instanceof ClassOrInterfaceType) { | ||
| ClassOrInterfaceType classType = (ClassOrInterfaceType) parentNode; | ||
| String simpleName = classType.getName().toString(); | ||
| String scopedName = classType.getNameWithScope(); | ||
| // TODO: Do I need to remove type parameters? | ||
| return gatf.isRelevant(simpleName) || gatf.isRelevant(scopedName); | ||
| } |
There was a problem hiding this comment.
Class/interface relevance check can suppress annotations that are actually relevant.
At Line 1133, relevance is decided by exact name matches only. But checker relevance semantics (via isRelevant(TypeMirror)) include subtype/supertype relationships, so this can return false for valid cases (for example, relevant supertype + annotated subtype), causing dropped annotations.
💡 Suggested fix
if (parentNode instanceof ClassOrInterfaceType) {
ClassOrInterfaceType classType = (ClassOrInterfaceType) parentNode;
String simpleName = classType.getName().toString();
String scopedName = classType.getNameWithScope();
- // TODO: Do I need to remove type parameters?
- return gatf.isRelevant(simpleName) || gatf.isRelevant(scopedName);
+ if (gatf.isRelevant(simpleName) || gatf.isRelevant(scopedName)) {
+ return true;
+ }
+ // Conservative fallback: if non-primitives can be relevant, don't suppress.
+ return gatf.nonprimitivesAreRelevant();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/common/wholeprograminference/WholeProgramInferenceJavaParserStorage.java`
around lines 1128 - 1134, The current ClassOrInterfaceType check in
WholeProgramInferenceJavaParserStorage uses only string name matching
(simpleName/scopedName) against gatf.isRelevant, which misses relevance
determined by subtype/supertype relations; change the logic in the block
handling parentNode instanceof ClassOrInterfaceType (the classType variable) to
resolve the declared type and call the type-aware relevance method (e.g.,
gatf.isRelevant(resolvedType) or gatf.isRelevant(TypeMirror)) instead of string
comparisons—use the JavaParser symbol/resolution API to obtain a
ResolvedReferenceType/TypeMirror for classType, catch resolution exceptions and
fall back to the existing name checks if resolution fails, and ensure you
reference WholeProgramInferenceJavaParserStorage, ClassOrInterfaceType, and gatf
in the fix.
| if (clazz.isPrimitive()) { | ||
| nonprimitivesAreRelevantTemp = true; | ||
| } else { |
There was a problem hiding this comment.
nonprimitivesAreRelevant is computed with inverted logic.
At Line 406-408, the code sets nonprimitivesAreRelevantTemp = true for primitive classes. That contradicts the field’s meaning and causes incorrect filtering outcomes downstream (notably for var/non-primitive contexts).
💡 Suggested fix
- if (clazz.isPrimitive()) {
- nonprimitivesAreRelevantTemp = true;
- } else {
+ if (!clazz.isPrimitive()) {
+ nonprimitivesAreRelevantTemp = true;
int dotIndex = typeString.lastIndexOf('.');
if (dotIndex != -1) {
// It's a fully-qualified name. Add the simple name as well.
// TODO: This might not handle a user writing a nested class like "Map.Entry".
relevantJavaTypeNamesTemp.add(typeString.substring(dotIndex + 1));
}
}Also applies to: 421-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@framework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.java`
around lines 406 - 408, The variable nonprimitivesAreRelevantTemp is being set
with inverted logic when checking clazz.isPrimitive(): instead of setting it
true for primitive classes, set nonprimitivesAreRelevantTemp = false when
clazz.isPrimitive() and true otherwise (i.e., flip the assignment around the
clazz.isPrimitive() check) so the field nonprimitivesAreRelevant correctly
reflects that non-primitives are relevant; apply the same fix to the other
occurrence updating nonprimitivesAreRelevantTemp at the second site (the other
clazz.isPrimitive() branch).
3 participants
No description provided.