Community
Participate
Working Groups
3.1 add an option to place the @SuppressWarning annotation in the smallest scope you can. Currently, it never adds it to local variables or parameters but places them on the method which is potentially dangerous because it hides all warnings in the method
We add the suppress warning annotation to the local declaration if the warning affects the declaration itself and not some code in the initialization. For example void foo() { int i; } results in void foo() { @SuppressWarnings("unused") int i; } However, void foo() { int i= bar(); } with a problem on bar results in putting the supress warning annotation to the method since you normally can't annotate expressions/statements. This is to make this consistent with other cases where statements or expression cause problems. Martin, additional comments ?
This was done intentionally. The question is, should an annotion of a local really include its initializer? The spec that says something like: it covers the full source range of the declaration. In an AST technical way, the initializer is part of the declaration, so jdt-core included the initializer. In our understanding, an annotation is most comparable to a modifier. A modifier for a field like 'private', 'final', 'transient' doesn't relate to the initializer, just to the declaration of the field. final int i= j; doesn't mean that also j is final. Should it really make a difference if your write: int i; i= j; or int i= j; As quick fix makes it easy to add new suppresswarnings, we don't want to support 'bad practice', thus we prefer to put the annotations to a place where whe think they make more sense. As soon as annotations are used more widely and spec'ed better we will change this of course.
*** Bug 190678 has been marked as a duplicate of this bug. ***
*** Bug 190701 has been marked as a duplicate of this bug. ***
(In reply to comment #2) > As soon as annotations are used more widely and spec'ed better we will change > this of course. Can it be reopened for 3.4? P.S. The bug subject should be "SuppressWarning*s*" (for better searchability).
bug 105668 is about a class' static finals aka constants. I do not consider it a "best practice" to turn off all warnings for a class just because one of it's constants is using deprecated API. I see your reasoning for locals and methods but not for class-level constructs (e.g. initializers.) Also, most programmers are not aware that one can put the @SuppressWarnings annotation not only at the method/class level put also at field/local level. I second the minimal scope approach.
reopen for 3.4
I think this is important to fix in the case of @SuppressWarnings("unchecked"), which should be added at the smallest possible scope. For example: public void foo(List<?> unknownList) { List<String> stringList = (List<String>) unknownList; // ... } This correctly gives an "unchecked cast" warning on the cast to List<String>, which we can suppress if we know that unknownList really is of type List<String>. But the quick fix puts @SuppressWarnings("unchecked") on the whole foo method rather than on the declaration of the stringList variable. As a result, any other unchecked warnings in the foo method will be masked, including ones that highlight genuine type safety problems that should not be suppressed. I.e. the correct quick fix should be: public void foo(List<?> unknownList) { @SuppressWarnings("unchecked") List<String> stringList = (List<String>) unknownList; // ... } But the quick fix offered is: @SuppressWarnings("unchecked") public void foo(List<?> unknownList) { List<String> stringList = (List<String>) unknownList; // ... }
I totally agree with Neil's comment. The best possible fix should be suggested and in my opinion that's the one with the narrowest scope. When migrating EMF 2.3 to Java 5.0 this shortcoming was annoying and in fact I didn't know until well into the process that I could put an annotation on any declaration, including one nested in a method body.
I'll try to get this in for 3.6 (a patch would always be welcome ;-).
*** Bug 100444 has been marked as a duplicate of this bug. ***
I agree with the "narrowest scope" arguments. I have just one caveat for warnings in method parameter lists, e.g. with "Usage of raw types" set to "Warning": public void foo(ArrayList unknownList, Callable c) { } => Do you really want the @SuppressWarnings(..) for the raw types to be added for each parameter separately? Or would you expect two fix proposals, one for "foo()" and one for "unknownList" in this case? Other cases where you'd expect two proposals? Note: Bug 290034 has changed the token for suppressing raw types from "unchecked" to "rawtypes".
*** Bug 299812 has been marked as a duplicate of this bug. ***
Bug 299812 has a patch, but the questions from comment 12 need to be answered first.
(In reply to comment #12) > I agree with the "narrowest scope" arguments. I have just one caveat for > warnings in method parameter lists, e.g. with "Usage of raw types" set to > "Warning": > > public void foo(ArrayList unknownList, Callable c) { > } > > => Do you really want the @SuppressWarnings(..) for the raw types to be added > for each parameter separately? Or would you expect two fix proposals, one for > "foo()" and one for "unknownList" in this case? Other cases where you'd expect > two proposals? > > Note: Bug 290034 has changed the token for suppressing raw types from > "unchecked" to "rawtypes". I would expect separate @SuppressWarnings(..) on each parameter. The annotation should cover the SMALLEST scope possible.
> I would expect separate @SuppressWarnings(..) on each parameter. The annotation > should cover the SMALLEST scope possible. I agree with that, but I think for problems in a method signature, we also need to keep the quick fix that adds @SuppressWarnings to the method. Otherwise, it e.g. becomes cumbersome if you have a method with several raw parameters and you want to suppress them all at once. If we ONLY offer the smallest scope, we take away the existing support for such scenarios.
(In reply to comment #16) > > I would expect separate @SuppressWarnings(..) on each parameter. The annotation > > should cover the SMALLEST scope possible. > > I agree with that, but I think for problems in a method signature, we also need > to keep the quick fix that adds @SuppressWarnings to the method. Otherwise, it > e.g. becomes cumbersome if you have a method with several raw parameters and > you want to suppress them all at once. If we ONLY offer the smallest scope, we > take away the existing support for such scenarios. I did not mean to imply that the old @SuppressWarnings at the method-level be removed, only that an alternative be added to do so at the smallest scope possible (both should be offered).
(In reply to comment #17) > I did not mean to imply that the old @SuppressWarnings at the method-level be > removed, only that an alternative be added to do so at the smallest scope > possible (both should be offered). OK, so we're on the same side.
General rules: - show a quick fix for the smallest possible scope - show a quick fix for the smallest enclosing IMember - if the 2 targets are not the same, make the smaller scope more relevant Here's an example with expected quick fixes after each problem: import java.util.*; public class Try { List // Add @SuppressWarnings 'rawtypes' to 'myField' myField = new ArrayList(); // Add @SuppressWarnings 'rawtypes' to 'myField' List<String> myList = new // Add @SuppressWarnings 'unchecked' to 'myList' ArrayList // Add @SuppressWarnings 'rawtypes' to 'myList' (); List // Add @SuppressWarnings 'rawtypes' to 'm' m(List // Add @SuppressWarnings 'rawtypes' to 'param' // Add @SuppressWarnings 'rawtypes' to 'm' param) { new ArrayList(); // Add @SuppressWarnings 'rawtypes' to 'm' ArrayList // Add @SuppressWarnings 'rawtypes' to 'local' // Add @SuppressWarnings 'rawtypes' to 'm' local // Add @SuppressWarnings 'unused' to 'local' // Add @SuppressWarnings 'unused' to 'm' = new ArrayList(); // Add @SuppressWarnings 'rawtypes' to 'local' // Add @SuppressWarnings 'rawtypes' to 'm' param.add(null); // Add @SuppressWarnings 'unchecked' to 'm' return param; } }
Created attachment 165827 [details] Patch (In reply to comment #19) Here'a patch(built from Olivier's patch in bug 299812) working with the smallest scope for local variables, field variables and method parameters as shown by the above example. I am having some problems running the tests, so attached here's just the fix. Will update with the tests soon.
Tests that fail with Patch 1: org.eclipse.jdt.ui.tests.quickfix.LocalCorrectionsQuickFixTest org.eclipse.jdt.ui.tests.quickfix.ModifierCorrectionsQuickFixTest The methods you added in ASTResolving are not good. You could do what they do with ASTResolving#findAncestor(ASTNode, int), so these implementations are redundant and should be removed. Even worse is that they don't follow the pattern of the other #find*(..) methods: The existing methods all make sure that the traversal does not leave the scope of the given node. They e.g. return null if no parent could be found, and don't return an arbitrary node of the right type anywhere in the parent chain (e.g. if the given node is used somewhere inside an anonymous type inside a variable initializer). For this bug, you should implement the traversal along the ASTNode#getParent() chain manually. At each level, check whether the node could carry an annotation. Here are some more tricky cases: void m() { final Object object= new Object() { { for (List l= new ArrayList(), x= new Vector(); ;) { // proposals for 'l' and for 'object' if (l == x) break; } } }; final Runnable r= new Runnable() { public void run() { boolean b; for (b= new ArrayList().add(1); ;) { // proposals for 'run' if (b) return; } } }; }
Created attachment 167465 [details] Patch_v2 + tests Added a check for BodyDeclaration inside the findParentVariableDeclaration method that returns null if its not any kind of variable declaration in the scope of the node ensuring that some arbitrary node in the parent chain of the right type is not returned. Also added code to show proposals for anonymous class declarations and expressions inside for loops and some tests for all cases. Markus, could you pls have a look?
Created attachment 167978 [details] Fix v3 Sorry, patch v2 was too complicated for me (too many cases to consider...). Here's a version that keeps all the logic about what nodes can have SuppressWarnings annotations in one place (in the switch statement we already had). That will also make it easier to add new targets when jsr308 adds annotations in more places. The tests are great! I only had to added a third case to ModifierCorrectionsQuickFixTest.testSuppressWarningsAnonymousClass1(), since this patch always adds a proposal for the enclosing member and for all locals in between (patch v2 only added 2 proposals to the closest local variables).
Raksha, could you please review Fix v3?
(In reply to comment #24) > Raksha, could you please review Fix v3? Yep, the code looks simpler after removing existing if statements for Variable declarations. Rest looks good. +1
Fixed in HEAD.
Verified for 3.6 RC1 with build I20100516-0800.
