Community
Participate
Working Groups
BETA_JAVA8: The following program should report 4 errors, but reports only two: // -------- public class X { class Y { } Y y1 = new @Marker X().new @Marker Y(); Y y2 = new @Marker X().new <String> @Marker Y(); }
Please work with http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=5ea91ed47e2e2277fa878fc631fcabfaf9ef89b3 or later to see the problem properly.
Similarly in this: // --- public class X { X x; class Y { } Y y1 = @Marker x.new @Marker Y(); Y y2 = @Marker x.new <String> @Marker Y(); } The soon to be released tests: test033 and test034 in GrammarCoverageTests308 cover these cases and capture the present buggy ouput, so there is no need to write tests afresh. These can be reused.
(In reply to comment #2) > The soon to be released tests: test033 and test034 in GrammarCoverageTests308 > cover these cases and capture the present buggy ouput, so there is no need to > write tests afresh. These can be reused. Please pull in http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=25d72210a9980d62464f44386c5778b0542e3629
Created attachment 222111 [details] Proposed fix Couple of notes on the patch: I initially thought I would call resolveType on QualifiedAllocationExpression#type. Then looking at the code we already have to resolve the enclosed allocation through the enclosingInstance, I thought it's simpler to just invoke type#resolveAnnotations(). The scenario for anonymous instantiation (i.e. this.enclosingInstance == null ) is already covered with a type.resolveType. Hence the fix has been made only for the case: enclosingInstance != null This patch has revealed out an existing bug: The error highlighting for the second error is not correct in the test NegativeTypeAnnotationTest#test066(). The following code has the same problem too. Z z3 = (@Marker ZZ) null; // Marker exists and ZZ doesn't I also noticed that the block of code (line 339 - 365) in QualifiedAllocationExpression is exactly same as the one in AllocationExpression. I don't know why it wasn't reused.
(In reply to comment #4) > Created attachment 222111 [details] > Proposed fix Attached the wrong patch. This is the correct one.
Created attachment 222112 [details] Updated patch Correct patch
Here are a couple of comments: - Please remove ElementType definition from org.eclipse.jdt.core.tests.compiler.regression.NegativeTypeAnnotationTest.testBug391500() as it is not needed. - Fix in QAE needs to be made in CompletionOnQualifiedAllocationExpression also. The orthodox fix would have been to the resolve annotations for the qualified allocation expression inside resolveTypeEnclosing methods. But the present fix is simple enough and can be released as is with the two changes above. In future, can you please request the review at the bug level and not at the patch level, thanks!
Created attachment 222130 [details] Updated patch Updated patch with the suggestions incorporated. In this patch, I have moved the fix from QAE#resolveType() to STE#resolveTypeEnclosing(). There were two reasons for not doing that earlier: 1. The method name resolveTypeEnclosing didn't appear to me intuitive enough to put the annotation resolution code there. 2. We don't report all the annotations in the following code: public class Bug391500 { X.Y.Z z3 = new @Marker X().new @Marker Y().new @Marker Z(); } private class X { private class Y { private class Z { public Z(){} public Z(int i){} } } } But I think it's okay because the code is already broken and with the qualified type reference not visible, it's alright not to resolve it's annotations. Plus, having the code STE makes sure that other sub classes, including CompletionOnQualifiedAllocationExpression, get the fix too. Srikanth, please let me know if point 2 is something we can live with.
Patch looks good. Please release. (In reply to comment #8) > But I think it's okay because the code is already broken and with the > qualified type reference not visible, it's alright not to resolve it's > annotations. Plus, having the code STE makes sure that other sub classes, > including CompletionOnQualifiedAllocationExpression, get the fix too. > > Srikanth, please let me know if point 2 is something we can live with. Yes. cf behavior on test below: We don't complain on Q and K. // -- public class Bug391500 { XTop.X.Q.K z3 = new XTop().new @Marker X().new @Marker Q().new @Marker K(); } class XTop { private class X { private class Y { private class Z { public Z(){} public Z(int i){} } } } }
(In reply to comment #8) > 1. The method name resolveTypeEnclosing didn't appear to me intuitive enough > to put the annotation resolution code there. Actually this is the right place - I agree the method name is messed up. It is not actually resolving the "Enclosing" type at all, it is resolving the enclosed type *in* the Enclosing context. I would leave the name as is though.
Released the fix, which can be seen here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=6e5d8fb1fa59c43c60c58828e1e292aaafa3465f
