Bugzilla – Full Text Bug Listing
| Summary: | [null] Wrong warning about unchecked conversion. Constraints lost after using streams. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Marc-André Laperle <malaperle> | ||||||
| Component: | Core | Assignee: | Till Brychcy <register.eclipse> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | Stephan Herrmann <stephan.herrmann> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | alexmonthy, jarthana, manoj.palat, matthew.khouzam, patrick.tasse, register.eclipse, sasikanth.bharadwaj, stephan.herrmann | ||||||
| Version: | 4.6 | Flags: | sasikanth.bharadwaj:
review+
jarthana: review+ |
||||||
| Target Milestone: | 4.6 RC2 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| See Also: |
https://git.eclipse.org/r/69082 https://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=78439d31c46454ce758b8068b8dcf447748481a2 |
||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 493151 | ||||||||
| Attachments: |
|
||||||||
I have noticed another similar problem with recent JDT, and I think it's the same bug.
We often use the pattern:
List<Something> list = somethings.stream()
...
...
.collect(Collectors.toList());
and it was annoying to have to null check the resulting 'list' all the time, so we've added external annotations to Collectors.toList() :
> public static <T> Collector<T, ?, List<T>> toList()
becomes
> public static <T> Collector<T, ?, @NonNull List<T>> toList()
so that the list returned by .collect(Collectors.toList()) would be a "@NonNull List<T>". T remains unannotated of course.
This used to work, but since about a week or two (using I-Builds) the returned List is no longer @NonNull, and we get warnings. It seems the @NonNull is lost when going through the .collect() call.
(In reply to Marc-Andre Laperle from comment #0) Thanks, I can reproduce. The obvious workaround is to change the invocation of getString() like so: .map(trace -> Main.<@Nullable String>getString(String.class)) This does not seem to be a regression, previous versions report the same unchecked conversion. Versions in [4.5M5,4.6M3] additionally report: ---------- 2. ERROR in /tmp/Main.java (at line 10) .map(trace -> Main.getString(String.class)) ^^^^^^^^^^^^ Null type safety (type annotations): The expression of type 'Class<String>' needs unchecked conversion to conform to '@NonNull Class<@NonNull String>' ---------- Explanation: the bound of <T> is implicitly @NonNull (via @NNBD), and only since 4.6M4 do we recognize String.class as having type '@NonNull Class<@NonNull String>' (In reply to Alexandre Montplaisir from comment #1) Would you like to extend this to a reproducing example? (In reply to Stephan Herrmann from comment #2) > (In reply to Marc-Andre Laperle from comment #0) > > Thanks, I can reproduce. > > The obvious workaround is to change the invocation of getString() like so: > > .map(trace -> Main.<@Nullable String>getString(String.class)) I imported Marc-André's project and tried doing this, but I get an error at the '<@Nullable String>' part: Null constraint mismatch: The type '@Nullable String' is not a valid substitute for the type parameter 'T extends @NonNull CharSequence' This is with the latest Integration build (I20160314-0800). > (In reply to Alexandre Montplaisir from comment #1) > Would you like to extend this to a reproducing example? Sure, I will prepare a test project shortly. Created attachment 260289 [details] Project showing the lost annotation to Collectors.toList() I have attached a project that exposes the problem I was mentioning in Comment 1. It contains an external annotation to Collectors.toList(), to return a Collector<T, ?, @NonNull List<T>> instead of Collector<T, ?, List<T>> so that calling .collect(Collectors.toList()) always returns a @NonNull List<T>. (I'm guessing we should not annotate .collect() directly, since a clumsy Collector could return null.) With the latest integration build (as well as M4 and M5) and the attached project, the signatures reported by the tooltips of toList() and collect(), respectively, are: > Collector<@NonNull String, ?, @NonNull List<@NonNull String>> java.util.stream.Collectors.toList() > List<@NonNull String> java.util.stream.Stream.collect(Collector<? super @NonNull String, ?, List<@NonNull String>> collector) We see in the second one that the List is not @NonNull anymore. I was pretty sure it was working previously, and in fact I had to go back to the 4.6 M3 build to get it working correctly: > Collector<@NonNull String, ?, @NonNull List<@NonNull String>> java.util.stream.Collectors.toList() > @NonNull List<@NonNull String> java.util.stream.Stream.collect(Collector<? super @NonNull String, ?, @NonNull List<@NonNull String>> collector) So it was broken for longer than I thought :) Thanks, I'll look into it during M7. New Gerrit change created: https://git.eclipse.org/r/69082 The example with …collect(toList()) is not the same problem as the other one.
Below is a distilled test case:
package test;
import org.eclipse.jdt.annotation.NonNullByDefault;
interface Collector<A, R> {
}
interface Stream {
<A1, R1> R1 collect(Collector<A1, R1> collector);
}
interface List<E> {
}
@NonNullByDefault
public class Test {
public static <T> Collector<?, List<T>> toList() {
return new Collector<Object, List<T>>(){};
}
public static List<String> myMethod(Stream stream) {
List<String> list = stream.collect(toList());
return list;
}
}
ANALYSIS:
This got broken by commit a8001f94dedfcf0ea807d46ff48ed3d0c7a3c2ef for bug 483146 which added the if-condition
if (nullHints != 0 && TypeBinding.equalsEquals(boundI.left, boundJ.left)) {
boundI.nullHints |= nullHints;
boundJ.nullHints |= nullHints;
}
but in this case, boundI.relation == ReductionResult.SAME and boundI.right is the same as boundJ.left:
boundI: Dependency List<T#2>#4 = R1#1
boundJ: TypeBound R1#1 = test.List<java.lang.Object>
PATCH (via gerrit)
changes the if-condition to
if (nullHints != 0) {
if (TypeBinding.equalsEquals(boundI.left, boundJ.left)
|| (boundI.relation == ReductionResult.SAME && TypeBinding.equalsEquals(boundI.right, boundJ.left))
|| (boundJ.relation == ReductionResult.SAME && TypeBinding.equalsEquals(boundI.left, boundJ.right))) {
boundI.nullHints |= nullHints;
boundJ.nullHints |= nullHints;
}
}
I don't have a patch for the original problem reported in this bug, but I have made a distilled test case without lambdas (for easier debugging, so no nested inference)
package test2;
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
@NonNullByDefault
public class TestWithoutLambda {
public @Nullable static <@NonNull A> A getString(Class<A> moduleClass) {
return null;
}
public static <X> X r(X x) {
return x;
}
public static void f() {
@NonNull String s=r(getString(String.class));
}
}
for which
----------
1. WARNING in test2\TestWithoutLambda.java (at line 18)
@NonNull String s=r(getString(String.class));
^^^^^^^^^^^^^^^^^^^^^^^^^^
Null type safety (type annotations): The expression of type 'String' needs unchecked conversion to conform to '@NonNull String'
----------
is reported instead of the expected
----------
1. ERROR in test2\TestWithoutLambda.java (at line 18)
@NonNull String s=r(getString(String.class));
^^^^^^^^^^^^^^^^^^^^^^^^^^
Null type mismatch (type annotations): required '@NonNull String' but this expression has type '@Nullable String'
----------
The problem is, that the @NonNull annotation on A is combined with the @Nullable of in the return type, but the @Nullable should instead supersede it.
Bulk move: too late for 4.6 M7. Very likely has to be moved out of 4.6 entirely. @Stephan, maybe the patch i made for the example with Collectors.toList() could still go into 4.6 RC1 ? This is a regression w.r.t. to Mars and the patch is really small (see comment #7) (In reply to Till Brychcy from comment #10) > @Stephan, maybe the patch i made for the example with Collectors.toList() > could still go into 4.6 RC1 ? This is a regression w.r.t. to Mars and the > patch is really small (see comment #7) In order to focus on this part (and keep the context to your investigation) I created bug 493151 for the original problem. I agree to the new logic when compared to the current version, because #left and #right of a SAME bound can be swapped at will. There may be more involved but that can be handled later (perhaps via bug 493151). I've rebased the change. Good for RC1 from my p.o.v. @Sasi, I'd like to request your +1 for RC1. As Till mentioned this is a regression wrt Mars. (In reply to Stephan Herrmann from comment #13) > Good for RC1 from my p.o.v. > > @Sasi, I'd like to request your +1 for RC1. As Till mentioned this is a > regression wrt Mars. Have we run out of time for RC1? What do you suggest, move to RC2 or to 4.7? One more attempt for RC2 to get sufficient reviews for this regression fix. Please see that we "highjacked" this ticket for the problem in comment 7 and its solution. (In reply to Stephan Herrmann from comment #14) > (In reply to Stephan Herrmann from comment #13) > > Good for RC1 from my p.o.v. > > > > @Sasi, I'd like to request your +1 for RC1. As Till mentioned this is a > > regression wrt Mars. > > Have we run out of time for RC1? > > What do you suggest, move to RC2 or to 4.7? Sorry, never saw this, probably got lost among other emails +1, looks good to me. Just one (probably naive) observation. If left and right of SAME bounds can be swapped, for completeness sake, we should also include right right comparison in the check? (In reply to Sasikanth Bharadwaj from comment #16) > +1, looks good to me. Thanks. > Just one (probably naive) observation. If left and > right of SAME bounds can be swapped, for completeness sake, we should also > include right right comparison in the check? Maybe Till has more on this, but: TypeBound is not fully symmetric in that #left is always an InferenceVariable, whereas #right is an arbitrary TypeBinding. Also see that the left-left check is from bug 483146 and Till only proposed to add the left-right & right-left checks. Anyway, I think it's good to do a minimal fix for the known regression now, and continue our search for a *complete* solution for 4.7 / 4.6.1 - see bug 493151 comment 2. (In reply to Stephan Herrmann from comment #17) > Maybe Till has more on this, but: TypeBound is not fully symmetric in that > #left is always an InferenceVariable, whereas #right is an arbitrary > TypeBinding. Also see that the left-left check is from bug 483146 and Till > only proposed to add the left-right & right-left checks. > > Anyway, I think it's good to do a minimal fix for the known regression now, > and continue our search for a *complete* solution for 4.7 / 4.6.1 - see bug > 493151 comment 2. I agree 100% and have nothing to add. Patch looks good to me. Gerrit change https://git.eclipse.org/r/69082 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=78439d31c46454ce758b8068b8dcf447748481a2 (In reply to Eclipse Genie from comment #20) > Gerrit change https://git.eclipse.org/r/69082 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=78439d31c46454ce758b8068b8dcf447748481a2 > Thanks to all. I rebased and released the change for 4.6 RC2. Verified for Eclipse Neon RC2 Build id: I20160519-1730 |
Created attachment 259931 [details] Sample project Using Eclipse 4.6-I20160223-0800 After using a few stream methods, it seems that the null constraints get lost. For example, in this code: @NonNullByDefault public class Main { public static void main(String[] args, Set<String> set) { Optional<@Nullable String> kam = set.stream() .map(trace -> getString(String.class)) .findFirst(); // Null type safety (type annotations): The expression of type 'Optional<String>' needs unchecked conversion to conform to 'Optional<@Nullable String>' System.out.println(kam); } public @Nullable static <T extends CharSequence> T getString(Class<T> moduleClass) { return null; } } After the map, it should be a stream of @Nullable String but instead it's just String (can be verified with hover). Because of that, findFirst returns an Optional<String> instead of Optional<@Nullable String> I attached a sample project for convenience.