Community
Participate
Working Groups
This preference was added a couple of years ago via bug 295551. It's been instrumental in keeping our code warning-free. Unfortunately, we just came across a side-effect. It is causing 'unused import' warnings to be silenced in certain situations. This is happening in 3.8 and 4.2 Reproducibility steps: 1. Create Plug-in project (use all defaults) 2. In Activator.java do the following - add unused import statement for java.util.List - add import statement for internal type org.eclipse.ui.internal.util.Descriptors - Add @SuppressWarnings(“restriction”) right above the class declaration to silence the warning caused by the last step At this point, the file should have two unused import warnings 3. Bring up the project properties and check “Enable project specific settings” in the Java Compiler > Errors/Warnings page 4. Change “Discouraged reference” from Warning to Error and do the same for “Suppress optional errors with ‘@SuppressWarnings’” BUG: the unused import warnings go away. Note the only things we are suppressing with @SuppressWarnings are restriction issues.. The unused imports should continue to be flagged as a warning.
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
