Don't write irrelevant annotations in .ajava files #6254
Conversation
| } else if (clazz.isArray()) { | ||
| throw new TypeSystemError( | ||
| "Don't use arrays other than Object[] in @RelevantJavaTypes on " | ||
| + this.getClass().getSimpleName()); | ||
| } 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.
| @@ -59,13 +59,11 @@ public static void testThisInference( | |||
|
|
|||
| public void compute5( | |||
| 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
No description provided.