Introduce IdentityConversionCheck
#27
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package tech.picnic.errorprone.bugpatterns; | ||
|
|
||
| import static com.google.common.collect.ImmutableSet.toImmutableSet; | ||
| import static com.google.errorprone.BugPattern.LinkType.NONE; | ||
| import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; | ||
| import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; | ||
| import static com.google.errorprone.matchers.Matchers.anyOf; | ||
| import static com.google.errorprone.matchers.Matchers.staticMethod; | ||
| import static com.google.errorprone.suppliers.Suppliers.OBJECT_TYPE; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import com.google.common.primitives.Primitives; | ||
| import com.google.errorprone.BugPattern; | ||
| 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; | ||
| import com.google.errorprone.matchers.Matcher; | ||
| import com.google.errorprone.util.ASTHelpers; | ||
| import com.google.errorprone.util.ASTHelpers.TargetType; | ||
| import com.sun.source.tree.ExpressionTree; | ||
| import com.sun.source.tree.MethodInvocationTree; | ||
| import com.sun.tools.javac.code.Type; | ||
| import com.sun.tools.javac.code.Types; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| /** A {@link BugChecker} that flags redundant identity conversions. */ | ||
| // XXX: Consider detecting cases where a flagged expression is passed to a method, and where removal | ||
| // of the identify conversion would cause a different method overload to be selected. Depending on | ||
| // the target method such a modification may change the code's semantics or performance. | ||
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| name = "IdentityConversion", | ||
| summary = "Avoid or clarify identity conversions", | ||
| linkType = NONE, | ||
| severity = WARNING, | ||
| tags = SIMPLIFICATION) | ||
| public final class IdentityConversionCheck extends BugChecker | ||
| implements MethodInvocationTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
| private static final Matcher<ExpressionTree> IS_CONVERSION_METHOD = | ||
| anyOf( | ||
| staticMethod() | ||
| .onClassAny( | ||
| "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.ImmutableRangeMap", | ||
| "com.google.common.collect.ImmutableRangeSet", | ||
|
There was a problem hiding this comment. TIL of a There was a problem hiding this comment. Good one, that one has a |
||
| "com.google.common.collect.ImmutableSet", | ||
| "com.google.common.collect.ImmutableSetMultimap", | ||
| "com.google.common.collect.ImmutableTable") | ||
| .named("copyOf"), | ||
| staticMethod() | ||
| .onClassAny( | ||
| Primitives.allWrapperTypes().stream() | ||
| .map(Class::getName) | ||
| .collect(toImmutableSet())) | ||
| .named("valueOf"), | ||
| staticMethod().onClass(String.class.getName()).named("valueOf"), | ||
| staticMethod().onClass("reactor.adapter.rxjava.RxJava2Adapter"), | ||
|
There was a problem hiding this comment. Probably I just had a really hard time finding it, for which method does this check apply for the There was a problem hiding this comment. For the conversion methods like There was a problem hiding this comment. Ah, that makes total sense, thanks a lot! NB: Would still make sense to test this. 👀 |
||
| staticMethod() | ||
| .onClass("reactor.core.publisher.Flux") | ||
| .namedAnyOf("concat", "firstWithSignal", "from", "merge"), | ||
|
Comment on lines
+69
to
+71
There was a problem hiding this comment. IIUC for Flux<T> concat(Publisher<? extends T>... sources)case. What about e.g. Flux<T> concat(Iterable<? extends Publisher<? extends T>> sources)couldn't we statically also check whether the Or would this be handled by a combination by refaster + this check already? 👀 Same thing should also apply for There was a problem hiding this comment. Interesting, good one. I think we could best achieve this by creating a |
||
| staticMethod().onClass("reactor.core.publisher.Mono").namedAnyOf("from", "fromDirect")); | ||
|
|
||
| @Override | ||
| public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { | ||
| List<? extends ExpressionTree> arguments = tree.getArguments(); | ||
| if (arguments.size() != 1 || !IS_CONVERSION_METHOD.matches(tree, state)) { | ||
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| ExpressionTree sourceTree = arguments.get(0); | ||
| Type sourceType = ASTHelpers.getType(sourceTree); | ||
| Type resultType = ASTHelpers.getType(tree); | ||
| TargetType targetType = ASTHelpers.targetType(state); | ||
|
There was a problem hiding this comment. Genuine question: What's the target type here exactly? Why not There was a problem hiding this comment. Javadoc
Javadoc
Nice question; the type would only give us information about the current expression itself (state), while we want to know what the "expected" type of the current expression is (i.o.w. what the parent expects/wants to receive) and thus what the target type is of this expression. So in the |
||
| if (sourceType == null || resultType == null || targetType == null) { | ||
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| if (!state.getTypes().isSameType(sourceType, resultType) | ||
| && !isConvertibleWithWellDefinedEquality(sourceType, targetType.type(), state)) { | ||
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| return buildDescription(tree) | ||
| .setMessage( | ||
| "This method invocation appears redundant; remove it or suppress this warning and " | ||
| + "add a comment explaining its purpose") | ||
| .addFix(SuggestedFix.replace(tree, Util.treeToString(sourceTree, state))) | ||
| .addFix(SuggestedFixes.addSuppressWarnings(state, canonicalName())) | ||
| .build(); | ||
| } | ||
|
|
||
| private static boolean isConvertibleWithWellDefinedEquality( | ||
| Type sourceType, Type targetType, VisitorState state) { | ||
| Types types = state.getTypes(); | ||
| return !types.isSameType(targetType, OBJECT_TYPE.get(state)) | ||
| && types.isConvertible(sourceType, targetType) | ||
| && Arrays.stream(TypesWithUndefinedEquality.values()) | ||
| .noneMatch(b -> b.matchesType(sourceType, state) || b.matchesType(targetType, state)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,17 +101,4 @@ ImmutableMultiset<T> after(Stream<T> stream) { | |
| return stream.collect(toImmutableMultiset()); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
ImmutableEnumMapandImmutableEnumSet?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have a
copyOfmethod 😉There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, both inherit a
copyOfmethod fromImmutable{Map,Set}respectively.I frankly don't know: Would this then already be covered by the defined
Immutable{Map,Set}here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good one, I totally forgot that 😄. Will check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImmutableEnumMapandImmutableEnumSetare package-private and final. So in the source code we can only ever match (public) supertypes of these. I.o.w.:ImmutableEnumMapandImmutableEnumSetare implementation details we don't need to concern ourselves with.