Skip to content

Commit

Permalink
IdentityConversion: validate types have a defined equality check
Browse files Browse the repository at this point in the history 
Also remove identity conversions if the source type is equal to the
type of the current tree.
  • Loading branch information
@rickie
rickie committed Feb 18, 2022
1 parent 8bf0f8f commit 9c9825e
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 12 deletions.
37 changes: 26 additions & 11 deletions ...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheck.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package tech.picnic.errorprone.bugpatterns;

import static com.google.common.base.Preconditions.checkState;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.staticMethod;

Expand All @@ -11,6 +12,7 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.bugpatterns.TypesWithUndefinedEquality;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
Expand All @@ -20,6 +22,7 @@
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.tools.javac.code.Type;
import java.util.Arrays;
import java.util.List;

/** A {@link BugChecker} that flags redundant identity conversions. */
Expand Down Expand Up @@ -74,18 +77,30 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
ExpressionTree sourceTree = arguments.get(0);
Type sourceType = ASTHelpers.getType(sourceTree);
TargetType targetType = ASTHelpers.targetType(state);
if (sourceType == null
|| targetType == null
|| !state.getTypes().isSubtype(sourceType, targetType.type())) {
return Description.NO_MATCH;
checkState(
sourceType != null && targetType != null,
"sourceType `%s` or targetType `%s` is null.",
sourceType,
targetType);

if (state.getTypes().isSameType(sourceType, ASTHelpers.getType(tree))
|| isSubtypeWithDefinedEquality(sourceType, targetType, state)) {
return buildDescription(tree)
.setMessage(
"This method invocation appears redundant; remove it or suppress this warning and "
+ "add an comment explaining its purpose")
.addFix(SuggestedFix.replace(tree, state.getSourceForNode(sourceTree)))
.addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName()))
.build();
}
return Description.NO_MATCH;
}

return buildDescription(tree)
.setMessage(
"This method invocation appears redundant; remove it or suppress this warning and "
+ "add an comment explaining its purpose")
.addFix(SuggestedFix.replace(tree, state.getSourceForNode(sourceTree)))
.addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName()))
.build();
private static boolean isSubtypeWithDefinedEquality(
Type sourceType, TargetType targetType, VisitorState state) {
return state.getTypes().isSubtype(sourceType, targetType.type())
&& Arrays.stream(TypesWithUndefinedEquality.values())
.noneMatch(
b -> b.matchesType(sourceType, state) || b.matchesType(targetType.type(), state));
}
}
21 changes: 20 additions & 1 deletion ...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/IdentityConversionCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,23 @@ void identification() {
" // BUG: Diagnostic contains:",
" Byte b1 = Byte.valueOf((Byte) Byte.MIN_VALUE);",
" Byte b2 = Byte.valueOf(Byte.MIN_VALUE);",
" // BUG: Diagnostic contains:",
" byte b3 = Byte.valueOf((Byte) Byte.MIN_VALUE);",
" // BUG: Diagnostic contains:",
" byte b4 = Byte.valueOf(Byte.MIN_VALUE);",
"",
" // BUG: Diagnostic contains:",
" Character c1 = Character.valueOf((Character) 'a');",
" Character c2 = Character.valueOf('a');",
" // BUG: Diagnostic contains:",
" char c3 = Character.valueOf((Character)'a');",
" // BUG: Diagnostic contains:",
" char c4 = Character.valueOf('a');",
"",
" // BUG: Diagnostic contains:",
" Integer int1 = Integer.valueOf((Integer) 1);",
" Integer int2 = Integer.valueOf(1);",
" // BUG: Diagnostic contains:",
" int int3 = Integer.valueOf((Integer) 1);",
" // BUG: Diagnostic contains:",
" int int4 = Integer.valueOf(1);",
Expand Down Expand Up @@ -108,6 +111,8 @@ void replacementFirstSuggestedFix() {
.setFixChooser(FixChoosers.FIRST)
.addInputLines(
"Foo.java",
"import static org.mockito.Mockito.when;",
"",
"import com.google.common.collect.ImmutableCollection;",
"import com.google.common.collect.ImmutableList;",
"import com.google.common.collect.ImmutableSet;",
Expand Down Expand Up @@ -140,12 +145,20 @@ void replacementFirstSuggestedFix() {
"",
" bar(Flux.concat(Flux.just(9)));",
" bar(Mono.from(Mono.just(10)));",
"",
" Object o1 = ImmutableSet.copyOf(ImmutableList.of());",
" Object o2 = ImmutableSet.copyOf(ImmutableSet.of());",
"",
" when(\"foo\".contains(\"f\"))",
" .thenAnswer(inv-> ImmutableSet.copyOf(ImmutableList.of(1)));",
" }",
"",
" void bar(Publisher<Integer> publisher) {}",
"}")
.addOutputLines(
"Foo.java",
"import static org.mockito.Mockito.when;",
"",
"import com.google.common.collect.ImmutableCollection;",
"import com.google.common.collect.ImmutableList;",
"import com.google.common.collect.ImmutableSet;",
Expand All @@ -165,7 +178,7 @@ void replacementFirstSuggestedFix() {
" ImmutableCollection<Integer> list2 = ImmutableList.copyOf(new ArrayList<>(ImmutableList.of(1)));",
"",
" Collection<Integer> c1 = ImmutableSet.of(1);",
" Collection<Integer> c2 = new ArrayList<>(ImmutableList.of(1));",
" Collection<Integer> c2 = ImmutableList.copyOf(new ArrayList<>(ImmutableList.of(1)));",
"",
" Flux<Integer> f1 = Flux.just(1).flatMap(e -> Flux.just(2));",
" Flux<Integer> f2 = Flux.just(3);",
Expand All @@ -178,6 +191,12 @@ void replacementFirstSuggestedFix() {
"",
" bar(Flux.just(9));",
" bar(Mono.just(10));",
"",
" Object o1 = ImmutableSet.copyOf(ImmutableList.of());",
" Object o2 = ImmutableSet.of();",
"",
" when(\"foo\".contains(\"f\"))",
" .thenAnswer(inv-> ImmutableSet.copyOf(ImmutableList.of(1)));",
" }",
"",
" void bar(Publisher<Integer> publisher) {}",
Expand Down

0 comments on commit 9c9825e

Please sign in to comment.