Comment 1 Srikanth Sankaran 2011-12-08 06:16:53 EST

This problem has been there for a long time and is not a regression.
I could reproduce this on 3.6.2 and looking at the test case,
I am sure it dates back even longer.

Basically the test case is so broken that we have contradictory
state in our bindings. We should recover rather than crash in these
situations.

org.eclipse.User.User(@NonNull String name, int uid, @Nullable String
email) appears as a method binding, but also has a local variable.

The null annotations are red herrings. They may as well have been
@Blah and @BlahBlah to have reproduced this problem.

As this is not a regression, I think we can defer this to 3.8 M5.
Dani, do you agree ?

Comment 2 Srikanth Sankaran 2011-12-08 06:20:13 EST

Version: 3.7.0
Build id: I20110613-1736


against the following test case I see the same problem:


public class Snippet {
    public void foo(@Blah Object o1) {        
System.out.println(o1.toString()); // OK: o1 cannot be null     }         
@Blah Object bar(@BlahBlah String s1) {         foo(null); // cannot pass
null argument         @Blah String s= null; // cannot assign null value     
    @Blah String t= s1; // cannot assign potentially null value         
return null; // cannot return null value     }
}

    org.eclipse.User.User(@Blah String name, int uid, @BlahBlah String
email)

// end of source

Comment 3 Dani Megert 2011-12-08 06:24:54 EST

> As this is not a regression, I think we can defer this to 3.8 M5.
> Dani, do you agree ?

Yes, I agree. Thanks for the quick investigation.

Comment 4 Markus Keller 2011-12-08 06:33:59 EST

Stripped-down example that also throws a CCE in 3.6.2 (at least one annotation seems to be necessary to reproduce):

package snippet;

public class Snippet {
    void foo(Object o1) {
    }
org.User(@Bla String a)

Comment 5 Srikanth Sankaran 2011-12-08 06:38:35 EST

Stephan, please see if we can recover gracefully from this situation.

It is not connected to null annotations per se, but since it was discovered
while testing your functionality :) you can have the privilege.

Comment 6 Stephan Herrmann 2011-12-08 08:03:00 EST

(In reply to comment #5)
> Stephan, please see if we can recover gracefully from this situation.
> 
> It is not connected to null annotations per se, but since it was discovered
> while testing your functionality :) you can have the privilege.

investigating

Comment 7 Stephan Herrmann 2011-12-08 08:25:13 EST

The bug is deep inside the parser, which for this particular case fails to
create a valid AST, meaning we get a non-tree structure: during syntax
recovery the same (Marker)Annotation instance is recorded both for the method
and the argument.

Subsequent phases have no chance to correctly set the recipient for this
annotation, as it must serve two masters.

It seems we need a way to tell the RecoveredType that a pendingAnnotation
has been consumed from consumeFormalParameter(). Currently there is no
connection at all in this direction.

When implementing this in consumeFormalParameter() we can check whether
arg.annotations and ((RecoveredType)currentElement).pendingAnnotations 
share common Annotation instances, and if so, mark that these have been 
taken care of and no longer should be considered "pending".

Not sure if other locations could produce the same problem.
The ambiguity is initially created when during recovery an annotation
is both pushed on the expression stack *and* recoreded in pendingAnnotations
(via a RecoveredAnnotation). So anybody consuming either side should
probably inform the other, too.

Comment 8 Stephan Herrmann 2011-12-13 19:24:21 EST

Created attachment 208361 [details]
snapshot of test & proposed fix

Here's a snapshot of a patch that will fix the root cause.
For the particular situation it does its job. I'm only concerned about
similar locations needing a similar fix as consumeFormalParameter().
consumeCatchFormalParameter() is an obvious candidate but I find it
tremendously difficult to create a test that reproduces the same
situation for other syntactic elements.

Comment 9 Ayushman Jain 2011-12-14 05:30:45 EST

Created attachment 208373 [details]
complete fix + tests

A few issues with the previous patch:
1. The index in annotationsConsumed(..) for if (consumedAnnotations[i] == pendingAnnotationAST) should be 'j' and not 'i'. The previous patch throws CCE with this- @Blah org.User(@Bla String str){}. Fixed in new patch.

2. The fix was also needed in other implementations of the consumeFormalParameter(..) method. A case in point:
org.User(@Bla String... str) - try code select on str: CCE thrown
Fixed in new patch.

3. Added ap parser test to make sure that the annoatation @Bla appears only on the parameter and not the method.

Stephan, please see if this patch is ok and release. Btw, I think tests added in NullAnnotationTest should be moved to a more general place, such as AnnotationTest.java

Comment 10 Ayushman Jain 2011-12-14 06:13:58 EST

(In reply to comment #9)
> Created attachment 208373 [details]
> complete fix + tests
All tests pass with this patch.

Comment 11 Stephan Herrmann 2011-12-15 09:31:11 EST

(In reply to comment #9)
> Created attachment 208373 [details]
> complete fix + tests
> 
> A few issues with the previous patch:
> 1. The index in annotationsConsumed(..) for if (consumedAnnotations[i] ==
> pendingAnnotationAST) should be 'j' and not 'i'. The previous patch throws CCE
> with this- @Blah org.User(@Bla String str){}. Fixed in new patch.

Thanks!
Silly beginner's mistake... good I marked the patch as a snapshot. 

> Stephan, please see if this patch is ok and release. Btw, I think tests added
> in NullAnnotationTest should be moved to a more general place, such as
> AnnotationTest.java

Done.


(In reply to comment #8)
> [...]. I'm only concerned about
> similar locations needing a similar fix as consumeFormalParameter().
> consumeCatchFormalParameter() is an obvious candidate but I find it
> tremendously difficult to create a test that reproduces the same
> situation for other syntactic elements.

Update: running all JDT/Core tests with a conditional breakpoint yielded
the following candidate:
CompletionParserTest.testMB_1FHSLMQ_2()
This one test enters consumeCatchFormalParameter() with currentElement set.
From this test it could be possible to construct a witness for a related
problem in consumeCatchFormalParameter(). However, now these bits don't fit:
- run as a completion test the CompletionParser sets 
  annotationRecoveryActivated to false => no pendingAnnotations recorded
- run as a compiler test, annotationRecoveryActivated is true but now
  statementRecoveryActivated is false and thus we bail out on error.
=> no success reproducing the exception against a catch formal param.

I guess at this point I should just release what we already have (with
some minor polish) unless s.o. has a quick pointer how to get a parser
with both mentioned flags set to true.

Comment 12 Ayushman Jain 2011-12-15 10:19:46 EST

(In reply to comment #11)
> I guess at this point I should just release what we already have (with
> some minor polish) unless s.o. has a quick pointer how to get a parser
> with both mentioned flags set to true.

+1 to release what we have right now. Srikanth, any objections?

Comment 13 Stephan Herrmann 2011-12-15 17:06:06 EST

Patch from comment 9 has been polished, retested and pushed as
http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=44a65692ed8ea84bd05a0826234a3cf62365dd1a

Comment 14 Srikanth Sankaran 2011-12-15 21:23:27 EST

(In reply to comment #12)
> (In reply to comment #11)
> > I guess at this point I should just release what we already have (with
> > some minor polish) unless s.o. has a quick pointer how to get a parser
> > with both mentioned flags set to true.
> 
> +1 to release what we have right now. Srikanth, any objections?

No, this plan sounds, good.

Comment 15 Srikanth Sankaran 2012-01-23 04:46:48 EST

Verified for 3.8 M5 using build id: I20120122-2000