Bug 198085 - Invalid annotation cracks the annotation parsing
Summary: Invalid annotation cracks the annotation parsing
Status: CLOSED DUPLICATE of bug 196200
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.4 M6   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 223838
  Show dependency tree
 
Reported: 2007-07-27 06:55 EDT by Kaloyan Raev CLA
Modified: 2008-05-28 11:02 EDT (History)
6 users (show)

See Also:


Attachments
test case (11.78 KB, application/octet-stream)
2007-07-27 06:58 EDT, Kaloyan Raev CLA
no flags Details
patch (1.72 KB, patch)
2007-08-22 12:40 EDT, Kaloyan Raev CLA
no flags Details | Diff
Proposed fix (1.63 KB, patch)
2007-08-23 21:14 EDT, Olivier Thomann CLA
no flags Details | Diff
Regression test (1.85 KB, patch)
2007-08-23 21:15 EDT, Olivier Thomann CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaloyan Raev CLA 2007-07-27 06:55:09 EDT
If I have the following class definition:

@Deprecated
@Invalid
public class NotWorking {

}

where @Deprecated is a valid annotation and @Invalid is an invalid annotation (not resolved), I would expect that the ITypeBinding.getAnnotations() method returns an array with the @Deprecated annotation. But, in fact, the latter method returns an empty array of annotations. 

If I comment the @Invalid annotation then the ITypeBinding.getAnnotations() method returns correctly an array with the @Deprecated annotation. 

The above example is only for illustration of the problem and is not the real case where the problem has been caught.
Comment 1 Kaloyan Raev CLA 2007-07-27 06:58:12 EDT
Created attachment 74785 [details]
test case

test case that illustrates the problem
Comment 2 Olivier Thomann CLA 2007-07-27 12:17:54 EDT
This is something we need to improve. Right now as soon as one of the annotations cannot be resolved, the returned value is an empty array.
We might need to change this in order to improve the apt tooling.
Walter, this might be one example of the cases we need to improve for apt.
I was thinking of creating a recovered binding in this case.
Comment 3 Olivier Thomann CLA 2007-07-27 12:19:18 EDT
Returning only one annotation is not really an option as there is no way for the user to find out that one annotation could not be resolved.
Comment 4 Walter Harley CLA 2007-07-27 14:51:09 EDT
The related bug is Bug 196200 (which considers both the case of invalid annotations on valid elements, and valid annotations on invalid elements).
Comment 5 Kaloyan Raev CLA 2007-08-22 12:37:41 EDT
As mentioned in comment #2 if one of the annotations is not resolved, then none of the the list of annotations to this TypeBinding is an empty array. In fact, all other valid annotations are not recognized. 

I understand that simply returning an array of only the resolved annotation is not the best solution (comment #3), but this is much better than the current situation. 

Let me give an example in the Java EE context. 
We build a model of an EJB project. The model parses the Java annotations on the EJB classes. Let's have an interface annotated with the @Remote annotation. The model considers this interface as a Remote business interfaces, which is correct. If the user enters an invalid annotation right after the @Remote one, then the @Remote annotations is no more resolved. Now the model considers this interface as a Local business interface, because the Java EE spec says that if an interface is not annotated, then it is a Local one. Well, this situation effectively breaks our model. 
Comment 6 Kaloyan Raev CLA 2007-08-22 12:40:55 EDT
Created attachment 76668 [details]
patch

This patch fixes the TypeBinding.getAnnotations() method. 
Before: if one of the annotations is not resolved then the return value is an empty array. 
Now: the unresolved annotations are ignored and the return value is an array of all resolved annotations. 

This patch effectively solves certain use cases mentioned in the previous comment. 
Please, consider this patch for 3.3.1.
Comment 7 Olivier Thomann CLA 2007-08-22 13:21:50 EDT
Philippe, Jérôme, Kent,

do you see any potential problems by adding resilience in the annotation resolution? We already do something similar for fields and methods where we use availableFields() and availableMethods() which prune all unresolvable methods or fields.
Comment 8 Olivier Thomann CLA 2007-08-22 13:23:19 EDT
Martin, you might want to add a comment here.
Comment 9 Walter Harley CLA 2007-08-22 16:19:38 EDT
To comply with the JSR269 spec we will need to do a bit more than Kaloyan's patch; we actually need to provide at least partial information on the unresolved annotations, i.e., we need to represent them with a token that is the equivalent to what would exist if an empty annotation type of the same name were resolved.

So, Kaloyan's patch might be appropriate and good enough for 3.3.1 but probably not enough for 3.4.
Comment 10 Martin Aeschlimann CLA 2007-08-23 03:56:53 EDT
Maybe we can use recovered bindings here? It would be better if the binding array size has the same size as the number of annotations.
Comment 11 Jerome Lanneluc CLA 2007-08-23 09:16:47 EDT
For 3.3.1, I think the patch is good enough. The spec says that it should return the list of *resolved* annotations. So there is no guaranty that if an annotation cannot be resolved, it should be in the list. So no client should rely on the fact that the number of returned annotation bindings is the same as in the source.

However, if we can use recovered bindings for 3.4, that would be better.
Comment 12 Jerome Lanneluc CLA 2007-08-23 10:41:25 EDT
+1 for the current patch for 3.3.1
Comment 13 Jerome Lanneluc CLA 2007-08-23 10:42:56 EDT
+1 for the current patch for 3.3.1 as a client of IBinding#getAnnotations() has no workaround for this problem.
Comment 14 Olivier Thomann CLA 2007-08-23 21:14:54 EDT
Created attachment 76845 [details]
Proposed fix

This patch doesn't create a list when all annotations can successfully be resolved and is closer to what is done in other places in the dom package.
Comment 15 Olivier Thomann CLA 2007-08-23 21:15:21 EDT
Created attachment 76846 [details]
Regression test
Comment 16 Olivier Thomann CLA 2007-08-23 22:10:55 EDT
Released patch for 3.3.1.
Comment 17 Olivier Thomann CLA 2007-08-23 22:12:27 EDT
Reopen for 3.4
Comment 18 Olivier Thomann CLA 2007-08-24 10:43:11 EDT
Released the patch also in HEAD to keep a consistent behavior between HEAD and 3.3 maintenance stream.
I keep this bug open to improve the support in HEAD.
Comment 19 Olivier Thomann CLA 2008-03-06 11:00:58 EST
Closing as a dup of bug 196200.
With support for bug 196200, you know get all annotations even the ones that cannot be resolved.

*** This bug has been marked as a duplicate of bug 196200 ***
Comment 20 Olivier Thomann CLA 2008-03-06 11:08:30 EST
You can even check that the binding for @Invalid is recovered.
Comment 21 Frederic Fusier CLA 2008-03-25 10:40:38 EDT
I'm not really sure that this bug should have been set as duplicate of 196200. IMO, it should have been set as FIXED while releasing the patch applied onto R3_3_maintenance stream. Then, either add a test case to bug 196200 or open a new bug to track behavior enhancement in this area (type annotations) after the missing types patch was released...

The problem is that the recovered annotation does not work properly. In this case, isRecovered() method returns false although I expected it to be true. And I cannot reopen this bug because it's a duplicate of another one...

To make simple, I consider this one as verified for 3.4M6 using build I20080324-1300, but I'll reopen a new one for the problem of the incorrect isRecovered returned value...
Comment 22 Kaloyan Raev CLA 2008-05-28 11:02:06 EDT
Closing