Bugzilla – Full Text Bug Listing
| Summary: | [1.8][compiler] Type annotations on intersection cast types dropped by code generator | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Srikanth Sankaran <srikanth_sankaran> | ||||||
| Component: | Core | Assignee: | Andrew Clement <aclement> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | jarthana | ||||||
| Version: | 4.3 | Flags: | srikanth_sankaran:
review+
|
||||||
| Target Milestone: | BETA J8 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 7 | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 409235 | ||||||||
| Attachments: |
|
||||||||
|
Description
Srikanth Sankaran
Andy, thanks for following up. Created attachment 232296 [details]
Fix for this issue
This patch ensures type annotations on intersection casts make it into the class file. For example:
I i = (@AnnoA I & @AnnoB J)o;
I<String> i = (I<@AnnoA String> & J<@AnnoB String>)o;
The implementation is pretty straightforward, reading/writing the cast target type now makes use of the extra byte for the index of the entry in the intersection cast. Two new testcases added.
Andy, for the test case:
public class X {
public void foo(Object o) {
I i = (@B(1) I & J) o;
J j = (I & @B(2) J) o;
}
}
the code generated is:
public void foo(java.lang.Object o);\n" +
" 0 aload_1 [o]\n" +
" 1 checkcast J [16]\n" +
" 4 checkcast I [18]\n" +
" 7 astore_2 [i]\n" +
" 8 aload_1 [o]\n" +
" 9 checkcast J [16]\n" +
" 12 checkcast I [18]\n" +
" 15 astore_3 [j]\n" +
" 16 return\n" +
while the annotations are:
RuntimeVisibleTypeAnnotations:\n" +
" #27 @B(\n" +
" #28 value=(int) 1 (constant type)\n" +
" target type = 0x47 CAST\n" +
" offset = 1\n" +
" type argument index = 0\n" +
" )\n" +
" #27 @B(\n" +
" #28 value=(int) 2 (constant type)\n" +
" target type = 0x47 CAST\n" +
" offset = 9\n" +
" type argument index = 1\n" +
" )\n"
OIOW, in both instances the offset points to the instruction that does
a checkCast for J. Is this reasonable ? 8b92 also generates confusing code,
so please take a look and tell me what you think.
(In reply to comment #3) Likewise the offset field need to be scrutinized for the test test070b_codeblocks_castWithIntersectionCast also. Andy could you also follow the instructions here to comply with the new CLA rules. Please do for your preferred email address that you want me to use. Otherwise I cannot push your changes under the new regime. http://dev.eclipse.org/mhonarc/lists/eclipse.org-committers/msg00933.html Otherwise patch looks good and ready to go. (In reply to comment #4) > Andy could you also follow the instructions here to comply with the > new CLA rules. Please do for your preferred email address that you > want me to use. Otherwise I cannot push your changes under the new > regime. > > http://dev.eclipse.org/mhonarc/lists/eclipse.org-committers/msg00933.html Andy, please let me know if this step is complete - TIA., Will do Srikanth, currently wrestling with signing the CLA using my gopivotal ID, it isn't as straightforward as I'd hoped. > OIOW, in both instances the offset points to the instruction that does
a checkCast for J. Is this reasonable ? 8b92 also generates confusing code,
so please take a look and tell me what you think.
Yes, that needs another look, it is a bit weird. Let me do that. I am assuming the offset byte indicates which checkcast is important but two more testcases I also want to add:
- multiple intersection casts in one expression
- intersection casts with discarded casts (although I know there is another bugzilla for discarded casts, maybe this would set better in that patch).
I overlooked that JDT generates the intersection casts 'backwards' (from right to left as they were expressed in the file). I think I might reverse that logic to bring things more inline with javac and make them more intuitive. Although saying that, javac b97 and the language tools that I use to test jsr 308 seem to generate extra unnecessary casts for some reason, for "Object o2 = (I & J)o" those tools generate: 0: aload_0 1: checkcast #2 // class I 4: checkcast #3 // class J 7: checkcast #2 // class I whilst jdt was generating: 0: aload_0 1: checkcast #3 // class J 4: checkcast #2 // class I but with my latest tweak is generating: 0: aload_0 1: checkcast #2 // class I 4: checkcast #3 // class J this ensures the byte index for the type annotation points to the right cast. Re: the other issue with dropping casts, it looks like we never drop them in intersection casts (that saves some work...) Created attachment 233340 [details]
patch that fixes this issue.
New version of the patch. Only change from the previous in real code is in codestream, we now iterate over intersection cast type bindings forwards rather than backwards. This seemed preferable to having to encode the knowledge that they are generated backwards into the code that computes the attribute.
I also added two more testcases for multiple annotated casts within the same expression (one testcase for non-intersection, one for intersection). I also changed test expected data to include the bytecode in addition to the formatted annotation data.
(In reply to comment #7) > Will do Srikanth, currently wrestling with signing the CLA using my > gopivotal ID, it isn't as straightforward as I'd hoped. Andy, where are we with this ? If you need any help, Jay will be ready to help out. Fix looks good. Jay, please release and resolve as I am still having issues with releasing other author's code - TIA. Jay, please note - Andy's patch seems to have bad content for ExtendedAnnotation.java: The following line in the patch @@ -209,7 +210,10 @@ public class ExtendedAnnotation extends ClassFileStruct implements IExtendedAnno has undergone mysterious truncation and so the patch will not apply as it is. You will have to manually tweak it. TIA. (In reply to comment #10) > Created attachment 233340 [details] > patch that fixes this issue. Andy, the new eclipse contribution process also requires you to sign off the patch - something like this would do: "This contribution complies with http://www.eclipse.org/legal/CoO.php" This contribution complies with http://www.eclipse.org/legal/CoO.php |