Community
Participate
Working Groups
BETA_JAVA8: JSR308: section 2.1.1 states: Static member accesses are preceded by a type name, but that type name may not be annotated: @Illegal Outer . StaticNestedClass // illegal! @Illegal Outer . staticField // illegal! It further states: A static member access may itself be a type use, in which case the used type may be annotated by annotating the last component of the static member access, which is the simple name of the type being used. Outer . @Legal StaticNestedClass x = ...; // legal Note however that support for annotations on nested type names is still in the works (bug 383596) At the moment, the following program triggers two errors: // -------------------------------------------- import java.lang.annotation.Target; public class X { static class Y {}; public static void main(String[] args) { Object o = (@Marker X.Y) null; } } @Target(TYPE_USE) @interface Marker { } //---------------------------------------------- (1) The annotation @Marker is disallowed for this location (2) TYPE_USE cannot be resolved to a variable The latter is due to the absence of a JRE that supports TYPE_USE. But once a good JRE becomes available, (1) would go away too and the program would start compiling while it should not.
Jay, please follow up, TIA.
Created attachment 220127 [details] Proposed fix This patch has to be applied on the latest patch from bug 383596. Note that not all cases work or tested since the above mentioned bug is still in progress. package p; public class Bug385137 { static class Y {}; public static void main(String[] args) { Object o = (@Marker @Annot Bug385137. Y) null; // 1. illegal and reported Object o2 = (Bug385137. @Marker Y) null; // 2. legal but reported Object o3 = (@Marker p.Bug385137.Y) null; // 3. Illegal but not reported } } Cases 2 and 3 don't work because the annotations for a QualifiedTypeReference are not stored properly. The failing cases are expected to be addressed once bug 383596 is fixed.
(In reply to comment #2) > Cases 2 and 3 don't work because the annotations for a > QualifiedTypeReference are not stored properly. The failing cases are > expected to be addressed once bug 383596 is fixed. Jay, now that bug 390784 and bug 383596 are resolved, please proceed on this as soon as possible. TIA.
(In reply to comment #2) > package p; > public class Bug385137 { > static class Y {}; > public static void main(String[] args) { > Object o = (@Marker @Annot Bug385137. Y) null; // 1. illegal and reported > Object o2 = (Bug385137. @Marker Y) null; // 2. legal but reported > Object o3 = (@Marker p.Bug385137.Y) null; // 3. Illegal but not > reported > } > } > > Cases 2 and 3 don't work because the annotations for a > QualifiedTypeReference are not stored properly. The failing cases are > expected to be addressed once bug 383596 is fixed. The second case works now after bug 390784 has been fixed. But 3 is not reported. Having said I may have been wrong initially when I said that. I don't even remember why I thought it should be reported. Will check with the spec and confirm.
(In reply to comment #4) > The second case works now after bug 390784 has been fixed. But 3 is not > reported. Having said I may have been wrong initially when I said that. I > don't even remember why I thought it should be reported. Will check with the > spec and confirm. Now, I get it all back. The spec says (2.1.1): "A type annotation appears before the type, as in @NonNull String or @NonNull java.lang.String. A type annotation appears before the package name, if any." But in the third case, i.e. Object o3 = (@Marker p.Bug385137.Y) null; // 3. Illegal but not reported the annotation is stored against the package 'p' and not 'Bug385137'.
(In reply to comment #5) > (In reply to comment #4) > > The second case works now after bug 390784 has been fixed. But 3 is not > > reported. Having said I may have been wrong initially when I said that. I > > don't even remember why I thought it should be reported. Will check with the > > spec and confirm. > > Now, I get it all back. The spec says (2.1.1): > > "A type annotation appears before the type, as in @NonNull String or > @NonNull java.lang.String. A type annotation appears before the package > name, if any." > > But in the third case, i.e. > Object o3 = (@Marker p.Bug385137.Y) null; // 3. Illegal but not reported > > the annotation is stored against the package 'p' and not 'Bug385137'. Yes, this does complicate the logic a bit - At the AST construction stage, we don't whether p is a type or a package. So we view annotations as simply being applied to the component at a certain level of the qualified name. The prefix annotations should be properly seen to apply to the earliest type name. See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=390882.
The proper snippet from comment#0 should be: // ------------------------------ import static java.lang.annotation.ElementType.*; import java.lang.annotation.Target; public class X { static class Y {}; public static void main(String[] args) { Object o = (@Marker X.Y) null; } } @Target(TYPE_USE) @interface Marker { } Note that Javac8b56 compiles this alright - a bug. So we have two things that need fix: (1) Test case above. (2) Test case from comment#2 - case 3, the full test case being: // ------------------- package p; import static java.lang.annotation.ElementType.*; import java.lang.annotation.Target; public class X { static class Y {}; public static void main(String[] args) { Object o = (@Marker @Annot X. Y) null; // 1. illegal and reported Object o2 = (X. @Marker Y) null; // 2. legal but reported Object o3 = (@Marker p.X.Y) null; // 3. Illegal but not reported } } @Target(TYPE_USE) @interface Marker { }
Created attachment 221789 [details] Proposed fix Since the fix for bug 390784 has been made, looks like TypeReference.annotations can be null. The previous patch had to be adjusted after the aforementioned fix. The test require a JDK with JSR 308 support to pass.
(In reply to comment #8) > Created attachment 221789 [details] > Proposed fix > > Since the fix for bug 390784 has been made, looks like > TypeReference.annotations can be null. The previous patch had to be adjusted > after the aforementioned fix. > > The test require a JDK with JSR 308 support to pass. Since at this point we don't yet require any runtime support from JSR335, we should all upgrade to using 8b56 from the 308 branch - Stephan FYI. The runtime support we need for JSR308 itself is tiny - Just the TYPE_USE target element type. I am told this will show up in the lambda branch in a "few weeks."
(In reply to comment #8) > Since the fix for bug 390784 has been made, looks like > TypeReference.annotations can be null. TypeReference.annotations is now an array of arrays. We don't want to allocate this array of arrays unless there are type annotations. That said, I still don't see the connection - It could have been null even earlier - It is not as though we used to signal the absence of annotations with a singleton/sentinel - What did you see earlier when a typeRef didn't have any annotations ?
Please raise the review request flag when you think you are done - TIA.
(In reply to comment #10) > It could have been > null even earlier - It is not as though we used to signal the absence > of annotations with a singleton/sentinel - What did you see earlier > when a typeRef didn't have any annotations ? I think it used to be an empty array. Please note I was working on top of the earlier patch from bug 383596.
Created attachment 221835 [details] Updated patch Same patch with minor formatting changes and updated test. Though it's not completely blocked, the tests have some dependency on the fix for bug 390882. So, it's better be released after bug 390882.
Jay, looking at the message - I feel that we should introduce altogether new messages for this case. These are not syntax errors - these are semantic errors and show we should print a description that calls out why they are illegal.
(In reply to comment #14) > Jay, looking at the message - I feel that we should introduce altogether new > messages for this case. These are not syntax errors - these are semantic > errors > and show we should print a description that calls out why they are illegal. How does this sound - "Type annotations are not allowed on static member access" ?
(In reply to comment #15) > (In reply to comment #14) > > Jay, looking at the message - I feel that we should introduce altogether new > > messages for this case. These are not syntax errors - these are semantic > > errors > > and show we should print a description that calls out why they are illegal. > > How does this sound - "Type annotations are not allowed on static member > access" ? "Type annotations are not allowed on type names used to access static members" ?
(In reply to comment #15) > (In reply to comment #14) > > Jay, looking at the message - I feel that we should introduce altogether new > > messages for this case. These are not syntax errors - these are semantic > > errors > > and show we should print a description that calls out why they are illegal. > > How does this sound - "Type annotations are not allowed on static member > access" ? This message could be construed to imply the annotation below is illegal. Outer . @Legal StaticNestedClass x = ...; // legal
Created attachment 221939 [details] Updated patch Same patch, but the error message is updated as per comment #16 and a new problem type is added and used for the new errors being reported. Note that the new test requires JSR 308 enabled JDK to pass. I intend to disable it before releasing the changes. Srikanth, please review.
(In reply to comment #18) > Created attachment 221939 [details] > Updated patch Please ignore this patch. This also needs to address the new test cases reported in bug 390882, comment #10. This might require a bit of refactoring to minimize code duplication. I am working on a patch.
Created attachment 221967 [details] Updated patch Updated patch. New tests added but require JDK with JSR 308 support to pass.
Created attachment 221985 [details] A few cleanups This is the same patch as earlier, with a few minor clean ups. Most importantly it shows how to write tests that use TYPE_USE that will run fine on both 308 and 335 openJDKs. Jay, I went ahead and modified a few of the old tests to use this technique. Could you please make sure to modify other tests that are disabled that can similarly be amended and restored ? Please release if patch looks alright and remember to post updated list at https://bugs.eclipse.org/bugs/show_bug.cgi?id=383608. TIA.
(In reply to comment #21) > Created attachment 221985 [details] > A few cleanups Thanks, Srikanth, I have released it here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=58de6f137d08c13c9fea4a7115d63c9a6f7786b6
Marking as resolved.
