Community
Participate
Working Groups
Given the following code: package nullEnumSort; import java.util.Collections; import java.util.List; import org.eclipse.jdt.annotation.NonNullByDefault; @NonNullByDefault public class EnumProblem { void f(List<MyEnum> list) { Collections.sort(list); } } ---- package nullEnumSort; import org.eclipse.jdt.annotation.NonNullByDefault; @NonNullByDefault enum MyEnum { x } a bogus warning appears at the "Collections.sort" invocation if EnumProblem is edited and saved (so MyEnum is accessed as BinaryTypeBinding): Null constraint mismatch: The type '@NonNull MyEnum' is not a valid substitute for the type parameter 'T extends Comparable<? super T>' happens since 26442c4da93bb37c148503ede232875b5244e58f for bug 482242
Thanks for catching. I'll have a look for RC1. First guess: perhaps s.t. relating to whether or not the implicit "extends Enum<MyEnum>" should annotate that type argument? In that case bug 482242 would simply bring to light an inconsistency that has always existed?
I'm also seeing this bug several hundred times in production code, since switching to 2020-03 M2. Just saying, so you are aware this is not a corner case, and effectively prohibits upgrading on projects that have null analysis set to severity error. Not sure if it helps you in the analysis: If the enum is moved as nested class into the caller class (instead of being a top level class itself), then the error goes away. However, in real world code that workaround is not possible in most cases.
Observations; In the source case, type "@NonNull MyEnum" has this superclass: - Enum<MyEnum> In the binary case it's: - Enum<@NonNull MyEnum> The problem detected by NAM.analyse has severity UNCHECKED_TO_UNANNOTATED, so *if* a problem is to be reported, then it should be the new info Unsafe null type conversion (type annotations): The value of type 'List<@NonNull MyEnum>' is made accessible using the less-annotated type 'List<MyEnum>' To check whether the problem is justified, let's try to trigger NPE. E.g.: @NonNullByDefault public class EnumProblem { void f(List<MyEnum> list) { MyCollections.sort(list); } } class MyCollections { static <T extends Comparable<? super T>> void sort(List<T> list) { list.add(null); } } Here we succeed to feed 'null' into the list and if subsequently code in f() dereferences list elements without a null check, then we'll see NPE. Question: do we really need to account for code like "list.add(null);"? In a recent blog post [1] I argued our warning is kind-of good enough to alert about this problem in unannotated code, at list.add(null) we warn: Null type mismatch (type annotations): 'null' is not compatible to the free type variable 'T' Attempts to declare a variable of type T and assign null to it, will always lead to a warning at least. I'm ready to believe that the situation is "safe enough". What's missing in bug 482242 and my blog post: consider type variables as type arguments in the receiving method's signature. Currently, I consider only concrete types and wildcards. Things to consider: 1. don't let TVB.nullBoundCheck() report when we see UNCHECKED_TO_UNANNOTATED. -> simple but possibly incomplete 2. fully integrate TVB as type argument in the solution from bug 482242 3. avoid the difference between source and binary case (regarding annotations within parameterized superclass). [1] https://objectteams.wordpress.com/2020/02/06/interfacing-null-safe-code-with-legacy-code/
New Gerrit change created: https://git.eclipse.org/r/157908
(In reply to Eclipse Genie from comment #4) > New Gerrit change created: https://git.eclipse.org/r/157908 This applies option (1) from comment 3, i.e., avoid reporting any new problem (from bug 482242) in this particular situation, thus fixing the regression. I'm aware that today is test day, but as I'll not be able to work on this tomorrow, I prepared the simplest possible fix today, which I'm planning to release today EOD, unless someone objects.
Gerrit change https://git.eclipse.org/r/157908 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=4ce8fc36a787f1398903447a1816ff5fb4392149
(In reply to Eclipse Genie from comment #6) > Gerrit change https://git.eclipse.org/r/157908 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=4ce8fc36a787f1398903447a1816ff5fb4392149 Minimal regression fix released to master for 4.15 M3 Left-over issues have their new home in bug 560296.
Thanks, works!
