Community
Participate
Working Groups
Created attachment 236249 [details] Test case to reproduce the issue Outer1. @Marker1 Inner is represented as a SimpleType of QualifiedName where the anotations sit on SimpleType, while QN includes Outer1 and Inner but not the annotation. Need to alter the representation. [follow up of bug 417659]
Please always set the target appropriately so this shows in queries. TIA
PackageQualifiedType's structure would be ideal to represent this kind of scenario. Agreed that the name is a misnomer, but otherwise, this representation would also have qualifier as Name.class, name as name.class and the annotation. Proposal: a) Rename PackageQualifiedType to NameQualifiedType b) add a boolean to isPackage to denote a package (if true). c) and the consequent changes in the astConverter and other code. This would remove the QualifiedName spanning across the annotations. Jay,Markus, Srikanth: Comments welcome.
Markus, do you see any problems with the proposal from comment#2 - would it suffice for your team ?
Note:Enable the following tests after fixing this issue: ASTConverter18Test : test0006 TypeBindingTests308 : test020
*** Bug 424320 has been marked as a duplicate of this bug. ***
I didn't see this bug and was only CC'd today. Bug 404489 comment 4 explains how this should be parsed. I'll update the Javadoc of QualifiedType to clarify this. See bug 424320 for the right solution: If "Outer1" is resolved to a type, then "Outer1" should be a SimpleType and "Outer1. @Marker1 Inner" should be a QualifiedType. Note that this is already correct if you remove the "<Integer>" type argument. The outer ParameterizedType "One<..>" is irrelevant for example in comment 0.
Created attachment 238523 [details] Hypothetical Javadoc for QualifiedType explaining NameQualifiedType BTW: I almost got a heart attack when I saw comment 2 and initially thought we really can't do without a NameQualifiedType. I wrote a hypothetical Javadoc for QualifiedType that would overthrow the decision from bug 404489 comment 4 and use NameQualifiedType for both of these cases: a.b.@A X // a.b is a package a.X.@A Y // a.X is a type The rules for ASTParser should result in a single canonical representation for all source forms regardless of whether bindings are available or not. The rules are designed to meet the original goal that Java 7 source code is parsed into the same ASTNodes as for JLS4: Since SimpleType(QualifiedName) is preferred, a NameQualifiedType can only show up if there's a type annotation on the last SimpleName -- and that never happens before JLS8. I think we could go this way, but the current PackageQualifiedType solution is also fine for me. However, if we really want to change directions, then this should be decided quickly.
Created attachment 238576 [details] Proposed Patch (In reply to Markus Keller from comment #7 > I wrote a hypothetical Javadoc for QualifiedType that would overthrow the > decision from bug 404489 comment 4 and use NameQualifiedType for both of > these cases: > > a.b.@A X // a.b is a package > a.X.@A Y // a.X is a type > > The rules for ASTParser should result in a single canonical representation > for all source forms regardless of whether bindings are available or not. Markus: Thanks for the comments and the sample doc - The solution proposed in the attached patch implements the NameQualifiedType as per the above comment, ie the above cases would result in a NQT regardless of binding. > > is preferred, a NameQualifiedType can only show up if there's a type > annotation on the last SimpleName -- and that never happens before JLS8. Yes. SimpleType-QualifiedName structure retained for <JLS8. > I think we could go this way, but the current PackageQualifiedType solution > is also fine for me. Markus: Which solution did you mean by the "current PQT solution"? With the above patch the PQT is replaced by NameQT (which is in effect only a name change - sans binding - for current scenarios that implement PQT) and hence I believe this should be fine. Let me know otherwise. Noopur/UI team: Once we have this solution ready for commit, references to PQT need to be updated - Will raise a bug against ui once we have this patch ready and would have to commit in tandem.
(In reply to Manoj Palat from comment #8) > > I think we could go this way, but the current PackageQualifiedType solution > > is also fine for me. > Markus: Which solution did you mean by the "current PQT solution"? The current solution is the solution as specified in BETA_JAVA8 (with clarified Javadoc of QualifiedType: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=09f42f58bcdf2fa84ccd2b8b65f28ab3036222ca ) ... and as explained in comment 6: > See bug 424320 for the right solution: If "Outer1" is resolved to a type, > then "Outer1" should be a SimpleType and "Outer1. @Marker1 Inner" should be > a QualifiedType. Currently, a PackageQualifiedType is only to be used if the qualifier resolves to a package (whose segments cannot contain type annotations in legal Java 8). Otherwise, a QualifiedType is used.
(In reply to Manoj Palat from comment #8) > Created attachment 238576 [details] [diff] > Proposed Patch This is great, thanks! I've played around with the patch and everything looked good in the ASTView. It solves the known bugs and reduces complexity by using the same AST structure for resolved and unresolved ASTs. Recovery for JLS4 ASTs is still fine and the AST structure doesn't change for pure Java 7 code. I think we should release this change. Nits: - Block comment in ASTConverter#createBaseType(..) can be removed - Types like ASTVisitor and ASTMatcher that declare methods for each concrete ASTNode type should stay sorted. I.e. move method "visit(NameQualifiedType)" to between "visit(Modifier)" and "visit(NormalAnnotation)", etc.
Thanks Markus for the comments. Incorporated the same. Raised bug 424920 against ui as a followup for associated changes. committed as per http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=25dbd9e17c7b6920dccf1b458296dbbe00f382de
Fixed order of more NameQualifiedType-related methods with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=23f99b8ee2d416cf9b0622bf3ddf2ea4961ce8f4 The original fix produced bad source ranges for new java.util.HashMap<>(); Fixed with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=1a9cac53d4ac29d3f80117ac2f0972315a7c6461 This fix should also work fine for (currently) illegal constructs like this: some.pack.Outer<>.Inner<> i; Manoj, please add a regression test for the qualified diamond case.
(In reply to Markus Keller from comment #12) > Fixed order of more NameQualifiedType-related methods with > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=23f99b8ee2d416cf9b0622bf3ddf2ea4961ce8f4 > > > The original fix produced bad source ranges for > > new java.util.HashMap<>(); > > Fixed with > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/ > ?id=1a9cac53d4ac29d3f80117ac2f0972315a7c6461 > > This fix should also work fine for (currently) illegal constructs like this: > > some.pack.Outer<>.Inner<> i; > > Manoj, please add a regression test for the qualified diamond case. Thanks Markus for the additional testing.. Committed via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=c2c89873c61d71348f0b43a177f550bf104965a7 uncovered bug 424977 in the process. Note: To enable_testBug41897_002() once bug 424977 is fixed.
(In reply to Manoj Palat from comment #13) > uncovered bug 424977 in the process. > Note: To enable_testBug41897_002() once bug 424977 is fixed. Ref bug 424977 comment 2 to enable the test.
