Community
Participate
Working Groups
BETA_JAVA8: This program compiles without any errors when built against 308 enabled 8b56: // ----------- import java.lang.annotation.Target; import static java.lang.annotation.ElementType.*; public class X { Object o1 = (@Marker java.lang.Integer) null; // 1. Right. Object o2 = (java. @Marker lang.Integer) null; // 2. Wrong. Object o3 = (java.lang. @Marker Integer) null; // 3. Wrong. } @Target(TYPE_USE) @interface Marker { } // ---------------------------------- 2 is annotating a nested package name - 308 does not allow this. 3 is annotating a top level type - the proper way to this is to use 1 style prefix annotations. Note that 8b50 javac crashes on this test case.
Jay, please take a look.
Created attachment 221787 [details] Proposed fix Fix + test for the test case in comment #0. The tests probably adjusted for the time being so that it will pass with a JDK without JSR 308 support.
Created attachment 221834 [details] Updated patch Some minor changes to the previous patch. Test has been updated as well.
Patch looks good. I recommend that you add two more tests: (a) one with more than one illegal annotation to make sure the source range reported is correct (b) another with just a package name like @Marker java.@lang lang. Just looking at the code, I expected to see an AIOOB for (b), but it is handled well before package binding comes out as null - anyways a test is welcome - it will be missing the new errors, but that is probably ok as it will be a secondary error.
I think the patch is good - but we should consider emitting more specific messages than syntax errors. It can be argued these are not syntax errors at all. Jay, what do you think ?
(In reply to comment #5) > I think the patch is good - but we should consider emitting more > specific messages than syntax errors. It can be argued these are > not syntax errors at all. Jay, what do you think ? Yes, it can be argued. However, I think the spec only talks about type qualifier and not qualifiers in general. In that sense, we can call it a syntax error also, can't we?
Created attachment 221938 [details] Same fix with more tests Patch is same as previous, but includes the two new tests Srikanth suggested. Note that I intend to release with the tests disabled until we get a JDK with both 308 and Lambda support.
(In reply to comment #7) > suggested. Note that I intend to release with the tests disabled until we > get a JDK with both 308 and Lambda support. Please update the list at bug 383608 - we want one list of all disabled tests (i.e not scattered across multiple comments that would need to be scanned) - TIA.
Please also fold the entry at https://bugs.eclipse.org/bugs/show_bug.cgi?id=383608#c5 into the master list currently at https://bugs.eclipse.org/bugs/show_bug.cgi?id=383608#c6. Thanks.
The latest patch fails for qualified generic type references and array of qualified types. Here is the new list of tests including the one mentioned in comment #4. Object o1 = (@Marker java.lang.Integer) null; // 1. Right. Object o2 = (java. @Marker lang.Integer) null; // 2. Wrong. Object o3 = (java.@Marker lang) null; // 3. Wrong. Object o4 = (java.util.@Marker List<String>) null; // 4. Wrong. Object o5 = (java.lang.@Marker Integer[]) null; // 5. Wrong. Object o6 = (java.util.@Marker List<String>[]) null; // 6. Wrong. Object o7 = (java.lang.Integer @Marker []) null; // 7. Right. I will post an updated patch to handle the new cases.(In reply to comment #8) > (In reply to comment #7) > Please update the list at bug 383608 - we want one list of all disabled > tests (i.e not scattered across multiple comments that would need to be > scanned) - TIA. Sure, I was planning to do that after I release the change. At this point, we are still looking at adding more tests.
(In reply to comment #10) > The latest patch fails for qualified generic type references and array of > qualified types. Here is the new list of tests including the one mentioned > in comment #4. Please also tweak these tests, so there is an error reported for annotating at the inner package level. i.e java.@Marker util.@Marker List<String>
Created attachment 221943 [details] Updated patch This patch addresses the test cases that were failing later. I thought I should make the other sub classes (SelectionOnQualifiedTypeReference and such) to use the new code to report the problems. But looks like the selection parser doesn't (yet) care about type annotations - the annotations instance variable is null. Srikanth, should we file a new bug to make the selection parser to include the annotations?
(In reply to comment #12) > Srikanth, should we file a new bug to make the selection parser to include > the annotations? Done: bug 391214.
Patch looks good. Two small suggestions: reportAnnotationsOnPackageQualifiers is better named rejectAnnotationsOnPackageQualifiers (2) Please eliminate the second diff in both ParameterizedQualifiedTypeReference and QualifiedTypeReference. It needlessly leaks the variable i to outside the for loop and is not relevant to the fix any more It is enough to run just RunAllJava8Tests with these changes and release it.
Released the fix in BETA_JAVA8
The commit is here: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=BETA_JAVA8&id=62bc9b56e24288d1e73ff2195f3d44dd920a1f52
(In reply to comment #14) > (2) Please eliminate > the second diff in both ParameterizedQualifiedTypeReference and > QualifiedTypeReference. It needlessly leaks the variable i to outside the > for loop and is not relevant to the fix any more Only one of these seem to fixed. In QualifiedTypeReference, the variable is still leaking outside of the for loop :-(
(In reply to comment #17) > Only one of these seem to fixed. In QualifiedTypeReference, the variable is > still leaking outside of the for loop :-( Oops, looks like I amended the commit with some changes still unstaged :( I have fixed it now.
