Improve generate constructor parameter names in the @Memoized extension.

eamonnmcmanus · Google Java Core Libraries · commit 4f8dbea8a7ab · 2023-04-04T13:38:13.000-07:00
Previously we just added `$` to each name, in case one of them might be a Java keyword. This turns out to cause an issue with Error Prone's `/* foo= */` checked parameter-name comments, because people end up having to write `/* foo$= */` for the check to pass. With the approach here, you'll only have to write a funny parameter name if your property name is a keyword.

RELNOTES=The constructor parameter names in the class generated by `@Memoized` no longer add a `$`.
PiperOrigin-RevId: 521852747
diff --git a/value/processor/pom.xml b/value/processor/pom.xml
@@ -177,6 +177,10 @@
           <execution>
             <id>default-testCompile</id>
             <configuration>
+              <compilerArgs>
+                <!-- For MemoizeTest. -->
+                <arg>-parameters</arg>
+              </compilerArgs>
               <annotationProcessorPaths>
                 <path>
                   <groupId>com.google.auto.value</groupId>
diff --git a/value/src/main/java/com/google/auto/value/extension/memoized/processor/MemoizeExtension.java b/value/src/main/java/com/google/auto/value/extension/memoized/processor/MemoizeExtension.java
@@ -17,6 +17,7 @@
 
 import static com.google.auto.common.GeneratedAnnotationSpecs.generatedAnnotationSpec;
 import static com.google.auto.common.MoreStreams.toImmutableList;
+import static com.google.auto.common.MoreStreams.toImmutableMap;
 import static com.google.auto.common.MoreStreams.toImmutableSet;
 import static com.google.auto.value.extension.memoized.processor.ClassNames.MEMOIZED_NAME;
 import static com.google.auto.value.extension.memoized.processor.MemoizedValidator.getAnnotationMirror;
@@ -44,6 +45,7 @@
 import com.google.auto.service.AutoService;
 import com.google.auto.value.extension.AutoValueExtension;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.errorprone.annotations.FormatMethod;
 import com.squareup.javapoet.AnnotationSpec;
@@ -58,6 +60,7 @@
 import com.squareup.javapoet.TypeVariableName;
 import java.util.List;
 import java.util.Optional;
+import java.util.Set;
 import javax.annotation.processing.Messager;
 import javax.annotation.processing.ProcessingEnvironment;
 import javax.lang.model.SourceVersion;
@@ -193,15 +196,36 @@ private ImmutableList<TypeVariableName> annotatedTypeVariableNames() {
 
     private MethodSpec constructor() {
       MethodSpec.Builder constructor = constructorBuilder();
+      // TODO(b/35944623): Replace this with a standard way of avoiding keywords.
+      Set<String> propertyNames = context.properties().keySet();
+      ImmutableMap<String, String> parameterNames =
+          propertyNames.stream()
+              .collect(
+                  toImmutableMap(name -> name, name -> generateIdentifier(name, propertyNames)));
       context
           .propertyTypes()
-          .forEach((name, type) -> constructor.addParameter(annotatedType(type), name + "$"));
+          .forEach(
+              (name, type) ->
+                  constructor.addParameter(annotatedType(type), parameterNames.get(name)));
       String superParams =
-          context.properties().keySet().stream().map(n -> n + "$").collect(joining(", "));
+          context.properties().keySet().stream().map(parameterNames::get).collect(joining(", "));
       constructor.addStatement("super($L)", superParams);
       return constructor.build();
     }
 
+    private static String generateIdentifier(String name, Set<String> existingNames) {
+      if (!SourceVersion.isKeyword(name)) {
+        return name;
+      }
+      for (int i = 0;; i++) {
+        String newName = name + i;
+        if (!existingNames.contains(newName)) {
+          return newName;
+        }
+      }
+    }
+
+
 
     private boolean isHashCodeMemoized() {
       return memoizedMethods(context).stream()
diff --git a/value/src/main/java/com/google/auto/value/extension/toprettystring/processor/ExtensionClassTypeSpecBuilder.java b/value/src/main/java/com/google/auto/value/extension/toprettystring/processor/ExtensionClassTypeSpecBuilder.java
@@ -18,6 +18,7 @@
 
 import static com.google.auto.common.GeneratedAnnotationSpecs.generatedAnnotationSpec;
 import static com.google.auto.common.MoreStreams.toImmutableList;
+import static com.google.auto.common.MoreStreams.toImmutableMap;
 import static com.squareup.javapoet.MethodSpec.constructorBuilder;
 import static com.squareup.javapoet.TypeSpec.classBuilder;
 import static java.util.stream.Collectors.joining;
@@ -28,6 +29,7 @@
 import com.google.auto.value.extension.AutoValueExtension;
 import com.google.auto.value.extension.AutoValueExtension.Context;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.squareup.javapoet.AnnotationSpec;
 import com.squareup.javapoet.ClassName;
 import com.squareup.javapoet.MethodSpec;
@@ -36,6 +38,7 @@
 import com.squareup.javapoet.TypeSpec;
 import com.squareup.javapoet.TypeVariableName;
 import java.util.List;
+import java.util.Set;
 import javax.lang.model.SourceVersion;
 import javax.lang.model.type.TypeMirror;
 import javax.lang.model.util.Elements;
@@ -117,15 +120,34 @@ private ImmutableList<TypeVariableName> annotatedTypeVariableNames() {
 
   private MethodSpec constructor() {
     MethodSpec.Builder constructor = constructorBuilder();
+    // TODO(b/35944623): Replace this with a standard way of avoiding keywords.
+    Set<String> propertyNames = context.properties().keySet();
+    ImmutableMap<String, String> parameterNames =
+        propertyNames.stream()
+            .collect(toImmutableMap(name -> name, name -> generateIdentifier(name, propertyNames)));
     context
         .propertyTypes()
-        .forEach((name, type) -> constructor.addParameter(annotatedType(type), name + "$"));
+        .forEach(
+            (name, type) ->
+                constructor.addParameter(annotatedType(type), parameterNames.get(name)));
     String superParams =
-        context.properties().keySet().stream().map(n -> n + "$").collect(joining(", "));
+        context.properties().keySet().stream().map(parameterNames::get).collect(joining(", "));
     constructor.addStatement("super($L)", superParams);
     return constructor.build();
   }
 
+  private static String generateIdentifier(String name, Set<String> existingNames) {
+    if (!SourceVersion.isKeyword(name)) {
+      return name;
+    }
+    for (int i = 0; ; i++) {
+      String newName = name + i;
+      if (!existingNames.contains(newName)) {
+        return newName;
+      }
+    }
+  }
+
   /** Translate a {@link TypeMirror} into a {@link TypeName}, including type annotations. */
   private static TypeName annotatedType(TypeMirror type) {
     List<AnnotationSpec> annotations =
diff --git a/value/src/test/java/com/google/auto/value/extension/memoized/MemoizedTest.java b/value/src/test/java/com/google/auto/value/extension/memoized/MemoizedTest.java
@@ -15,7 +15,9 @@
  */
 package com.google.auto.value.extension.memoized;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.truth.Truth.assertThat;
+import static java.util.Arrays.stream;
 import static org.junit.Assert.fail;
 
 import com.google.auto.value.AutoValue;
@@ -27,6 +29,7 @@
 import java.lang.reflect.AnnotatedType;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
+import java.lang.reflect.Parameter;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -44,6 +47,8 @@ abstract static class ValueWithKeywordName {
 
     abstract boolean getNative0();
 
+    abstract String getNotKeyword();
+
     @Memoized
     boolean getMemoizedNative() {
       return getNative();
@@ -358,12 +363,19 @@ public void testToString() {
   }
 
   @Test
-  public void keywords() {
-    ValueWithKeywordName value = new AutoValue_MemoizedTest_ValueWithKeywordName(true, false);
+  public void keywords() throws Exception {
+    ValueWithKeywordName value =
+        new AutoValue_MemoizedTest_ValueWithKeywordName(true, false, "foo");
     assertThat(value.getNative()).isTrue();
     assertThat(value.getMemoizedNative()).isTrue();
     assertThat(value.getNative0()).isFalse();
     assertThat(value.getMemoizedNative0()).isFalse();
+
+    Constructor<?> constructor =
+        value.getClass().getDeclaredConstructor(boolean.class, boolean.class, String.class);
+    ImmutableList<String> names =
+        stream(constructor.getParameters()).map(Parameter::getName).collect(toImmutableList());
+    assertThat(names).contains("notKeyword");
   }
 
   @Test
