Community
Participate
Working Groups
I recently set the Java > Compiler > Errors/Warnings preference for "Unchecked generic type operation" from Warning to Error. Once the project was rebuilt, a number of warnings popped up related to unnecessary @SuppressWarnings annotations (as part of the "Unused '@SuppressWarnings' token" preference). The project contains instances of unused parameters and local variables, which were suppressed. Now, it seems JDT is not respecting the settings for "Value of local variable is not used" and "Value of parameter is not used" (and possibly others). Eclipse Platform 3.7.0.I20110310-1119 Eclipse Java Development Tools 3.7.0.v20110208-0800-7z8gFb7FMTfEULpkdZIlQtX9H155
Could you please provide a test case that shows the problem? Thanks.
I tried to boil the problem down as much as I could. It appears to have something to do with the use of varargs and generics together. ErrorsWarningsExample.java: // For Project or Workspace, under Java Compiler Errors/Warnings: // 1) Set all options to Ignore // 2) Set "Enable '@SuppressWarnings' annotations" to checked // 3) Set "Unused '@SuppressWarnings' token" to Warning // 4) Set "Suppress optional errors with '@SuppressWarnings'" to checked // 5) Set "Value of local variable is not used" to Warning // 6) Click Apply, then click Yes to rebuild project; no warnings should be preset // 7) Set "Unchecked generic type operation" to Error // 8) Click Apply, then click Yes to rebuild project; two warnings should be present // If the body of method two is commented out, the warning in method one goes away. // There is something about using the unchecked varargs method build that causes the warnings to appear. public class ErrorsWarningsExample { public void one() { @SuppressWarnings("unused") Object object = new Object(); } public void two() { @SuppressWarnings({ "unchecked", "unused" }) Object object = build(); } public final Object build(Class<? super Object>... objects) { return null; } }
This is a consequence of the fix for bug 336648. We need to "filter out" unused locals once we know that errors are left in the unit once SuppressWarnings have been applied. Here we have an error which is removed using @SuppressWarnings, but the unused locals warnings were not reported because this is not done if the unit has errors. Srikanth, I have a couple of ways to handle that. I need to investigate which one is the safest one in term of performance.
Created attachment 194283 [details] First draft This first patch filters out the unused local problem if the compilation unit contains an error that is not filtered out by SuppressWarnings annotation. This can have a potential performance impact.
Created attachment 194284 [details] Second draft This one does all the work in the finalizeProblems() call inside the CompilationUnitDeclaration. It can also have a performance hit. Srikanth, let me know what you think on these two patches.
(In reply to comment #4) > Created attachment 194283 [details] > First draft CompilationResult.java: - Is the following code: if ((newProblem.getID() & IProblem.Syntax) != 0) { if (newProblem.isError()) { this.hasSyntaxError = true; this.numberOfErrors++; } } else if (newProblem.isError()) { this.numberOfErrors++; } better written as if (newProblem.isError()) { this.numberOfErrors++; if ((newProblem.getID() & IProblem.Syntax) != 0) { this.hasSyntaxError = true; } } ?? BlockScope.java: - computeLocalVariablePositions: comments seems to have removed from just above the method. Is this intentional ? - The following loop looks suspicious to me: loop: for (int i = 0, max = compilationResult.problemCount; i < max; i++) { CategorizedProblem problem = problems[i]; if (problem.isError()) { CompilationUnitDeclaration compilationUnitDeclaration = this.compilationUnitScope().referenceContext; if (!compilationUnitDeclaration.filterOutProblem(problem)) { hasError = true; } break loop; } } Is the "break loop" in the right place ?
(In reply to comment #5) > Created attachment 194284 [details] > Second draft > > This one does all the work in the finalizeProblems() call inside the > CompilationUnitDeclaration. > It can also have a performance hit. > > Srikanth, let me know what you think on these two patches. CompilationResult.java: - Same comment as before. It looks like this code could be simplified a bit, Blockscope.java: - Lingering reference to bug 336648 should be deleted. Personally I like this patch better than the previous one. Partly, but only partly due to the fact that I didn't fully understand the previous patch (per earlier comments).
(In reply to comment #7) > - Same comment as before. It looks like this code could be simplified > a bit, I will fix this and attach a new patch for review. > - Lingering reference to bug 336648 should be deleted. Ok. > Personally I like this patch better than the previous one. Partly, but > only partly due to the fact that I didn't fully understand the previous > patch (per earlier comments). I have a preference for this patch as it is more localized.
Created attachment 194745 [details] Proposed fix + regression test Patch made from the second patch with the cleaning requested by Srikanth.
Srikanth, please review. Satyam, please run all performance tests with this patch and report back here the results. This might be a RC1 candidate as this looks like a regression over 3.6.2.
Patch looks good. Suggest add the old bug number (i.e bug 336648) to the end of the line comment // we need to check if we should discard ... in finalizeProblems() since this fix effectively replaces the earlier fix for that bug.
(In reply to comment #10) > Satyam, please run all performance tests with this patch and report back here > the results. I have run the compiler performance tests on linux and I don't see any regression.
(In reply to comment #11) > Patch looks good. Suggest add the old bug number (i.e bug 336648) > to the end of the line comment // we need to check if we should discard ... > in finalizeProblems() since this fix effectively replaces the earlier > fix for that bug. Ok, thanks. I will update the patch accordingly.
Created attachment 194832 [details] Final patch Same patch with the previous bug id added
Released for 3.7RC1.
This fix causes a few valid test failures in jdt.ui' LocalCorrectionsQuickFixTest, e.g. here: package test1; public class E { public void foo() { boolean res= process(); res= (super.hashCode() == 1); } public boolean process() { return true; } } When I set the severity of JavaCore.COMPILER_PB_UNUSED_LOCAL to ERROR, then I don't get any problem at all any more.
I think in general, optional errors should not hide any warnings. In contrast to mandatory errors, optional errors don't hamper the correct analysis for warnings.
Conservatively, it would be a better idea to revert and reopen bug 336648 - then the current bug becomes a nop. Bug 336648 can be looked at post 3.7
We only need to check if the problem we want to remove is a warning. If not, we keep it. The patch is ready.
Created attachment 194931 [details] Proposed fix + regression test Complement to the previous fix.
I have verified that Olivier's patch in comment #20 fixes the PDE/Build test failure from N20110505-2000
I also checked that this is fixing the JDT/UI failures. Released for RC1.
In fact this is still not good enough as if unused local is set to error, the error is still reported in the case of bug 336648. So reverting this one as well. We will need to come up with a better way to fix interaction between different errors/warnings for 3.8. Ideally if we could detect local usage prior to code generation, this would not longer be an issue.
I preserved the code to get the number of errors inside CompilationResult. I think this is worth being done as there is nothing to do to check if a compilation result has errors.
Does this bug have to stay open ? We can add the regression tests for the various scenarios in this bug and close it as WORKSFORME ?
Right. This could be closed as WORKSFORME.
I can't seem to reproduce the original case using steps in comment 2. After step (6), I see one warning on the unused parameter for build(..) method. After step (8), I see that warning only and nothing in addition. Did something change in this bug? I expected the original case to be still present.
(In reply to comment #27) > I can't seem to reproduce the original case using steps in comment 2. After > step (6), I see one warning on the unused parameter for build(..) method. > After step (8), I see that warning only and nothing in addition. > > Did something change in this bug? I expected the original case to be still > present. Sorry my mistake. Not reproducing the case 2 is what we want to do here, and so the build I20110511-0800 gives the desired behaviour. Hence verified.
(In reply to comment #23) > Ideally if we could detect local usage prior to code generation, this would not > longer be an issue. The pending patch in bug 328830 should be a good step in that direction :)
