Introduce IdentityConversion BugPattern
#5
Conversation
rickie
changed the title
IdentityConversion IdentityConversion BugPattern
rickie
force-pushed
the
rossendrijver/unwrap_unnecessary_expression
branch
from
5ab5742
to
abf7987
Compare
|
Rebased and also updated the year to 2022 ;). |
There was a problem hiding this comment.
Added a commit, but did not yet update the tests. I wonder how likely it is that (given the new insights) this PR will be accepted upstream. Perhaps we should instead just incorporate this into Error Prone Support, for now? (We can of course just ask, either now or later.)
If we do go for Error Prone Support then we'd write the tests slightly differently, which is why I didn't change those yet.
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| if (!IS_CONVERSION_METHOD.matches(tree, state) && tree.getArguments().size() != 1) { |
There was a problem hiding this comment.
| if (!IS_CONVERSION_METHOD.matches(tree, state) && tree.getArguments().size() != 1) { | |
| if (!IS_CONVERSION_METHOD.matches(tree, state) || tree.getArguments().size() != 1) { |
This also shows we should have better test coverage 👀
Next to that, the second check is cheaper, so I'd swap them:
| if (!IS_CONVERSION_METHOD.matches(tree, state) && tree.getArguments().size() != 1) { | |
| if (tree.getArguments().size() != 1 || !IS_CONVERSION_METHOD.matches(tree, state)) { |
| List<? extends ExpressionTree> arguments = tree.getArguments(); | ||
| ExpressionTree sourceTree = arguments.get(0); |
There was a problem hiding this comment.
As-is arguments can be inlined. But we can reuse the variable if we move it to before the if-statement.
| "com.google.common.collect.ImmutableBiMap", | ||
| "com.google.common.collect.ImmutableList", | ||
| "com.google.common.collect.ImmutableListMultimap", | ||
| "com.google.common.collect.ImmutableMap", | ||
| "com.google.common.collect.ImmutableMultimap", | ||
| "com.google.common.collect.ImmutableMultiset", | ||
| "com.google.common.collect.ImmutableRangeSet", | ||
| "com.google.common.collect.ImmutableSet", | ||
| "com.google.common.collect.ImmutableSetMultimap", | ||
| "com.google.common.collect.ImmutableSortedMap", | ||
| "com.google.common.collect.ImmutableSortedMultiset", | ||
| "com.google.common.collect.ImmutableSortedSet", | ||
| "com.google.common.collect.ImmutableTable") |
There was a problem hiding this comment.
These come with a number of caveats:
- Except for
ImmutableTablethey all have logic to handle partial views: in those cases a copy is made, which can be important for garbage collection purposes. - If the input is a
*Sorted*(sub)type then the result might be a copy with different semantics. SeeImmutableMap.copyOfandImmutableSet.copyOffor subtle examples, but this is even clearer for e.g.ImmutableSortedSet.copyOf, which will resort the input according to the elements' natural order if necessary. - In case of
ImmutableSetMultimap.copyOfthe value comparator is also considered.
I don't think this means we shouldn't include these methods, but perhaps we should perhaps provide an alternative suggested fix with a suppression.
| BugCheckerRefactoringTestHelper.newInstance(IdentityConversion.class, getClass()); | ||
|
|
||
| @Test | ||
| public void removeValueOf() { |
There was a problem hiding this comment.
Doesn't seem like we need to have two tests for the different kind of conversion methods. (But if we add a second suggested fix we should test that.)
|
I'm completely fine with moving this one to Error Prone Support. Will put it on the list and also update the tests accordingly. Changes look good! |
|
Moved this PR to |
No description provided.