NullPointer Exception in the AnnotationBinding

[subyssurendran666]

What it does

#2402

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[jarthana]

Please remove this change.

[jarthana]

To add some context to my last review comment, here are couple of experiments I want you to try:

1. Add this line to the end of your testcase
   assertEquals("incorrect number of annotations", 1, annotations.length);
2. Make this small change to your testcase:
   class anothername {} instead of class com{}
   Now if you run your test with class name as anothername, the test will pass. But with class com{}, it will fail.
   Here's what I found. When we have a valid class in the scope with the same name as the qualifier of the annotation, in this case, "com", the annotation resolution returns null. Otherwise, i.e. when the first qualifier (which is considered to be a package) is not found, we create a MissingTypeBinding and not null. But I guess this is irrelevant to this issue.

We should try and exclude the null annotation to the array if possible.

[stephan-herrmann]

2. Make this small change to your testcase:
   class anothername {} instead of class com{}
   Now if you run your test with class name as anothername, the test will pass. But with class com{}, it will fail.
   Here's what I found. When we have a valid class in the scope with the same name as the qualifier of the annotation, in this case, "com", the annotation resolution returns null. Otherwise, i.e. when the first qualifier (which is considered to be a package) is not found, we create a MissingTypeBinding and not null. But I guess this is irrelevant to this issue.

Well, maybe this finding will help us to prevent the null in the first place? Why does one unresolvable name produce null while a similar case produces MissingTypeBinding (which is very much to be preferred!).

[jarthana]

Well, maybe this finding will help us to prevent the null in the first place? Why does one unresolvable name produce null while a similar case produces MissingTypeBinding (which is very much to be preferred!).

Well, I do know why we do that and assumed that's the expected way. In fact, almost copied you on this issue, but didn't bother you. Anyway, since we are here, let me say what's going on. When there's a type in the same CU with the same name (com) as the first qualifier of the type in question (com.Missing), the first call to findTypeOrpackage returns non empty and from there on (and since we don't have any other package/type with that name), we keep the scope as that type and return null. Do you think we should still create a missing type binding?

[stephan-herrmann]

Well, maybe this finding will help us to prevent the null in the first place? Why does one unresolvable name produce null while a similar case produces MissingTypeBinding (which is very much to be preferred!).

Well, I do know why we do that and assumed that's the expected way. In fact, almost copied you on this issue, but didn't bother you. Anyway, since we are here, let me say what's going on. When there's a type in the same CU with the same name (com) as the first qualifier of the type in question (com.Missing), the first call to findTypeOrpackage returns non empty and from there on (and since we don't have any other package/type with that name), we keep the scope as that type and return null. Do you think we should still create a missing type binding?

Can you show me the location of that return null? Is it inside Scope.getTypeOrPackage() (which overload?) or is some caller doing that? Or are you actually looking at a different method?

[jarthana]

Can you show me the location of that return null? Is it inside Scope.getTypeOrPackage() (which overload?) or is some caller doing that? Or are you actually looking at a different method?

Sorry, this fell between cracks. What happens is, we return a SourceTypeBinding from Scope.getTypeOrPackage(char[], int, boolean). This is called from getPackage, line number 3173. Since it's not null, it escapes the null check in next line and then we hit this few lines below:
if (!(binding instanceof PackageBinding)) return null;

We are looking for a qualified name "com.Missing". But getTypeOrPackage() call only gets one segment. So, it is oblivious to the fact that com.Missing is actually from a package "test", which is not part of the qualified name. So, ideally, getTypeOrPackage() should have returned null instead of STB.

[jarthana]

@subyssurendran666 Read the comment on the Scope#getPackage() method:

/* Answer the package from the compoundName or null if it begins with a type.
* Intended to be used while resolving a qualified type name. */

This is what is happening now. But looks like the clients of the getPackage() are not really prepared to receive a null.

[jarthana]

OK, after having spent quite a bit of time, I am more of less convinced that this is by design. The compiler is unaffected. Only the DOM is, which we can mitigate by the null-check. I will go ahead with the original patch from Suby. @subyssurendran666 can you please resolve the conflicts and update the PR?