diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AvoidValueSetter.java b/core/src/main/java/com/google/errorprone/bugpatterns/AvoidValueSetter.java new file mode 100644 index 00000000000..a31d5597f39 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AvoidValueSetter.java @@ -0,0 +1,148 @@ +/* + * Copyright 2026 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.common.base.Ascii.toUpperCase; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.VisitorState.memoize; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.fixes.SuggestedFixes.renameMethodInvocation; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.util.ASTHelpers.constValue; +import static com.google.errorprone.util.ASTHelpers.getEnclosedElements; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.isSubtype; +import static java.lang.String.format; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import com.sun.tools.javac.code.Type; +import java.util.Optional; +import java.util.regex.Pattern; + +/** A bugpattern; see the summary. */ +@BugPattern( + summary = + "Prefer using the enum-accepting rather than the int-accepting setter for enum fields.", + severity = WARNING) +public final class AvoidValueSetter extends BugChecker implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + var symbol = getSymbol(tree); + String name = symbol.getSimpleName().toString(); + if (!isSubtype(symbol.owner.type, MESSAGE_BUILDER.get(state), state) + || !name.endsWith("Value")) { + return NO_MATCH; + } + var matcher = PREFIX.matcher(name); + if (!matcher.find()) { + return NO_MATCH; + } + String fieldName = matcher.group(2); + + Type protoBuilderType = symbol.owner.type; + Type protoType = symbol.owner.owner.type; + + String fieldNumberMemberNameCamel = toUpperCase(fieldName) + "_FIELD_NUMBER"; + if (protoType != null + && getEnclosedElements(protoType.tsym).stream() + .anyMatch(member -> member.getSimpleName().contentEquals(fieldNumberMemberNameCamel))) { + return NO_MATCH; + } + + int argIndex; + if ((name.startsWith("set") || name.startsWith("add")) && tree.getArguments().size() == 1) { + argIndex = 0; + } else if (name.startsWith("put") && tree.getArguments().size() == 2) { + argIndex = 1; + } else { + return NO_MATCH; + } + + ExpressionTree arg = tree.getArguments().get(argIndex); + var numericValue = constValue(arg, Integer.class); + if (numericValue == null) { + return NO_MATCH; + } + + String enumSetterName = name.substring(0, name.length() - "Value".length()); + + for (Symbol member : protoBuilderType.tsym.members().getSymbols()) { + if (member instanceof MethodSymbol method + && method.name.contentEquals(enumSetterName) + && method.getParameters().size() == tree.getArguments().size() + && isSubtype( + method.getParameters().get(argIndex).type, PROTOCOL_MESSAGE_ENUM.get(state), state)) { + return fix( + tree, + arg, + numericValue, + state, + method.getParameters().get(argIndex).type, + enumSetterName); + } + } + + return NO_MATCH; + } + + private Description fix( + MethodInvocationTree tree, + ExpressionTree arg, + int numericValue, + VisitorState state, + Type enumType, + String enumSetterName) { + return findEnumConstant(enumType, numericValue) + .map( + enumConst -> { + SuggestedFix.Builder fixBuilder = + SuggestedFix.builder().merge(renameMethodInvocation(tree, enumSetterName, state)); + fixBuilder.replace( + arg, format("%s.%s", qualifyType(state, fixBuilder, enumType.tsym), enumConst)); + return describeMatch(tree, fixBuilder.build()); + }) + .orElse(NO_MATCH); + } + + private static Optional findEnumConstant(Type enumType, int numericValue) { + return getEnclosedElements(enumType.tsym).stream() + .filter( + symbol -> + symbol instanceof VarSymbol vs + && vs.getConstantValue() instanceof Integer i + && i == numericValue + && symbol.getSimpleName().toString().endsWith("_VALUE")) + .map(symbol -> symbol.getSimpleName().toString().replaceAll("_VALUE$", "")) + .findFirst(); + } + + private static final Pattern PREFIX = Pattern.compile("^(set|add|put)(.+)$"); + + private static final Supplier MESSAGE_BUILDER = + memoize(state -> state.getTypeFromString("com.google.protobuf.Message.Builder")); + + private static final Supplier PROTOCOL_MESSAGE_ENUM = + memoize(state -> state.getTypeFromString("com.google.protobuf.ProtocolMessageEnum")); +} 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 6439fa7cdfd..cf742d98f27 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -47,6 +47,7 @@ import com.google.errorprone.bugpatterns.AutoValueImmutableFields; import com.google.errorprone.bugpatterns.AutoValueSubclassLeaked; import com.google.errorprone.bugpatterns.AvoidObjectArrays; +import com.google.errorprone.bugpatterns.AvoidValueSetter; import com.google.errorprone.bugpatterns.BadAnnotationImplementation; import com.google.errorprone.bugpatterns.BadComparable; import com.google.errorprone.bugpatterns.BadImport; @@ -913,6 +914,7 @@ public static ScannerSupplier warningChecks() { AutoValueFinalMethods.class, AutoValueImmutableFields.class, AutoValueSubclassLeaked.class, + AvoidValueSetter.class, BadComparable.class, BadImport.class, BadInstanceof.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AvoidValueSetterTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AvoidValueSetterTest.java new file mode 100644 index 00000000000..6e7dfe1e196 --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AvoidValueSetterTest.java @@ -0,0 +1,138 @@ +/* + * Copyright 2026 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.google.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class AvoidValueSetterTest { + + private final CompilationTestHelper helper = + CompilationTestHelper.newInstance(AvoidValueSetter.class, getClass()); + + @Test + public void positive() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.Proto3Test; + import com.google.errorprone.bugpatterns.proto.Proto3Test.TestProto3Message; + + class Test { + void f(TestProto3Message.Builder m) { + // BUG: Diagnostic contains: m.setMyEnum(Proto3Test.TestProto3Enum.VALUE_1); + m.setMyEnumValue(1); + } + } + """) + .doTest(); + } + + @Test + public void confusinglyNamedField_noFinding() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.ProtoTest.TestProtoWithConfusingNames; + + class Test { + void f(TestProtoWithConfusingNames.Builder m) { + m.setBarFieldValue(1); + } + } + """) + .doTest(); + } + + @Test + public void zerothElement_finding() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.Proto3Test; + import com.google.errorprone.bugpatterns.proto.Proto3Test.TestProto3Message; + + class Test { + void f(TestProto3Message.Builder m) { + // BUG: Diagnostic contains: m.setMyEnum(Proto3Test.TestProto3Enum.UNKNOWN); + m.setMyEnumValue(0); + } + } + """) + .doTest(); + } + + @Test + public void unrecognisedNumber_noFinding() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.Proto3Test; + import com.google.errorprone.bugpatterns.proto.Proto3Test.TestProto3Message; + + class Test { + void f(TestProto3Message.Builder m) { + m.setMyEnumValue(99); + } + } + """) + .doTest(); + } + + @Test + public void repeatedField() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.Proto3Test; + import com.google.errorprone.bugpatterns.proto.Proto3Test.TestProto3Message; + + class Test { + void f(TestProto3Message.Builder m) { + // BUG: Diagnostic contains: m.addRepeatedEnum(Proto3Test.TestProto3Enum.VALUE_1); + m.addRepeatedEnumValue(1); + } + } + """) + .doTest(); + } + + @Test + public void mapField() { + helper + .addSourceLines( + "Test.java", + """ + import com.google.errorprone.bugpatterns.proto.Proto3Test; + import com.google.errorprone.bugpatterns.proto.Proto3Test.TestProto3Message; + + class Test { + void f(TestProto3Message.Builder m) { + // BUG: Diagnostic contains: m.putMapEnum("a", Proto3Test.TestProto3Enum.VALUE_1); + m.putMapEnumValue("a", 1); + } + } + """) + .doTest(); + } +} diff --git a/core/src/test/proto/proto3_test.proto b/core/src/test/proto/proto3_test.proto index 1fc8956ccad..4d8dd9ab1e4 100644 --- a/core/src/test/proto/proto3_test.proto +++ b/core/src/test/proto/proto3_test.proto @@ -24,6 +24,8 @@ message TestProto3Message { string my_string = 1; bytes my_bytes = 2; TestProto3Enum my_enum = 3; + repeated TestProto3Enum repeated_enum = 4; + map map_enum = 5; } enum TestProto3Enum {