RedundantSetterCall: note that setFooValue and setFooBytes can be aliases for the field foo.

Flume: unknown commit
PiperOrigin-RevId: 800535370

Author: graememorgan

core/src/main/java/com/google/errorprone/bugpatterns/RedundantSetterCall.java

@@ -16,8 +16,10 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static com.google.common.base.Ascii.toUpperCase;
 import static com.google.common.base.CaseFormat.UPPER_CAMEL;
 import static com.google.common.base.CaseFormat.UPPER_UNDERSCORE;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.Streams.stream;
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
 import static com.google.errorprone.VisitorState.memoize;
@@ -39,10 +41,12 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.StandardTags;
+import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
 import com.google.errorprone.fixes.SuggestedFix;
@@ -105,16 +109,27 @@ && isWithinAutoValueBuilder(symbol, state)
                       (ExpressionTree) state.getPath().getParentPath().getParentPath().getLeaf(),
                       state)));
 
+  private final boolean improvements;
+
   @Inject
-  RedundantSetterCall() {}
+  RedundantSetterCall(ErrorProneFlags flags) {
+    this.improvements = flags.getBoolean("RedundantSetterCall:Improvements").orElse(true);
+  }
 
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
     if (!TERMINAL_FLUENT_SETTER.matches(tree, state)) {
       return Description.NO_MATCH;
     }
+
+    var owner = getUpperBound(getType(tree), state.getTypes()).tsym.owner;
+    boolean isProto = owner != null && isSubtype(owner.type, MESSAGE_LITE.get(state), state);
+
     ListMultimap<Field, FieldWithValue> setters = ArrayListMultimap.create();
-    ImmutableMap<String, OneOfField> oneOfSetters = scanForOneOfSetters(getType(tree), state);
+    ImmutableMap<String, OneOfField> oneOfSetters =
+        isProto ? scanForOneOfSetters(getType(tree), state) : ImmutableMap.of();
+    ImmutableSet<String> fieldNames = isProto ? getFields(owner) : ImmutableSet.of();
+
     Type type = ASTHelpers.getReturnType(tree);
     for (ExpressionTree current = tree;
         FLUENT_SETTER.matches(current, state);
@@ -133,6 +148,17 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
       if (methodName.endsWith("Builder")) {
         break;
       }
+      if (improvements && isProto && methodName.startsWith("set")) {
+        String withoutSet = methodName.replaceFirst("^set", "");
+        if (!fieldNames.contains(toUpperCase(withoutSet))) {
+          if (methodName.endsWith("Value")) {
+            methodName = methodName.replaceFirst("Value$", "");
+          }
+          if (methodName.endsWith("Bytes")) {
+            methodName = methodName.replaceFirst("Bytes$", "");
+          }
+        }
+      }
       for (FieldType fieldType : FieldType.values()) {
         FieldWithValue match = fieldType.match(methodName, method, state);
         if (match != null) {
@@ -158,7 +184,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     return Description.NO_MATCH;
   }
 
-  private ImmutableMap<String, OneOfField> scanForOneOfSetters(Type type, VisitorState state) {
+  private static ImmutableMap<String, OneOfField> scanForOneOfSetters(
+      Type type, VisitorState state) {
     var owner = getUpperBound(type, state.getTypes()).tsym.owner;
     if (owner == null || isSubtype(owner.type, GENERATED_MESSAGE_LITE.get(state), state)) {
       return ImmutableMap.of();
@@ -179,9 +206,27 @@ private ImmutableMap<String, OneOfField> scanForOneOfSetters(Type type, VisitorS
     return builder.buildOrThrow();
   }
 
+  /**
+   * Returns all the field names in the proto in uppercase with all components concatenated.
+   *
+   * <p>This is an odd format, but it works to compare the existence of a field based on a getter,
+   * given both {@code foo_bar} and {@code fooBar} as field names generate a getter named {@code
+   * getFooBar} (so we normalise to {@code FOOBAR}).
+   */
+  private static ImmutableSet<String> getFields(Symbol proto) {
+    return getEnclosedElements(proto).stream()
+        .map(element -> element.getSimpleName().toString())
+        .filter(name -> name.endsWith("_FIELD_NUMBER"))
+        .map(name -> toUpperCase(name.replaceFirst("_FIELD_NUMBER$", "").replace("_", "")))
+        .collect(toImmutableSet());
+  }
+
   private static final Supplier<Type> GENERATED_MESSAGE_LITE =
       memoize(state -> state.getTypeFromString("com.google.protobuf.GeneratedMessageLite"));
 
+  private static final Supplier<Type> MESSAGE_LITE =
+      memoize(state -> state.getTypeFromString("com.google.protobuf.MessageLite"));
+
   private Description describe(
       Field field, Collection<FieldWithValue> locations, VisitorState state) {
     // We flag up all duplicate sets, but only suggest a fix if the setter is given the same

core/src/test/proto/proto3_test.proto

@@ -31,3 +31,10 @@ enum TestProto3Enum {
   VALUE_1 = 1;
   VALUE_2 = 2;
 }
+
+message TestProto3WithOneof {
+  oneof foo {
+    TestProto3Enum enum_field = 1;
+    string string_field = 2;
+  }
+}

core/src/test/proto/proto_test.proto

@@ -33,6 +33,8 @@ message TestProtoMessage {
   /* Named ending in "builder" to check the Java builder API */
   optional TestFieldProtoMessage foo_builder = 6;
 
+  optional string string_field = 7;
+
   extensions 100 to 199;
 }
 
@@ -41,13 +43,23 @@ message TestOneOfMessage {
     string foo = 1;
     string bar = 2;
     string baz = 3;
+    TestEnum enum_field = 6;
   }
   oneof AnotherOneOf {
     string quux = 4;
     string frobber = 5;
   }
 }
 
+message TestProtoWithConfusingNames {
+  optional TestEnum foo = 1;
+  optional int32 foo_value = 2;
+
+  // Fields that both collide, and are named in camelcase.
+  optional TestEnum barField = 3;
+  optional int32 barFieldValue = 4;
+}
+
 message TestMessageContainingOneOfMessage {
   optional TestOneOfMessage message = 1;
 }