Summary: | [quick fix] @SuppressWarnings does not work as expected inside a for loop | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] JDT | Reporter: | Raksha Vasisht <raksha.vasisht> | ||||||
Component: | Core | Assignee: | Olivier Thomann <Olivier_Thomann> | ||||||
Status: | VERIFIED FIXED | QA Contact: | |||||||
Severity: | normal | ||||||||
Priority: | P3 | CC: | amj87.iitr, jarthana, markus.kell.r, Olivier_Thomann, srikanth_sankaran | ||||||
Version: | 3.6 | Flags: | frederic_fusier:
review+
|
||||||
Target Milestone: | 3.6 RC1 | ||||||||
Hardware: | PC | ||||||||
OS: | Windows XP | ||||||||
Whiteboard: | |||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 105668 | ||||||||
Attachments: |
|
Description
Raksha Vasisht
2010-05-06 06:11:19 EDT
I'll investigate for 3.6RC1 Created attachment 167310 [details]
Proposed fix + regression tests
Srikanth, please review. Note that the quick fix is still in the works (bug 105668), so you need to put the @SuppressWarnings("unchecked") at the right place manually. The proposed fix works fine for me (though it's a little strange that declarationSourceStart/End means the whole field for FieldDeclarations but only a single variable for LocalDeclarations...). (In reply to comment #4) > Note that the quick fix is still in the works (bug 105668), so you need to put > the @SuppressWarnings("unchecked") at the right place manually. Bug 105668 is tagged as RC1. So I thought this one should be fixed quickly as it blocks a RC1 bug. > The proposed fix works fine for me (though it's a little strange that > declarationSourceStart/End means the whole field for FieldDeclarations but only > a single variable for LocalDeclarations...). This is the case since the beginning for historical reasons. I won't change that now for sure. Sometimes we have to live with inconsistencies :-). (In reply to comment #5) Thanks, +1 then. A comment in declarationSourceStart about the different usages would make this more easily understandable. +1 for adding that if you agree. Released for 3.6RC1. Added regression tests in: org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test287 org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test288 Note org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test288 would pass without the fix. It is there to ensure that field declaration source ends cover multiple fields declarations in case we want to fix to change that in the future. (In reply to comment #3) > Srikanth, please review. Patch looks good. Note that the junits references does not reference this bug number, but the related bug# 304031 instead. (In reply to comment #7) Verified. Thanks for the quick fix Olivier! The fix only works if the locals don't have other annotations. E.g. here, it still warns on the second arg.add(..) because of @Other (same with @Deprecated): import java.util.*; class Bug { @SuppressWarnings("rawtypes") void foo(ArrayList arg) { for ( // @Deprecated @Other @SuppressWarnings("unchecked") boolean a = arg.add(1), b = arg.add(2); Boolean.FALSE; ) { System.out.println(a && b); } } } @interface Other { } Created attachment 167791 [details]
Proposed fix + regression test
Good catch. I got caught by the return statement. So only the first annotation would be checked.
This patch fixes that issue.
Frédéric, Markus, please review. I would like this to be part of tomorrow's JDT contribution. Patch looks good to me. Released for 3.6RC1. New regression test added in: org.eclipse.jdt.core.tests.compiler.regression.AnnotationTest#test289 I noticed one inconsistency. In the following code import java.util.*; class Bug { @SuppressWarnings("rawtypes") void foo(ArrayList arg) { @SuppressWarnings("unchecked") // bad: unnecessary suppressWarnings warn boolean aa = arg.add(1), // good: local variable a never read warning bb = arg.add(1); if (bb) System.out.println("hi"); } } If either aa, or bb is not read the suppressWarnings annotation is warned by the compiler as unnecessary. This is a bogus warning since the annotation applies for atleast one variable in the declaration series. Olivier, what do you think? (In reply to comment #15) Good catch, that's a regression compared to 3.6M7. Please open a new bug for this (we don't reopen bugs after the target milestone has already shipped). Raised bug 313109 for FUP. Verified for 3.6 RC1 using build I20100513-1500. Verified. |