Bug 434899

Comment 1 Stephan Herrmann 2014-05-14 19:00:24 EDT

At a cursory look I can see that the problem disappears as soon as you assign the result of assertNonNull() to a variable.

The point is: by ignoring the return, all that is left to analyse by the inference is the '@Nullable T' parameter. With this input and no other participant vetoing we infer 'T' to be @Nullable. Hence applying @NonNull to this type seems to create a contradiction.

Two possible solutions:

(1) Even if the return does not produce any constraints for inference, we still may want to feed the @NonNull hint into inference to prevent the @Nullable hint from winning: with contradictory *hints* we refrain from inferring any nullness of 'T' itself -> no real contradiction created.

(2) Just ignore the inferred contradiction on a return type if the return is unused.

Comment 2 Timo Kinnunen 2014-05-15 14:29:05 EDT

Perhaps there should only be a single annotation (internally): a repeatable @Nullable(boolean). Then each level of indirection types could check against the outermost annotation only and @Nullable(true) @Nullable(false) T and @Nullable(false) @Nullable(true) T would be a valid types. The former would mean "a T that could be made null but wasn't null" and the latter mean "a T that can't be made null but could have already been null".

Motivation: this should be possible:

public class Snippet {
	static <T> T validate(@Nullable T value, T defaultValue) {
		return value != null ? value : defaultValue;
	}
	static String test1(@Nullable String t1, @NonNull String t2) {
		@Nullable String s1 = validate(t1, null);
		@Nullable String s2 = validate(t2, null);
		@NonNull String s3 = validate(t1, "N/A");
		@NonNull String s4 = validate(t2, "N/A");
		return "[" + s1 + " " + s2 + " " + s3 + " " + s4 + "]";
	}
	static String test2(@Nullable String t1, @NonNull String t2) {
		@Nullable String s1 = validate(t1, t1);
		@Nullable String s2 = validate(t2, t1);
		@NonNull String s3 = validate(t1, t2);
		@NonNull String s4 = validate(t2, t2);
		return "[" + s1 + " " + s2 + " " + s3 + " " + s4 + "]";
	}
	public static void main(String[] args) {
		System.out.println("test 1 " + test1("s_1", "s_2"));
		System.out.println("test 2 " + test2("s_1", "s_2"));
		System.out.println("test 1 " + test1(null, "s_2"));
		System.out.println("test 2 " + test2(null, "s_2"));
	}
}

I'll note that the latest Checker Framework doesn't get this example right either :)

Comment 3 Stephan Herrmann 2014-05-15 15:50:21 EDT

The main focus of null type annotations is being explicit about nullness.

Using a few hints here and there to infer null type annotations during type inference is a special bonus brought to you by JDT.

I'm aware that this bonus feature isn't yet perfect. The thing that needs fixing ASAP is: preventing compile failure due to this inference.

Situations where null type annotation inference is shyer than necessary are not a high priority (yet).

BTW: In terms of self-explaining code I'd actually recommend to split your method validate into two, because it's not trivial to see, what exactly it's guarantees should be.

Comment 4 Timo Kinnunen 2014-05-15 18:00:36 EDT

(In reply to comment #3)
> BTW: In terms of self-explaining code I'd actually recommend to split your
> method validate into two, because it's not trivial to see, what exactly it's
> guarantees should be.

Oh come on :) It's about the simplest method possible and what it does in terms of types is easy to explain as well: it peels away one layer of @Nullable. But OK, how about this:

	public static <T> T validate(@Nullable T value, T defaultValue) { return internalValidate(value, defaultValue); }
	static <T> T internalValidate(@Nullable T value, T defaultValue) { return value != null ? value : defaultValue; }

Now there's a warning about a free type variable T. Or how about this addition, which I think should create an arbitrary length @Nullable @Nullable @Nullable [...] T:

	static <T> T validateN(@Nullable T value, T defaultValue, int levels) { return validate(levels > 0 ? validateN(value, null, levels - 1) : value, defaultValue); }

It very much feels like @Nullable @Nullable @NonNull T should be a valid intermediate type or at least there should be a conversion between @Nullable T and @Nullable @NonNull T to make the types fit when needed.

Comment 5 Stephan Herrmann 2014-05-15 18:10:28 EDT

(In reply to Timo Kinnunen from comment #4)
> It very much feels like @Nullable @Nullable @NonNull T should be a valid
> intermediate type or at least there should be a conversion between @Nullable
> T and @Nullable @NonNull T to make the types fit when needed.

Sorry, I have no idea what you are talking about :)

OK, back to facts: each type can only have one of @NonNull or @Nullable (just like: null is either legal or illegal). Everything else is fiction ...

Comment 6 Tom van den Berge 2014-05-19 04:01:56 EDT

I noticed that the target milestone is set to 4.5. If I understand it correctly, this is going to be released in June 1015? Does this mean that I should not expect a fix for this bug within a year? That would be extremely disappointing.

This bug is one of the reasons that most of my projects don't compile when I upgrade them from Java 7 to 8. I'm heavily using non-null annotations, so this would mean that I can't use Java 8 for at least another year, which would be quite unacceptable if you ask me.

I really hope you can find a way to fix this bug in 4.3 or 4.4.

Comment 7 Stephan Herrmann 2014-05-19 05:22:08 EDT

I'd like to fix this ASAP, too.

Some dealines have already passed with more urgent stuff haven priority. 4.3 is EOL, 1015 has passed, too :)

I'll make a quick go at a group of 2-3 bugs in this area tonight (I'll look up the other(s) soon).

Jay, this is a follow-up of the recent changes in null analysis for type variables. I'll try to come up with a patch, under the general disclaimer, that everything is under the guard of the null annotation option.

To avoid confusion with later discussion in this bug: sole basis for a potential fix is the analysis in comment 1.

Comment 8 Stephan Herrmann 2014-05-20 09:35:07 EDT

Created attachment 243294 [details]
proposed patch

This patch implements the strategy (1) from comment 1:

We only have to 'touch' the return type "@NonNull T" to add another nullHint to the InferenceVariable corresponding to <T> ("T#0"). Now we have contradictory nullHints (from argument & return) which lets inference refrain from guessing any null annotations for the naked 'T'.

This is implemented by invoking inference variable substitution on the return type. While visiting the type "@NonNull T" and its substitution "T#0", we record the desired nullHint.

This was missing only for standalone contexts, other contexts will visit the return type anyway during inference.

Comment 9 Stephan Herrmann 2014-05-20 09:36:08 EDT

Full test suite is currently running, but patch should already be ready for review. Thanks.

Comment 10 Stephan Herrmann 2014-05-20 12:22:21 EDT

RunJDTCoreTests was green including this patch.

Comment 11 Jay Arthanareeswaran 2014-05-20 12:58:07 EDT

Patch looks good. Was a feeling a bit delusional about the testcase in comment #2, but in the end, looks alright to me.

Comment 12 Stephan Herrmann 2014-05-20 17:32:31 EDT

(In reply to Jayaprakash Arthanareeswaran from comment #11)
> Patch looks good.

Thanks.

> Was a feeling a bit delusional about the testcase in
> comment #2, but in the end, looks alright to me.

Yea, there's a bit a of wishful thinking in that comment.
For now I focussed on not producing wrong result.
Always finding optimal solutions remains as an exercise for the reader :)

Comment 13 Manoj N Palat 2014-05-20 20:37:26 EDT

patch looks good.

Comment 14 Jay Arthanareeswaran 2014-05-21 13:56:21 EDT

Stephan, In my eagerness to see the bug count go down, I have gone ahead and released this one :)

http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b3e17a909a1b31bd9a62f4bc41b8e8da13b23038

Comment 15 Jay Arthanareeswaran 2014-05-26 05:42:47 EDT

Verified for 4.4 RC2 with build I20140524-1500