-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[java] Ignore unused declarations that have special name #2957
Comments
2958 adds to this by resolving the Thanks to @marquiswang |
I still don't think making this a property would buy us anything useful. It could be inconsistent between rules, and is encouraging projects making up their own conventions, like your dollar prefix. |
@oowekyala Project conventions are exactly that---project conventions. It would provide them flexibility. Yes, there's potential for misuse but surely nobody's going to fill that property field with numerous variable names. The other option, as I pointed out, is to make an exception for the main method parameters, when unused, via an additional property. That's more correct. |
|
It also avoids lengthening the variable name. |
Standardisation is all very fine but I'd rather aim for 'user delight' than 'user satisfaction'. |
I don't think there's any delight in adding a configuration property that needs to be synchronized between more than 3 rules to be consistent. The root problem is possibly that we have several rules that basically check variants of the same thing (UnusedFormalParameter, UnusedPrivateField, UnusedLocalVariable). Merging them could make this mostly go away (eg, into a single UnusedDeclaration rule), and adding a property could make sense. But we don't need that to implement the first part (make them all uniformly recognise |
This part has to be implemented with I like the idea about merging all the rules together. That and the rest can be implemented in a later release (7.0.0?). |
It doesn't. This is a rarer convention (which I've in fact only ever seen in other languages), and some projects may use the To sum it up, we will:
I'm still against supporting custom suppression annotations. The example you linked is obviously not real-world code, and the point of Also I'm not sure why @rsoesemann you scheduled it for 6.30.0, are you working on it? |
Whether projects assign significance to the |
I'm with you on this. But only for the reason that you don't wish to give your users something and then take it away. This, however, can be mitigated by defaulting this value in the property to be added. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Sorry for chiming in a bit late, but two comments from my side:
As you mention, that the original example was driven by ForLoopCanBeForEach loop - maybe you are more talking about a false-positive of ForLoopCanBeForEachLoop rather than UnusedLocalVariable? |
How does naming a variable, method or parameter starting with
This is not a false-positive example. It's a case where the rule applies and should be mitigated by the measures suggested by the rule. As is done by
Yep. |
I don't think this is theoretical at all. For example, resources:
Those are by the way false negatives of UnusedLocalVariable. They should be reported, unless, like here, they are meant to be unused (and this should be obvious in their name, which here it isn't). Another example is when you have a method which is used by a method reference, and so must have the same formals as the functional interface. Recently I wrote a bunch of those, the unused parameters are there for future use if need be, and it's ok to just suppress the violation. Suppressing by naming is more lightweight than annotations. Also I think it's normal to allow suppression via name, when EmptyCatchBlock and UnusedAssignment already do support this. This makes even more sense if we merge some of the rules together. Also, the Intellij and error prone checks support this. --
I think there's a misunderstanding: @adangel is talking about having the identifier just |
You mean we should use SuppressWarnings("RuleName") , SuppressWarnings("unused") or rename the variable as outlined in the rule documentation. (SuppressWarnings("all") will not retain semantics.) An annotation is elegant but there is an overhead to it. |
I mean suppressing by renaming the variable, specifically as opposed to using an annotation or comment |
@oowekyala In terms of readability and style, annotations are better. |
So what? You admit yourself there is an overhead. You cannot just take both sides of the argument for the sake of contradiction. And my point is that those naming conventions (prefix a variable with "unused") are already considered acceptable by other tools, and by some of our rules (eg, EmptyCatchBlock). |
I'm looking at it from the user's perspective.
|
If you're concerned about 'polluting' the code with annotations, then renaming the variable/method/parameter or suppressing it via regex/Xpath may be the best solution. I doubt, however, that @SuppressWarnings can be avoided altogether for any large code base. |
Xpath suppressions can mitigate the main conundrum by allowing the user to suppress the rule for the method. |
This won't work well with a merged rule since I'd only like to suppress UnusedFormalParameter unless there's plans to specify interactions with sub-components or sub-rules or other properties. |
Implemented solution via #3066:
The following rules are affected:
The following variable names are ignored:
ignored
, e.g.ignoredParameter
unused
, e.g.unusedVar
Is your feature request related to a problem? Please describe.
UnusedLocalVariable, UnusedFormalParameter flag all unused variables.
Describe the solution you'd like
Please add a property to the rule specifying a pattern such as starts with $, _ or unused for the variable name so that variables that meet the criteria are excluded. Additionally, you could support custom annotations that mark the variables as Unused. e.g.,
@Unused
or@Ignored
.E.g., https://github.com/Lanchon/r8/blob/e22a77d26e30615bfc9b5f598bb0f58f10dd082c/src/test/java/com/android/tools/r8/shaking/ifrule/IfOnAnnotationTestClasses.java
Describe alternatives you've considered
Example code flagged by ForLoopCanBeForEach when usual for loop used:
Additional context
Haskell/Python style
Error-prone
Also see #2923
Referenced from: #2838
The text was updated successfully, but these errors were encountered: