Bug 434602

Comment 1 Alex W 2014-05-11 19:13:14 EDT

Created attachment 242950 [details]
Test project with example of error

Comment 2 shankha banerjee 2014-05-11 23:07:24 EDT

Hi Alex,

Thanks for the example. I have shortened the example. I removed the TestNode class, wildcards and some other code in the main class. Please let me know if you agree:

---
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;

class Y {}
@NonNullByDefault
class Z {
	void foo() {
		Z z = new Z();
		z.bar(); // Error: [1]
	}

	public <T extends Y> @Nullable T bar() {
		return null;
	}
}

// [1] : Contradictory null annotations: method was inferred as '@NonNull @Nullable Y bar()', but only one of '@NonNull' and '@Nullable' can be effective at any location

---

I will take a look.

Thanks

Comment 3 Alex W 2014-05-11 23:13:07 EDT

Yep, that gives an error for me! Sorry for the overly long example, I didn't realize that it could be reduced so much.

Comment 4 Stephan Herrmann 2014-05-12 02:35:02 EDT

Quick note: inside a @NonNullByDefault this
  <T extends Y>
is read as
  <T extends @NonNull Y>

That's probably where the @NonNull originates from. However, a @Nullable on the return type 'T' should override that.

Comment 5 Stephan Herrmann 2014-05-20 09:42:09 EDT

Shankha, as you could reproduce this issue, could you please check if the proposed patch in bug 434899 also fixes this issue? I could think so ....

Comment 6 shankha banerjee 2014-05-20 10:31:39 EDT

Hi Stephan,
The issue (Test Example and Comment2) is not solved with the patch mentioned in 
Bug 434899.

The error message remains the same.

Thanks

Comment 7 Stephan Herrmann 2014-05-20 11:04:17 EDT

(In reply to shankha banerjee from comment #6)
> Hi Stephan,
> The issue (Test Example and Comment2) is not solved with the patch mentioned
> in 
> Bug 434899.
> 
> The error message remains the same.

Thanks for trying. I see. While the other bug is about type inference, this one is about combining an implicit @NonNull from the bound with an explicit @Nullable. The explicit annotation should actually override the implicit one.

Comment 8 shankha banerjee 2014-05-22 04:13:05 EDT

Hi Stephan,

Please consider the 2 examples:

--- Example 1---
class Y2 {}
class Y3 extends Y2 {}

class Z2 {
	void foo() {
		Z2 z = new Z2();
		z.bar2(new Y3());
		z.bar();
	}

	public <T extends Y2> T bar(T t) {
  		return null; // [1]
	 }
	
	public <T extends Y2> T bar2(@Nullable T t) {
         	   return t; // [2]
	 }
}

[1]: Null type mismatch (type annotations): 'null' is not compatible to the free type variable 'T' 

[2]: Null type mismatch (type annotations): required 'T' but this expression has type '@Nullable T extends Y2', where 'T' is a free type variable


[1] looks correct, but should we have [2] ?

--- Example 1---

--- Example 2 ---

class Y2 {}
class Y3 extends Y2 {}

class Z2 {
	void foo() {
		Z2 z = new Z2();
		z.bar3(new Y3());
	}
	
	public <T extends Y2> T bar3(T t) {
		return t; // [1]
	}
}

No error/warning gets reported at [1].

Should we report a warning like:

Null type safety (type annotations): The expression needs unchecked conversion to conform to 'T' where 'T' is a free type variable.

Thanks

Comment 9 Stephan Herrmann 2014-05-22 09:20:46 EDT

(In reply to shankha banerjee from comment #8)
> [1]: Null type mismatch (type annotations): 'null' is not compatible to the
> free type variable 'T' 
> 
> [2]: Null type mismatch (type annotations): required 'T' but this expression
> has type '@Nullable T extends Y2', where 'T' is a free type variable
> 
> 
> [1] looks correct, but should we have [2] ?

yes, both messages are good.
 

> --- Example 2 ---
> No error/warning gets reported at [1].
> 
> Should we report a warning like:
> 
> Null type safety (type annotations): The expression needs unchecked
> conversion to conform to 'T' where 'T' is a free type variable.

No.

The difference being:

Example 1: whatever type will be provided for 'T', '@Nullable T' will be its nullable variant and if T itself is nonull, then the nullable variant is not compatible with T.

Example 2: both sides use the unmodified 'T' which is always compatible with itself.


We have another bug somewhere, where the overriding of nullness of a free type variable is not yet handled correctly, but the examples you show in comment 8 all look well.

Comment 10 shankha banerjee 2014-05-23 13:01:12 EDT

Hi Stephan,

One more example: 

class Y {}
class X {
	void foo() {
		X x = new X();
		x.bar();
	}

	public <@Nullable T extends Y> @NonNull T bar() {
		return null; // [1]
	}
}

Shouldn't we report a error here [1]. The return is a NonNull variant of @Nullable T. 

What should be the error [1] if it is one? Should it be:

1) Null type mismatch: required '@NonNull T extends Y' but the provided value is null

or Simply

2)  Contradictory Null Annotations : @NonNull and @Nullable ....

Thanks

Comment 11 shankha banerjee 2014-05-23 13:10:03 EDT

(In reply to Stephan Herrmann from comment #9)
 
> We have another bug somewhere, where the overriding of nullness of a free
> type variable is not yet handled correctly, but the examples you show in
> comment 8 all look well.

Is it recently filed one? I could not find one.

Comment 12 shankha banerjee 2014-05-26 11:30:41 EDT

Created attachment 243496 [details]
Patch

Test results are good.

Hi Stephan,
Could you please review it. I have made the changes at one place. There are few more places where the similar condition can occur. I was not able to figure out test cases for those. e.g.

1) Line No: 683:

this.returnType = Scope.substitute(this, this.returnType);
ParametrizedGenericMethodBinding#inferFromExpectedType

2)
Line No: 124
this.returnType = Scope.substitute(substitution, this.returnType);

ParameterizedMethodBinding#ParameterizedMethodBinding

3) 
Line No: 261
this.returnType = Scope.substitute(substitution, this.returnType);

ParameterizedMethodBinding#ParameterizedMethodBinding

I have not found test cases with respect to these situations. 
Please let me know if you think the present error could appear at these places. 

Thanks

Comment 13 Stephan Herrmann 2014-05-26 18:23:05 EDT

I'd prefer not to infer conflicting null annotations in the first place, rather than trying to repair after the fact. We already have some logic for this avoidance in BoundSet.

Comment 14 shankha banerjee 2014-05-28 07:04:00 EDT

Created attachment 243617 [details]
WIP: Patch

There is WIP Patch.

A new test case other than the one given as part of the bug is also attached.
The new test case exposes issue with setting of return types in ParameterizedMethodBinding.

There is regression in NullTypeAnnotation#testNullTypeInference3c.

Thanks

Comment 15 Stephan Herrmann 2014-05-28 17:18:46 EDT

Seeing your attempts in binding classes: have you traced how BoundSet contributes to the problem? (see comment 13).

Comment 16 shankha banerjee 2014-05-29 07:17:44 EDT

Hi Stephan,
I am looking into BoundSet. In the patch posted in Comment14 I had incorporated the suggestion in Comment 13 (First part)

(In reply to Stephan Herrmann from comment #13)
> I'd prefer not to infer conflicting null annotations in the first place,
> rather than trying to repair after the fact. 

and moved the code to ParameterizedGenericMethodBinding#substitute

and put in code to stop adding conflicting null annotations into substitute.


Line No: 742 : 

return originalVariable.hasTypeAnnotations() ? this.environment.createAnnotatedType(substitute, originalVariable.getTypeAnnotations()) : substitute;

---
BoundSet:

I am looking to 

combineAndUseNullHints
setInstantiation

Thanks,
Shankha

Comment 17 Stephan Herrmann 2014-06-14 17:46:25 EDT

(In reply to Stephan Herrmann from comment #4)
> Quick note: inside a @NonNullByDefault this
>   <T extends Y>
> is read as
>   <T extends @NonNull Y>
> 
> That's probably where the @NonNull originates from. However, a @Nullable on
> the return type 'T' should override that.

Another example where explicitly overriding nullness of a type variable does not work can be found here: https://www.eclipse.org/forums/index.php/mv/msg/781138/1384369/#msg_1384369

Comment 18 Stephan Herrmann 2014-07-12 14:45:55 EDT

To be resolved in coordination with the new umbrella bug.

Comment 19 Stephan Herrmann 2014-07-22 06:23:29 EDT

As I'm about to resolve the umbrella bug 438458 I'm taking this one, too.

Comment 20 Stephan Herrmann 2014-07-22 10:50:09 EDT

Saying that this should be fixed in BoundSet was actually no good advice, sorry.

Using this test from comment 12 (with modifications):

//---
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;

class Y { void doit() {} }
@NonNullByDefault
class X {
   void foo() {
       X x = new X();
       Y y = x.bar(); // Error: Contradictory null annotations before the fix
       y.doit(); // check that @Nullable from bar's declaration has effect on 'y'
   }

   public <T extends Y> @Nullable T bar() {
       return null;
   }
}
//---

The type bound 'extends Y' is affected by @NonNullByDefault, so we must infer T to a nonnull type. It would actually be wrong to drop this information during inference.
Treating the return type '@Nullable T' as the nullable variant of whatever type T represents must apply the same logic as bug 438179 comment 5. In fact the fix for this bug is just a tiny extension of that other fix.
BTW: The fix is mildly similar to the proposal in comment 12, but having the change inside Substitution.substitute(TVB) is much better localized than inspecting all callers of substitute().



However, the fix triggered a regression: Inside TypeVariableBinding.hasRelevantTypeUseNullAnnotations() I observed a type variable 'T' whose declaring element was the class X!! (which triggered AIOOBE because X has not type variables).

This happened, because:
- during initialization of a binary method, its type variables are first created with the declaring class as the (dummy) declaringElement, which is later fixed within the same method.
- an annotated occurrence of the same type variable now created a clone of this half-initialized type variable. This cloned type variable escaped the fixup inside BTB.createMethod().

Fixed by delegating this fixup to TypeSystem, which applies the same fix to all derived types of the same ID (after checking that its a TypeVariableBinding indeed :) 


Fix still needs one final round of testing.

Comment 21 Stephan Herrmann 2014-07-22 12:05:57 EDT

Released for 4.5 M1 via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=b62a0575eabeb814900afcc3426a14b0da5fd6ad

Comment 22 Srikanth Sankaran 2014-08-06 02:13:29 EDT

Verified for 4.5 M1 using Version: Mars (4.5) Build id: I20140804-2000

Comment 23 Tom van den Berge 2014-09-19 07:52:52 EDT

Is there a way to have this fixed in 4.4? It is currently breaking my code, and I'd rather not wait until 4.5 is released.