Bugzilla – Full Text Bug Listing
| Summary: | The preference "Suppress optional errors with '@SuppressWarnings'" ends up silencing warnings it shouldn't | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | John Cortell <john.cortell> | ||||||
| Component: | Core | Assignee: | Satyam Kandula <satyam.kandula> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, jarthana, satyam.kandula, srikanth_sankaran, teodor.madan | ||||||
| Version: | 3.8 | Flags: | amj87.iitr:
review+
|
||||||
| Target Milestone: | 3.8 M7 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 7 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
John Cortell
Correction. Step (4) should read: 4. Change “Discouraged reference” from Warning to Error and *CHECK* “Suppress optional errors with ‘@SuppressWarnings’” Satyam, Please take a look. Thanks. Checking for the 'unused imports' is being done at the end of the 'resolve' stage and it is done only if the error count is 0. Suppressed errors are removed from the error list only at the end of the compilation process. In this case, the errors like 'forbidden reference' are found during the 'resolve' stage and hence the error count is not zero at the time of the check of 'unused imports' causing this issue. This could be fixed by using the 'non-suppressed' error count as this condition for checking of 'unused imports'. As of now we don't have this number and we can get this by looking if the error is suppressed while the error is being added. We do that currently in some circumstances and we can probably extend that now, but there is a small performance impact. IMO we could just use 'non-optional' error count instead of 'non-suppressed' error count for checking. 'non-optional' errors are the errors which cannot be configured. There will not be a performance impact because of this and more importantly I don't see any issue with this approach. I will post a patch shortly. Created attachment 211404 [details]
Patch under consideration
Srikanth, Can you please review this? Something is wrong. I see the changes already in master and may be causing some failures. I am going to revert the commit for now. (In reply to comment #6) > Something is wrong. I see the changes already in master and may be causing some > failures. I am going to revert the commit for now. Ooops, you are right. My workspace wasn't clean and I accidently released it along with another bug :(. (In reply to comment #6) > Something is wrong. I see the changes already in master and may be causing some > failures. I am going to revert the commit for now. I didn't see the revert. So, I reverted it. (In reply to comment #8) > I didn't see the revert. So, I reverted it. I thought I would just run all tests once before pushing. Now that you have reverted, HEAD should be alright now. Satyam, is the patch ready for review ? Comment#6 refers to some failures. Ayush, can you please review this when it is ready ? (In reply to comment #11) > Ayush, can you please review this when it is ready ? Sure. (In reply to comment #10) > Satyam, is the patch ready for review ? Comment#6 refers to some > failures. Yes, there are test failures :(. These are caused by presence of 'resolve' errors in the javadoc. Without this patch, 'unused import' analysis wasn't being done if there are any errors. With this patch, presence of non-optional errors will only stop processing the 'unused import' analysis. As javadoc errors are optional, the import analysis is being done and these errors are causing the extra failures. IMHO, this is right behaviour, but I will have to investigate all the test failures properly. I will update once I am done. Ayush, please wait until I update - Thanks. *** Bug 376390 has been marked as a duplicate of this bug. *** Created attachment 214363 [details]
Proposed patch
Patch after remastering the failed tests. They were javadoc tests which were failing. In all these cases they were import errors because of javadoc 'resolution' errors. I think it is better to get those errors in this cases.
Ayush, please review this patch and let me know your comments.
Patch looks good.
Note that now we do warn unused imports even when they're mandatory errors (Such as the case below).
import java.util.*;
public class E {
@SuppressWarnings({ "null", "unused" })
void foo(Object o) {
int i;
o = null;
if (o == null) {
i = 1 ;
}
System.out.println(i);
}
}
However, since these are discovered during analysis stage, i.e. after resolution stage, its ok to assume that everything is resolved and the import is indeed unused.
Thanks Ayush for your review. Released the patch in master via commit d36a5b020e7b8fc57d912810db0ffb9dd045eb9a Verified for 3.8 M7 with build I20120429-1800 |