Skip to content
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

Generalise ProtoRedundantSet to handle autovalues too. #4253

Merged
merged 1 commit into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 [email protected] (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<ExpressionTree> 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<ExpressionTree> 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<ExpressionTree> TERMINAL_PROTO_FLUENT_METHOD =
private static final Matcher<ExpressionTree> 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<ProtoField, FieldWithValue> setters = ArrayListMultimap.create();
ListMultimap<Field, FieldWithValue> 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;
}
Expand All @@ -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);
Expand All @@ -109,16 +120,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}

for (Map.Entry<ProtoField, Collection<FieldWithValue>> entry : setters.asMap().entrySet()) {
ProtoField protoField = entry.getKey();
for (Map.Entry<Field, Collection<FieldWithValue>> entry : setters.asMap().entrySet()) {
Field field = entry.getKey();
Collection<FieldWithValue> 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<FieldWithValue> locations, VisitorState state) {
Field protoField, Collection<FieldWithValue> 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,
Expand Down Expand Up @@ -150,9 +161,10 @@ private Description describe(
private static void match(
MethodInvocationTree method,
String methodName,
ListMultimap<ProtoField, FieldWithValue> setters) {
ListMultimap<Field, FieldWithValue> 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);
}
Expand All @@ -163,21 +175,20 @@ 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;
}
},
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) {
Expand All @@ -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) {
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,11 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* Tests for {@link ProtoRedundantSet} bugpattern.
*
* @author [email protected] (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() {
Expand Down Expand Up @@ -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;",
Expand Down Expand Up @@ -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();
}
}
14 changes: 0 additions & 14 deletions docs/bugpattern/ProtoRedundantSet.md

This file was deleted.

14 changes: 14 additions & 0 deletions docs/bugpattern/RedundantSetterCall.md
Original file line number Diff line number Diff line change
@@ -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();
```
Loading