Open blablubbabc opened 1 month ago
stephan-herrmann
commented
1 month ago Quick question: do all sets of null annotations share the same @Target definition? Seeing checkerframework in the picture, you probably need all annotations to specify a target of TYPE_USE. Is that given?
blablubbabc
commented
1 month ago The external project in question seems to use an older version of the jetbrains annotations (version org.jetbrains:annotations-java5:24.1.0), which don't seem to have the target for TYPE_USE: @Target({ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}).
The current version seems to include that target: https://github.com/JetBrains/java-annotations/blob/master/java-annotations/src/main/java/org/jetbrains/annotations/Nullable.java#L39
Could this explain the issue? Is there any workaround, since I don't have control over the external library.
stephan-herrmann
commented
1 month ago Is there any workaround, since I don't have control over the external library.
This could be difficult to work around, since both kinds of annotations define quite different semantics. It would be like switching compilers half-way through the build :). And indeed JDT derives the choice of semantics from the @Target of the primary set of annotations.
blablubbabc
commented
1 month ago I just tried updating the jetbrains annotations dependency of the external project, building it, and depending on that modified version, and the issue is gone.
I don't know if there is any technical reason to sticking to the older version, but I will propose to update the dependency in the external project. Thank you!
Edit: Seems like they are using the java5 version to avoid some issue with the annotations showing up twice on methods in the javadocs (https://github.com/JetBrains/java-annotations/issues/37). I guess I will have to suppress the null checks for now until this issue has been resolved.
stephan-herrmann
commented
1 month ago Edit: Seems like they are using the java5 version to avoid some issue with the annotations showing up twice on methods in the javadocs (JetBrains/java-annotations#37).
Thanks for sharing. IMHO they should have gone straight from declaration annotations to only TYPE_USE target.
I guess I will have to suppress the null checks for now until this issue has been resolved.
Hopefully this can be done at a fairly narrow scope (using @SuppressWarnings)? You might have to enable "Suppress optional errors with @SuppressWarnings" for this to work.
When thinking about how JDT could help more: should we raise a warning (error?), when secondary null annotations are incompatible with the primary annotations in this way?
blablubbabc
commented
1 month ago When thinking about how JDT could help more: should we raise a warning (error?), when secondary null annotations are incompatible with the primary annotations in this way?
I mean, JDT is able to interpret those annotations in some way if I use them as primary ones. Ideal would be if JDT would be able to detect these secondary Java5 annotations as well and merge the nullability information/hints derived from them into its model about what it expects can be null or nullable. E.g. if the method is annotated with this Java5 Nullable annotation, to interpret this as if the return type was annotated as nullable. If the parameter is annotated, to interpret this as the parameter type is annotated, etc. If there is also a type_use annotation found, this would override these other hints. If there is no nullability information at all, e.g. on type parameters, this could be interpreted as "unknown"/"potentially nullable" as usual, unless there is also a "NonNullByDefault" annotation on the package to define the default interpretation of un-annotated types.
But I am probably missing something that makes this more complicated than it seems to me.
stephan-herrmann
commented
1 month ago But I am probably missing something that makes this more complicated than it seems to me.
One of the simpler questions that would follow: how should it interpret void m(@org.jetbrains.annotations.Nullable String[] a) when primary annotations are TYPE_USE?
a, so the parameter is nullable, i.e., the array (while nothing is said about array elements).And that's just the start. So, frankly I don't see anybody having the leisure to implement this without causing lots of bad surprises.
Back to the warning regarding inconsistent configuration: do you think this is useful?
blablubbabc
commented
1 month ago One of the simpler questions that would follow: how should it interpret
void m(@org.jetbrains.annotations.Nullable String[] a)when primary annotations are TYPE_USE?
Wouldn't it be possible to check the actually used annotation type in each individual case, see if it defines TYPE_USE, and then interpret it accordingly, instead of only considering this aspect for the primary annotation? I.e. if I depend on a mix of external projects, and some are using annotations with TYPE_USE and some are using legacy annotations without TYPE_USE, Eclipse would interpret each annotation case according to the kind of annotation used in this case.
In the example above, if the Java5 jetbrains annotations without TYPE_USE are used, this would mean "the array is not null".
Back to the warning regarding inconsistent configuration: do you think this is useful?
If there is really no solution to make this mix work, then a warning, maybe inside the Eclipse UI where the secondary annotations are defined, could make sense. Similar to the warning that is already used there when the specified annotation is not found.
stephan-herrmann
commented
1 month ago Wouldn't it be possible [...]
possible: yes. affordable: maybe not.
I don't have a minimal reproduction project available currently, but a description of my situation is as follows:
org.jetbrains.annotations.Nullableto annotate their interfaces. Example:org.checkerframework.checker.nullness.qual.Nullableas the primary annotation, but listsorg.jetbrains.annotations.Nullablein the secondary annotations.provided, notcompilein their pom). But this did not make any difference.TabCompleterinterface:Issue: The
@Nullablein my signature is flagged in Eclipse with errorThe return type is incompatible with '@NonNull List<@NonNull String>' returned from TabCompleter.onTabComplete(CommandSender, Command, String, String[]) (mismatching null constraints)Eclipse allows me to mark the method as NonNull, but then nobviously no longer allows the supposedly valid null return value.
Interestingly:
org.jetbrains.annotations.Nullablein my implementation, the error goes away, but I am still not able to use null as return value. Error thenNull type mismatch: required '@NonNull List<@NonNull String>' but the provided value is nullorg.jetbrains.annotations.Nullableand then also use that to annotate my interface implementation, the error is gone and I can use a null return as expected.It behaves as if the secondary Nullable annotations are not recognized.
jdt prefs: