Bug 433478

Comment 1 shankha banerjee 2014-04-24 20:15:13 EDT

Hi Dobes,
I could not see with the bug latest code. If possible could you please check
if you see the bug with the latest build. I will check with previous builds and
try to figure the root cause. 

Thanks

Comment 2 shankha banerjee 2014-04-24 20:22:17 EDT

(In reply to shankha banerjee from comment #1)
> Hi Dobes,
> I could not see with the bug latest code. If possible could you please check
> if you see the bug with the latest build. I will check with previous builds
> and
> try to figure the root cause. 
> 
> Thanks

I meant the bug was not reproducible with the latest code.

Comment 3 shankha banerjee 2014-04-25 01:51:26 EDT

Hi Dobes,
Please ignore comment 1.

Able to reproduce on Tip.

To reproduce the issue faster, don't import

functionalJava project. 

It takes a long time to build.

Just import banjo-lang and you would able to see the error.

Thanks.

Comment 4 shankha banerjee 2014-04-25 07:52:49 EDT

We are not able to determine the return type of the inherited method to which the current method is matched.

The inherited type is from:

org.eclipse.jdt.internal.compiler.lookup.MethodVerifier15.checkMethods(MethodVerifier15.java:586)

checkAgainstInheritedMethods(currentMethod, matchingInherited, index + 1, nonMatchingInherited); // pass in the length of matching

matchingInherited -> [<no type> operator(@NonNull FileRange, @NonNull String) ]

---

Will continue investigating.

Thanks

Comment 5 Srikanth Sankaran 2014-04-28 02:32:44 EDT

Tentatively targetting 4.4

Comment 6 Stephan Herrmann 2014-04-28 11:06:37 EDT

(In reply to shankha banerjee from comment #4)
> We are not able to determine the return type of the inherited method to
> which the current method is matched.
> ...
> Will continue investigating.

Question would be: if we're not able to find the return type, how come it is not set to MissingTypeBinding nor ProblemBinding?

Comment 7 shankha banerjee 2014-05-04 04:50:27 EDT

(In reply to Stephan Herrmann from comment #6)
> Question would be: if we're not able to find the return type, how come it is
> not set to MissingTypeBinding nor ProblemBinding?

Hi Stephan,
Sorry I was not clear in my comment. 

The binding is ProblemMethodBinding. The return type is not set.

The reason is:
SourceTypeBinding(ReferenceBinding).isCompatibleWith(TypeBinding, Scope) line: 1238	

return one.returnType.isCompatibleWith(two.returnType);

Now here "two" is ProblemMethodBinding. The field two.returnType is not set
and therefore leads to NPE down the way.

"two" -> <no type> operator(@NonNull FileRange, @NonNull String)


Thanks

Comment 8 Stephan Herrmann 2014-05-04 05:15:59 EDT

(In reply to shankha banerjee from comment #7)
> (In reply to Stephan Herrmann from comment #6)
> > Question would be: if we're not able to find the return type, how come it is
> > not set to MissingTypeBinding nor ProblemBinding?
> 
> Hi Stephan,
> Sorry I was not clear in my comment. 

Now you get me really confused: 
 
> The binding is ProblemMethodBinding. The return type is not set.
> 
> The reason is:
> SourceTypeBinding(ReferenceBinding).isCompatibleWith(TypeBinding, Scope)
> line: 1238	

That's one past the line of the NPE? So it will never be reached and cannot be the reason for anything. What are you telling me?
 
> return one.returnType.isCompatibleWith(two.returnType);

That's the line in MethodVerifier? Your comment reads as if I should find this line in SourceTypeBinding or ReferenceBinding.
 
> Now here "two" is ProblemMethodBinding. The field two.returnType is not set
> and therefore leads to NPE down the way.

OK, we get one step closer to the root cause, but that's not enough. What's wrong here? Is it wrong that a ProblemMethodBinding has null returnType? Is it wrong that any code accesses MethodBinding.returnType without a null check (I see lots of places like that)? Is it wrong that MethodVerifier operates with a ProblemMethodBinding? Is it wrong that we have a ProblemMethodBinding for this method? What kind of problem (ProblemReason) is it anyway?

Those questions you should ask yourself and more like that ...

Comment 9 shankha banerjee 2014-05-09 04:24:18 EDT

Created attachment 242882 [details]
Patch

All tests (along with the newly added test) are green & the original project attached in the bug compiles without any NPE.

--- Issue ---

NullAnnotationMatching.java#checkForContraditions
Line No: 393 
if (scope == null)
-->   return new ProblemMethodBinding(method, method.selector, method.parameters, ProblemReasons.ContradictoryNullAnnotations);

A ProblemMethodBinding (PMD) is created and we do not have a return type for the new object (PMD).

Please have a look at the first four methods of the call stack:

org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding.isCompatibleWith(ReferenceBinding.java:1237)
org.eclipse.jdt.internal.compiler.lookup.TypeBinding.isCompatibleWith(TypeBinding.java:590)
org.eclipse.jdt.internal.compiler.lookup.MethodVerifier.areReturnTypesCompatible(MethodVerifier.java:91)
org.eclipse.jdt.internal.compiler.lookup.MethodVerifier.areReturnTypesCompatible(MethodVerifier.java:80
org.eclipse.jdt.internal.compiler.lookup.MethodVerifier.checkAgainstInheritedMethods(MethodVerifier.java:163)

MethodVerifier.java#checkAgainstInheritedMethods
if (!areReturnTypesCompatible(currentMethod, inheritedMethod))

The return type of the original method and inherited method are compared.

If the inheritedMethod return type is not set, it leads to a crash at:
ReferenceBinding.java#isCompatibleWith:1237: 

if (otherType.id == TypeIds.T_JavaLangObject)

--- Solution ---

1) One possible solution is to put null checks. There are many places where
we crash because of not setting the return type (I counted 6, with this example).

So in my opinion it is not feasible to put null checks everywhere.

2) Set the return type of the object of type PMD.
This is what I followed in this patch.

--- Patch ---

1) NullAnnotationMatching#checkForContraditions signature has been changed.
   This let us pass the inherited method return type.

   Why?

   The returnType value should match the value when everything is good. i.e.
   where we "do" not need to create a PMD. The value of    
   inheritedMethod.returnType in  

MethodVerification#checkAgainstInheritedMethods
if (!areReturnTypesCompatible(currentMethod, inheritedMethod)

as seen in both the cases (PMD case and non PMD case) should be the same.

2) Although I do not think the changes are necessary for 
NullAnnotationMatching#checkForContraditions
Line 405:
if (scope == null)
return new ProblemMethodBinding(method, method.selector, method.parameters, ProblemReasons.ContradictoryNullAnnotations)

as there is no Contradiction between the return types of the original and inherited method, it is good to have the return type set.

This can be removed if there is a different opinion.

3) NullAnnotationMatching#checkForContraditions

is called from 5 places. I have modified 2 of them and left the other 3 out as
I could not figure out a way when they would be hit and we would need the extra
parameter (return type) introduced. So I have retained there original behavior.

--- Test Case ---

1) I have added a test case testBug433478 in NullTypeAnnotationTest.java.

The test case does lead to a NPE without the fix. The call stack is different from the above bug but the underlying reason is the same.

2) I have not been able to get a small test case which leads to the exact same
call stack as the one mentioned in the bug.

I will keep looking into it.

Thanks

Comment 10 shankha banerjee 2014-05-09 04:34:15 EDT

Created attachment 242883 [details]
Patch

I had to remove a unnecessary comment from previous patch.
Please have a look at Comment 9. It is valid for this patch.
There are no other changes.

Thanks

Comment 11 shankha banerjee 2014-05-11 23:21:15 EDT

Not related to the present bug. Some of the errors that show up after compilation may be related to Bug 434602.

Comment 12 Stephan Herrmann 2014-05-13 06:49:59 EDT

(In reply to shankha banerjee from comment #9)
> --- Patch ---
> 
> 1) NullAnnotationMatching#checkForContraditions signature has been changed.
>    This let us pass the inherited method return type.
> 
>    Why?
> 
>    The returnType value should match the value when everything is good. i.e.
>    where we "do" not need to create a PMD. The value of    
>    inheritedMethod.returnType in  
> 
> MethodVerification#checkAgainstInheritedMethods
> if (!areReturnTypesCompatible(currentMethod, inheritedMethod)
> 
> as seen in both the cases (PMD case and non PMD case) should be the same.

This part is bogus. By using the return type of the inherited method we end up reporting (the test explicitly expects this):

       	public Y foo() {
       	       ^
     The return type is incompatible with I<Y>.foo()

This error is wrong. 

OTOH, the root error (about contradictory null annotations) is not being reported, because we have no scope when detecting.

I'm preparing an alternative patch ... not sure if the missing error will be covered or needs a follow-up bug ...

Comment 13 Stephan Herrmann 2014-05-13 08:52:03 EDT

Created attachment 243022 [details]
minimal patch

This minimal patch seems to suffice.

Jay, if you find a minute, reviewing shouldn't take much time for this one :)

Background: the situation arises because we are detecting a potential conflict between null annotations from the type declaration and its instantiation at odd points in time, basically during on-demand computation of ParameterizedMethodBindings.

At this point we have no source location nor a useful scope for error reporting. I tried to avoid this situation by passing scope and invocationSite via a few more methods, but the way we cache ParameterizedMethodbindings in PTB.methods defeats this attempt.

So if NAM.checkForContraditions (typo to be corrected before release) lacks a site and scope, we have no other option than creating a ProblemMethodBinding. Only if the client requesting the method is an invocation (MessageSend etc.) the client will report invalidMethod() which selects the appropriate error message from the ProblemReason. (Hence, my mentioning of a missing error message in comment 12 was a red herring).

Ergo: the overall strategy is appropriate viz. I don't see a better one (that would, e.g., avoid creating the ProblemMethodBinding in favor of reporting errors directly when detected).

Hence, the only thing needing fixing is indeed the null ProblemMethodBinding.returnType. Seeing that ProblemMethodBinding already initializes other fields from the closestMatch, the simplest solution is just to include returnType in that strategy and we're done.

Comment 14 Jay Arthanareeswaran 2014-05-13 10:12:37 EDT

(In reply to Stephan Herrmann from comment #13)
> Created attachment 243022 [details]
> minimal patch
> 
> This minimal patch seems to suffice.
> 
> Jay, if you find a minute, reviewing shouldn't take much time for this one :)

Thanks for the explanation, the patch looks good to me. Just a reminder that the copyright needs update in ProblemMethodBinding.

Comment 15 Stephan Herrmann 2014-05-13 14:21:23 EDT

Test & fix (incl. copyright update) released for 4.4 RC1 via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=df0c43ea1f2d79898416bb692849429fa09bfbf9

Comment 16 Jay Arthanareeswaran 2014-05-14 06:45:17 EDT

Verified for 4.4 RC1 with build I20140513-2000.