Skip to content

Commit

Permalink
Improve generate constructor parameter names in the @Memoized exten…
Browse files Browse the repository at this point in the history 
…sion.

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
  • Loading branch information
@eamonnmcmanus
eamonnmcmanus authored and Google Java Core Libraries committed Apr 4, 2023
1 parent 1f52972 commit 4f8dbea
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 6 deletions.
4 changes: 4 additions & 0 deletions value/processor/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
28 changes: 26 additions & 2 deletions value/src/main/java/com/google/auto/value/extension/memoized/processor/MemoizeExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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()
Expand Down
26 changes: 24 additions & 2 deletions ...m/google/auto/value/extension/toprettystring/processor/ExtensionClassTypeSpecBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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 =
Expand Down
16 changes: 14 additions & 2 deletions value/src/test/java/com/google/auto/value/extension/memoized/MemoizedTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -44,6 +47,8 @@ abstract static class ValueWithKeywordName {

abstract boolean getNative0();

abstract String getNotKeyword();

@Memoized
boolean getMemoizedNative() {
return getNative();
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 4f8dbea

Please sign in to comment.