Community
Participate
Working Groups
As part of the fix for bug 391890, ANNOTATIONS_PROPERTY was added to Name nodes. These need to be removed since the specification has changed to disallow constructs of the form: @Marker java.util.List The proper way to annotate these is to say: java.util. @Marker List Thus the presence of annotations before a certain segment of a qualified type signals that what follows is a type and cannot be a name. Very similar to how the presence of type parameters signals a type. See also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c4 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c7 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c33 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c34 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c37 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c39 https://bugs.eclipse.org/bugs/show_bug.cgi?id=391847#c46 When we converty a QualifiedTypeReference or its subtypes, we should convert everything before the first type annotation seen as a SimpleType of QualifiedName/SimpleName and convert everything further as types. Compare how type parameters are handled in conversion of PQTR.
Out of curiosity: (In reply to comment #0) > These need to be removed since the specification has changed to disallow > constructs of the form: > > @Marker java.util.List > > The proper way to annotate these is to say: > > java.util. @Marker List Has the compiler been updated for this spec change? Which bug should I read/follow for that?
(In reply to comment #1) > Out of curiosity: > > (In reply to comment #0) > > These need to be removed since the specification has changed to disallow > > constructs of the form: > > > > @Marker java.util.List > > > > The proper way to annotate these is to say: > > > > java.util. @Marker List > > Has the compiler been updated for this spec change? > Which bug should I read/follow for that? Please see https://bugs.eclipse.org/bugs/show_bug.cgi?id=394356
Created attachment 225127 [details] Proposed Patch - WIP Non-ASTRewrite Part of the patch - WIP TODO: - create another dependent bug for ASTRewrite Part. - Investigate if more test cases are required. - Look for more optimized/reduced code additions in terms of tests and implementation. - Eyeball patch for adherence to code conventions. Note: test0012 of TypeAnnotationsConverterTest (RunDOMTests) is expected to fail until the fix of Bug 392132 is applied as this is a case of intersection of these two bugs.
Created attachment 225202 [details] Proposed Fix - ANNOTATIONS_PROPERTY has been kept for QualifiedName and SimpleName for the sole reason of not breaking the Rewrite part. A separate bug (bug 397421) has been filed for the ASTRewrite part which would take care of the same. Note that all the references of these from the non-ASTRewrite part has been removed in the patch.
Created attachment 225266 [details] Proposed Patch Additional to the code in previous patch, removed the ANNOTATIONS_PROPERTY and associated references in ASTRewrite files as well.
Name.java: (1) The roll back is very partial and leaves many stale things: (a) For example the javadoc still says: "For JLS8, the Name may be preceded by optional type annotations. ..." which is incorrect. (b) BASE_NAME_NODE_SIZE is not bumped down. I think the right thing to do is to restore from commit id bd6803034b95b7e0dd8c0cbcd0aead0a5c726f65 (2) Likewise in QualifiedName: (a) Incorrect statements about JLS8 exist and QualifiedName has no child list properties anymore so the method internalGetChildListProperty does not make sense. The right thing to do is to restore from 34ffedac0864f9670be9b90418a519412edd8a4d (3) Likewise for SimpleName: There are various stale things still left in. The right thing to do is to restore from 699cc351076b7c5897a886d674772d111233f0a3 (4) ASTRewriteAnalyzer: org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteAnalyzer.visit(SimpleName) still references ANNOTATIONS_PROPERTY. (5) ASTRewriteFlattener: org.eclipse.jdt.internal.core.dom.rewrite.ASTRewriteFlattener.visit(SimpleName) still references ANNOTATIONS_PROPERTY. (6) ASTMatcher: org.eclipse.jdt.core.dom.ASTMatcher.match(QualifiedName, Object) What is the difference between the different case arms of the switch ? (7) org.eclipse.jdt.core.dom.ASTMatcher.match(SimpleName, Object) still references annotations() (8) ASTConverter: The method org.eclipse.jdt.core.dom.ASTConverter.annotateName(Name, Annotation[]) is completely redundant now since Names cannot be annotated anymore. All call sites need to be inspected and removed/modified. (9) I'll review the tests and ASTConverter changes deeper after these are addressed and a fresh patch is posted.
Created attachment 225506 [details] Proposed Patch - Reworked as per listed commit diffs. - References to bug number kept intentionally at a couple of places (only) at ASTCoverter and in test files.
Created attachment 225804 [details] Proposed Patch Made patch current to the latest source and addressed the comments so far.
First batch of changes released via: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=b7bb9d2950572af77f85c2c708c3816367817930
Created attachment 226013 [details] Proposed Patch - part 2 Completes the fix.
(1) DBR: the change in getTypeArguments() is not inline with the method name at all. Fortunately, since this method is called at only one place it can be readily renamed into getTypeCount() which is a better name even earlier. (2) ASTConverter18Test: Has lots of irrelevant changes. Please eliminate them.
Created attachment 226019 [details] Proposed Patch - Part 2
Fix and tests released via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=c0d8ddbb4b72e166c7dc81eb807bc47fd1beaa74 Thanks Manoj.
(In reply to Srikanth Sankaran from comment #13) > Fix and tests released via > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?h=BETA_JAVA8&id=c0d8ddbb4b72e166c7dc81eb807bc47fd1beaa74 > > Thanks Manoj. The change in NaiveASTFlattener was wrong. SimpleType doesn't support QualifiedName as name in JLS8, and it shouldn't look like it would be supported. Reverted this change with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=0d3451e9a7c942ed5f2531f8cbfba408096982ef
