From 2cbed1924470f67558513a02f1eca0d4b2bbc504 Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 17 Jan 2024 03:38:47 -0800 Subject: [PATCH] Generalise ProtoRedundantSet to handle autovalues too. PiperOrigin-RevId: 599119407 --- ...ndantSet.java => RedundantSetterCall.java} | 117 +++++++++++------- .../scanner/BuiltInCheckerSuppliers.java | 4 +- ...Test.java => RedundantSetterCallTest.java} | 40 ++++-- docs/bugpattern/ProtoRedundantSet.md | 14 --- docs/bugpattern/RedundantSetterCall.md | 14 +++ 5 files changed, 118 insertions(+), 71 deletions(-) rename core/src/main/java/com/google/errorprone/bugpatterns/{ProtoRedundantSet.java => RedundantSetterCall.java} (67%) rename core/src/test/java/com/google/errorprone/bugpatterns/{ProtoRedundantSetTest.java => RedundantSetterCallTest.java} (87%) delete mode 100644 docs/bugpattern/ProtoRedundantSet.md create mode 100644 docs/bugpattern/RedundantSetterCall.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java b/core/src/main/java/com/google/errorprone/bugpatterns/RedundantSetterCall.java similarity index 67% rename from core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java rename to core/src/main/java/com/google/errorprone/bugpatterns/RedundantSetterCall.java index b295425a6c6..2a13deb3099 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/ProtoRedundantSet.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/RedundantSetterCall.java @@ -18,7 +18,12 @@ import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.hasAnnotation; +import static com.google.errorprone.util.ASTHelpers.isAbstract; +import static com.google.errorprone.util.ASTHelpers.isSameType; import com.google.auto.value.AutoValue; import com.google.common.collect.ArrayListMultimap; @@ -43,54 +48,60 @@ import java.util.regex.Pattern; import org.checkerframework.checker.nullness.qual.Nullable; -/** - * Checks that protocol buffers built with chained builders don't set the same field twice. - * - * @author ghm@google.com (Graeme Morgan) - */ +/** A BugPattern; see the summary. */ @BugPattern( - summary = "A field on a protocol buffer was set twice in the same chained expression.", + summary = "A field was set twice in the same chained expression.", severity = WARNING, + altNames = "ProtoRedundantSet", tags = StandardTags.FRAGILE_CODE) -public final class ProtoRedundantSet extends BugChecker implements MethodInvocationTreeMatcher { +public final class RedundantSetterCall extends BugChecker implements MethodInvocationTreeMatcher { - /** Matches a chainable proto builder method. */ - private static final Matcher PROTO_FLUENT_METHOD = - instanceMethod() - .onDescendantOfAny( - "com.google.protobuf.GeneratedMessage.Builder", - "com.google.protobuf.GeneratedMessageLite.Builder") - .withNameMatching(Pattern.compile("^(set|add|clear|put).+")); + /** Matches a fluent setter method. */ + private static final Matcher FLUENT_SETTER = + anyOf( + instanceMethod() + .onDescendantOfAny( + "com.google.protobuf.GeneratedMessage.Builder", + "com.google.protobuf.GeneratedMessageLite.Builder") + .withNameMatching(Pattern.compile("^(set|add|clear|put).+")), + (tree, state) -> { + if (!(tree instanceof MethodInvocationTree)) { + return false; + } + var symbol = getSymbol((MethodInvocationTree) tree); + return isAbstract(symbol) + && isWithinAutoValueBuilder(symbol, state) + && isSameType(symbol.owner.type, symbol.getReturnType(), state); + }); /** - * Matches a terminal proto builder method. That is, a chainable builder method which is either - * not followed by another method invocation, or by a method invocation which is not a {@link - * #PROTO_FLUENT_METHOD}. + * Matches a terminal setter. That is, a fluent builder method which is either not followed by + * another method invocation, or by a method invocation which is not a {@link #FLUENT_SETTER}. */ - private static final Matcher TERMINAL_PROTO_FLUENT_METHOD = + private static final Matcher TERMINAL_FLUENT_SETTER = allOf( - PROTO_FLUENT_METHOD, + FLUENT_SETTER, (tree, state) -> !(state.getPath().getParentPath().getLeaf() instanceof MemberSelectTree - && PROTO_FLUENT_METHOD.matches( + && FLUENT_SETTER.matches( (ExpressionTree) state.getPath().getParentPath().getParentPath().getLeaf(), state))); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!TERMINAL_PROTO_FLUENT_METHOD.matches(tree, state)) { + if (!TERMINAL_FLUENT_SETTER.matches(tree, state)) { return Description.NO_MATCH; } - ListMultimap setters = ArrayListMultimap.create(); + ListMultimap setters = ArrayListMultimap.create(); Type type = ASTHelpers.getReturnType(tree); for (ExpressionTree current = tree; - PROTO_FLUENT_METHOD.matches(current, state); + FLUENT_SETTER.matches(current, state); current = ASTHelpers.getReceiver(current)) { MethodInvocationTree method = (MethodInvocationTree) current; if (!ASTHelpers.isSameType(type, ASTHelpers.getReturnType(current), state)) { break; } - Symbol symbol = ASTHelpers.getSymbol(current); + Symbol symbol = getSymbol(current); if (!(symbol instanceof MethodSymbol)) { break; } @@ -100,7 +111,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState if (methodName.endsWith("Builder")) { break; } - match(method, methodName, setters); + match(method, methodName, setters, state); } setters.asMap().entrySet().removeIf(entry -> entry.getValue().size() <= 1); @@ -109,16 +120,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return Description.NO_MATCH; } - for (Map.Entry> entry : setters.asMap().entrySet()) { - ProtoField protoField = entry.getKey(); + for (Map.Entry> entry : setters.asMap().entrySet()) { + Field field = entry.getKey(); Collection values = entry.getValue(); - state.reportMatch(describe(protoField, values, state)); + state.reportMatch(describe(field, values, state)); } return Description.NO_MATCH; } private Description describe( - ProtoField protoField, Collection locations, VisitorState state) { + Field protoField, Collection locations, VisitorState state) { // We flag up all duplicate sets, but only suggest a fix if the setter is given the same // argument (based on source code). This is to avoid the temptation to apply the fix in // cases like, @@ -150,9 +161,10 @@ private Description describe( private static void match( MethodInvocationTree method, String methodName, - ListMultimap setters) { + ListMultimap setters, + VisitorState state) { for (FieldType fieldType : FieldType.values()) { - FieldWithValue match = fieldType.match(methodName, method); + FieldWithValue match = fieldType.match(methodName, method, state); if (match != null) { setters.put(match.getField(), match); } @@ -163,13 +175,12 @@ private static String nTimes(int n) { return n == 2 ? "twice" : String.format("%d times", n); } - interface ProtoField {} - enum FieldType { SINGLE { @Override - @Nullable FieldWithValue match(String name, MethodInvocationTree tree) { - if (name.startsWith("set") && tree.getArguments().size() == 1) { + @Nullable FieldWithValue match(String name, MethodInvocationTree tree, VisitorState state) { + if ((name.startsWith("set") || isWithinAutoValueBuilder(getSymbol(tree), state)) + && tree.getArguments().size() == 1) { return FieldWithValue.of(SingleField.of(name), tree, tree.getArguments().get(0)); } return null; @@ -177,7 +188,7 @@ enum FieldType { }, REPEATED { @Override - @Nullable FieldWithValue match(String name, MethodInvocationTree tree) { + @Nullable FieldWithValue match(String name, MethodInvocationTree tree, VisitorState state) { if (name.startsWith("set") && tree.getArguments().size() == 2) { Integer index = ASTHelpers.constValue(tree.getArguments().get(0), Integer.class); if (index != null) { @@ -190,7 +201,7 @@ enum FieldType { }, MAP { @Override - @Nullable FieldWithValue match(String name, MethodInvocationTree tree) { + @Nullable FieldWithValue match(String name, MethodInvocationTree tree, VisitorState state) { if (name.startsWith("put") && tree.getArguments().size() == 2) { Object key = ASTHelpers.constValue(tree.getArguments().get(0), Object.class); if (key != null) { @@ -201,15 +212,27 @@ enum FieldType { } }; - abstract FieldWithValue match(String name, MethodInvocationTree tree); + abstract FieldWithValue match(String name, MethodInvocationTree tree, VisitorState state); + } + + private static boolean isWithinAutoValueBuilder(MethodSymbol symbol, VisitorState state) { + return hasAnnotation(symbol.owner, "com.google.auto.value.AutoValue.Builder", state); + } + + interface Field { + @Override + boolean equals(@Nullable Object other); + + @Override + int hashCode(); } @AutoValue - abstract static class SingleField implements ProtoField { + abstract static class SingleField implements Field { abstract String getName(); static SingleField of(String name) { - return new AutoValue_ProtoRedundantSet_SingleField(name); + return new AutoValue_RedundantSetterCall_SingleField(name); } @Override @@ -219,13 +242,13 @@ public final String toString() { } @AutoValue - abstract static class RepeatedField implements ProtoField { + abstract static class RepeatedField implements Field { abstract String getName(); abstract int getIndex(); static RepeatedField of(String name, int index) { - return new AutoValue_ProtoRedundantSet_RepeatedField(name, index); + return new AutoValue_RedundantSetterCall_RepeatedField(name, index); } @Override @@ -235,13 +258,13 @@ public final String toString() { } @AutoValue - abstract static class MapField implements ProtoField { + abstract static class MapField implements Field { abstract String getName(); abstract Object getKey(); static MapField of(String name, Object key) { - return new AutoValue_ProtoRedundantSet_MapField(name, key); + return new AutoValue_RedundantSetterCall_MapField(name, key); } @Override @@ -252,15 +275,15 @@ public final String toString() { @AutoValue abstract static class FieldWithValue { - abstract ProtoField getField(); + abstract Field getField(); abstract MethodInvocationTree getMethodInvocation(); abstract ExpressionTree getArgument(); static FieldWithValue of( - ProtoField field, MethodInvocationTree methodInvocationTree, ExpressionTree argumentTree) { - return new AutoValue_ProtoRedundantSet_FieldWithValue( + Field field, MethodInvocationTree methodInvocationTree, ExpressionTree argumentTree) { + return new AutoValue_RedundantSetterCall_FieldWithValue( field, methodInvocationTree, argumentTree); } } diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 11139118774..981f040bb3b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -306,7 +306,6 @@ import com.google.errorprone.bugpatterns.PrivateSecurityContractProtoAccess; import com.google.errorprone.bugpatterns.ProtectedMembersInFinalClass; import com.google.errorprone.bugpatterns.ProtoBuilderReturnValueIgnored; -import com.google.errorprone.bugpatterns.ProtoRedundantSet; import com.google.errorprone.bugpatterns.ProtoStringFieldReferenceEquality; import com.google.errorprone.bugpatterns.ProtoTruthMixedDescriptors; import com.google.errorprone.bugpatterns.ProtocolBufferOrdinal; @@ -315,6 +314,7 @@ import com.google.errorprone.bugpatterns.RandomModInteger; import com.google.errorprone.bugpatterns.ReachabilityFenceUsage; import com.google.errorprone.bugpatterns.RedundantOverride; +import com.google.errorprone.bugpatterns.RedundantSetterCall; import com.google.errorprone.bugpatterns.RedundantThrows; import com.google.errorprone.bugpatterns.ReferenceEquality; import com.google.errorprone.bugpatterns.RemoveUnusedImports; @@ -1024,10 +1024,10 @@ public static ScannerSupplier warningChecks() { PrimitiveAtomicReference.class, ProtectedMembersInFinalClass.class, ProtoDurationGetSecondsGetNano.class, - ProtoRedundantSet.class, ProtoTimestampGetSecondsGetNano.class, QualifierOrScopeOnInjectMethod.class, ReachabilityFenceUsage.class, + RedundantSetterCall.class, ReferenceEquality.class, RethrowReflectiveOperationExceptionAsLinkageError.class, ReturnAtTheEndOfVoidFunction.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ProtoRedundantSetTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/RedundantSetterCallTest.java similarity index 87% rename from core/src/test/java/com/google/errorprone/bugpatterns/ProtoRedundantSetTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/RedundantSetterCallTest.java index 16ad2354545..d1545ec72c4 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/ProtoRedundantSetTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/RedundantSetterCallTest.java @@ -24,16 +24,11 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Tests for {@link ProtoRedundantSet} bugpattern. - * - * @author ghm@google.com (Graeme Morgan) - */ @RunWith(JUnit4.class) @Ignore("b/130670719") -public final class ProtoRedundantSetTest { +public final class RedundantSetterCallTest { private final CompilationTestHelper compilationHelper = - CompilationTestHelper.newInstance(ProtoRedundantSet.class, getClass()); + CompilationTestHelper.newInstance(RedundantSetterCall.class, getClass()); @Test public void positiveCase() { @@ -141,7 +136,7 @@ public void complexChaining() { @Test public void fixes() { - BugCheckerRefactoringTestHelper.newInstance(ProtoRedundantSet.class, getClass()) + BugCheckerRefactoringTestHelper.newInstance(RedundantSetterCall.class, getClass()) .addInputLines( "ProtoRedundantSetPositiveCases.java", "import com.google.errorprone.bugpatterns.proto.ProtoTest.TestFieldProtoMessage;", @@ -199,4 +194,33 @@ public void fixes() { "}") .doTest(TestMode.AST_MATCH); } + + @Test + public void autovalue() { + compilationHelper + .addSourceLines( + "Animal.java", + "import com.google.auto.value.AutoValue;", + "@AutoValue", + "abstract class Animal {", + " abstract String name();", + " static Builder builder() { return null; }", + " @AutoValue.Builder", + " abstract static class Builder {", + " abstract Builder setName(String name);", + " public Builder nonAbstractMethod(String foo) { return null; }", + " abstract Animal build();", + " }", + "}") + .addSourceLines( + "Test.java", + "class Test {", + " void test() {", + " // BUG: Diagnostic contains:", + " Animal.builder().setName(\"foo\").setName(\"bar\").build();", + " Animal.builder().nonAbstractMethod(\"foo\").nonAbstractMethod(\"bar\").build();", + " }", + "}") + .doTest(); + } } diff --git a/docs/bugpattern/ProtoRedundantSet.md b/docs/bugpattern/ProtoRedundantSet.md deleted file mode 100644 index eca6213fe37..00000000000 --- a/docs/bugpattern/ProtoRedundantSet.md +++ /dev/null @@ -1,14 +0,0 @@ -Proto builders provide a pleasant fluent interface for constructing instances. -Unlike argument lists, however, they do not prevent the user from providing -multiple values for the same field. - -Setting the same field multiple times in the same chained expression is -pointless (as the intermediate value will be overwritten), and certainly -unintentional. If the field is set to different values, it may be a bug, e.g., - -```java -return MyProto.newBuilder() - .setFoo(copy.getFoo()) - .setFoo(copy.getBar()) - .build(); -``` diff --git a/docs/bugpattern/RedundantSetterCall.md b/docs/bugpattern/RedundantSetterCall.md new file mode 100644 index 00000000000..34049828f86 --- /dev/null +++ b/docs/bugpattern/RedundantSetterCall.md @@ -0,0 +1,14 @@ +Proto and AutoValue builders provide a fluent interface for constructing +instances. Unlike argument lists, however, they do not prevent the user from +providing multiple values for the same field. + +Setting the same field multiple times in the same chained expression is +pointless (as the intermediate value will be overwritten), and can easily mask a +bug, especially if the setter is called with *different* arguments. + +```java +return MyProto.newBuilder() + .setFoo(copy.getFoo()) + .setFoo(copy.getBar()) + .build(); +```