Skip to content

Commit

Permalink
The methods returned by BuilderContext.buildMethod() and `.toBuilde…
Browse files Browse the repository at this point in the history
…rMethods()` can be inherited.

This change addresses cases like this:

```java
interface Parent<BuilderT> {
  BuilderT toBuilder();
  interface Builder<BuilderT, BuiltT> {",
    BuilderT setThing(String x);",
    BuiltT build();",
  }
}

@autovalue abstract class Foo implements Parent<Foo.Builder> {
  @AutoValue.Builder
  abstract static class Builder implements Parent.Builder<Builder, Foo> {}
}
```

Previously, there would be two problems with this. First, inheriting `toBuilder()` like this didn't work at all, because its return type is `BuilderT` and not `Foo.Builder`. Now we correctly interpret the inherited return type in the context of `Foo`. Second, in `BuilderContext.buildMethod()` we were looking for methods returning `Foo`, and again this failed because the inherited `build()` method returns `BuiltT`. The fix in both cases is to use `Types.asMemberOf` to determine the correct return type in context.

Fixes #933.

RELNOTES=The methods returned by `BuilderContext.buildMethod()` and `.toBuilderMethods()` can be inherited.
PiperOrigin-RevId: 342670816
  • Loading branch information
eamonnmcmanus authored and Google Java Core Libraries committed Nov 16, 2020
1 parent 97863af commit f2cb224
Show file tree
Hide file tree
Showing 4 changed files with 133 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1874,6 +1874,37 @@ public void testBuilderGenerics() {
assertEquals((Integer) 17, instance3.u());
}

public interface ToBuilder<BuilderT> {
BuilderT toBuilder();
}

@AutoValue
public abstract static class InheritedToBuilder<T, U>
implements ToBuilder<InheritedToBuilder.Builder<T, U>> {

public abstract T t();
public abstract U u();

public static <T, U> Builder<T, U> builder() {
return new AutoValue_AutoValueTest_InheritedToBuilder.Builder<T, U>();
}

@AutoValue.Builder
public abstract static class Builder<T, U> {
public abstract Builder<T, U> setT(T t);
public abstract Builder<T, U> setU(U u);
public abstract InheritedToBuilder<T, U> build();
}
}

@Test
public void testInheritedToBuilder() {
InheritedToBuilder<Integer, String> x =
InheritedToBuilder.<Integer, String>builder().setT(17).setU("wibble").build();
InheritedToBuilder<Integer, String> y = x.toBuilder().setT(23).build();
assertThat(y.u()).isEqualTo("wibble");
}

@AutoValue
public abstract static class BuilderWithSet<T extends Comparable<T>> {
public abstract List<T> list();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.auto.value.processor.ClassNames.AUTO_VALUE_NAME;
import static com.google.common.collect.Sets.difference;
import static com.google.common.collect.Sets.intersection;
import static java.util.Comparator.naturalOrder;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;

Expand All @@ -36,7 +37,6 @@
import java.lang.annotation.Annotation;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Optional;
Expand Down Expand Up @@ -136,7 +136,7 @@ public Set<String> getSupportedOptions() {
AutoValueExtension.IncrementalExtensionType incrementalType =
extensions.stream()
.map(e -> e.incrementalType(processingEnv))
.min(Comparator.naturalOrder())
.min(naturalOrder())
.orElse(AutoValueExtension.IncrementalExtensionType.ISOLATING);
builder.add(OMIT_IDENTIFIERS_OPTION).addAll(optionsFor(incrementalType));
for (AutoValueExtension extension : extensions) {
Expand Down Expand Up @@ -207,7 +207,7 @@ void processType(TypeElement type) {
Optional<BuilderSpec.Builder> builder = builderSpec.getBuilder();
ImmutableSet<ExecutableElement> toBuilderMethods;
if (builder.isPresent()) {
toBuilderMethods = builder.get().toBuilderMethods(typeUtils(), abstractMethods);
toBuilderMethods = builder.get().toBuilderMethods(typeUtils(), type, abstractMethods);
} else {
toBuilderMethods = ImmutableSet.of();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import static javax.lang.model.util.ElementFilter.methodsIn;
import static javax.lang.model.util.ElementFilter.typesIn;

import com.google.auto.common.MoreElements;
import com.google.auto.common.MoreTypes;
import com.google.auto.value.extension.AutoValueExtension;
import com.google.auto.value.processor.AutoValueOrOneOfProcessor.Property;
Expand All @@ -49,6 +50,7 @@
import javax.lang.model.element.TypeParameterElement;
import javax.lang.model.element.VariableElement;
import javax.lang.model.type.DeclaredType;
import javax.lang.model.type.ExecutableType;
import javax.lang.model.type.TypeKind;
import javax.lang.model.type.TypeMirror;
import javax.lang.model.util.Types;
Expand Down Expand Up @@ -142,14 +144,23 @@ && erasedTypeIs(m.getReturnType(), builderTypeElement))

@Override
public Optional<ExecutableElement> buildMethod() {
return methodsIn(builderTypeElement.getEnclosedElements()).stream()
Types typeUtils = processingEnv.getTypeUtils();
DeclaredType builderTypeMirror = MoreTypes.asDeclared(builderTypeElement.asType());
return MoreElements.getLocalAndInheritedMethods(
builderTypeElement, typeUtils, processingEnv.getElementUtils())
.stream()
.filter(
m ->
m.getSimpleName().contentEquals("build")
&& !m.getModifiers().contains(Modifier.PRIVATE)
&& !m.getModifiers().contains(Modifier.STATIC)
&& m.getParameters().isEmpty()
&& erasedTypeIs(m.getReturnType(), autoValueClass))
&& m.getParameters().isEmpty())
.filter(
m -> {
ExecutableType methodMirror =
MoreTypes.asExecutable(typeUtils.asMemberOf(builderTypeMirror, m));
return erasedTypeIs(methodMirror.getReturnType(), autoValueClass);
})
.findFirst();
}

Expand Down Expand Up @@ -201,18 +212,27 @@ public Set<ExecutableElement> toBuilderMethods() {
* <p>We currently impose that there cannot be more than one such method.
*/
ImmutableSet<ExecutableElement> toBuilderMethods(
Types typeUtils, Set<ExecutableElement> abstractMethods) {
Types typeUtils,
TypeElement autoValueType,
Set<ExecutableElement> abstractMethods) {

List<String> builderTypeParamNames =
builderTypeElement.getTypeParameters().stream()
.map(e -> e.getSimpleName().toString())
.collect(toList());

DeclaredType autoValueTypeMirror = MoreTypes.asDeclared(autoValueType.asType());
ImmutableSet.Builder<ExecutableElement> methods = ImmutableSet.builder();
for (ExecutableElement method : abstractMethods) {
if (builderTypeElement.equals(typeUtils.asElement(method.getReturnType()))) {
if (!method.getParameters().isEmpty()) {
continue;
}
ExecutableType methodMirror =
MoreTypes.asExecutable(typeUtils.asMemberOf(autoValueTypeMirror, method));
TypeMirror returnTypeMirror = methodMirror.getReturnType();
if (builderTypeElement.equals(typeUtils.asElement(returnTypeMirror))) {
methods.add(method);
DeclaredType returnType = MoreTypes.asDeclared(method.getReturnType());
DeclaredType returnType = MoreTypes.asDeclared(returnTypeMirror);
List<String> typeArguments =
returnType.getTypeArguments().stream()
.filter(t -> t.getKind().equals(TypeKind.TYPEVAR))
Expand Down Expand Up @@ -452,7 +472,7 @@ public String getNullableAnnotation() {
return nullableAnnotation;
}

public String copy(AutoValueProcessor.Property property) {
public String copy(Property property) {
String copy = copier.copy.apply(property.toString());
if (property.isNullable() && !copier.acceptsNull) {
copy = String.format("(%s == null ? null : %s)", property, copy);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1074,6 +1074,78 @@ public void builderContext() {
assertThat(compilation).succeededWithoutWarnings();
}

@Test
public void builderContextWithInheritance() {
JavaFileObject parent = JavaFileObjects.forSourceLines(
"foo.bar.Parent",
"package foo.bar;",
"",
"interface Parent<BuilderT> {",
" BuilderT toBuilder();",
" interface Builder<T, BuilderT, BuiltT> {",
" BuilderT setThing(T x);",
" BuiltT build();",
" }",
"}");
JavaFileObject autoValueClass = JavaFileObjects.forSourceLines(
"foo.bar.Baz",
"package foo.bar;",
"",
"import com.google.auto.value.AutoValue;",
"",
"@AutoValue",
"abstract class Baz<T> implements Parent<Baz.Builder<T>> {",
" abstract T thing();",
" static <T> Builder<T> builder() {",
" return new AutoValue_Baz.Builder<>();",
" }",
"",
" @AutoValue.Builder",
" abstract static class Builder<T> implements Parent.Builder<T, Builder<T>, Baz<T>> {",
" }",
"}");
ContextChecker checker =
context -> {
assertThat(context.builder()).isPresent();
BuilderContext builderContext = context.builder().get();

assertThat(builderContext.builderType().getQualifiedName().toString())
.isEqualTo("foo.bar.Baz.Builder");

Set<ExecutableElement> builderMethods = builderContext.builderMethods();
assertThat(builderMethods).hasSize(1);
ExecutableElement builderMethod = Iterables.getOnlyElement(builderMethods);
assertThat(builderMethod.getSimpleName().toString()).isEqualTo("builder");

Set<ExecutableElement> toBuilderMethods = builderContext.toBuilderMethods();
assertThat(toBuilderMethods).hasSize(1);
ExecutableElement toBuilderMethod = Iterables.getOnlyElement(toBuilderMethods);
assertThat(toBuilderMethod.getSimpleName().toString()).isEqualTo("toBuilder");

Optional<ExecutableElement> buildMethod = builderContext.buildMethod();
assertThat(buildMethod).isPresent();
assertThat(buildMethod.get().getSimpleName().toString()).isEqualTo("build");
assertThat(buildMethod.get().getParameters()).isEmpty();
assertThat(buildMethod.get().getReturnType().toString()).isEqualTo("BuiltT");

ExecutableElement autoBuildMethod = builderContext.autoBuildMethod();
assertThat(autoBuildMethod).isEqualTo(buildMethod.get());

Map<String, Set<ExecutableElement>> setters = builderContext.setters();
assertThat(setters.keySet()).containsExactly("thing");
Set<ExecutableElement> thingSetters = setters.get("thing");
assertThat(thingSetters).hasSize(1);
ExecutableElement thingSetter = Iterables.getOnlyElement(thingSetters);
assertThat(thingSetter.getSimpleName().toString()).isEqualTo("setThing");
};
ContextCheckingExtension extension = new ContextCheckingExtension(checker);
Compilation compilation =
javac()
.withProcessors(new AutoValueProcessor(ImmutableList.of(extension)))
.compile(autoValueClass, parent);
assertThat(compilation).succeededWithoutWarnings();
}

@Test
public void oddBuilderContext() {
JavaFileObject autoValueClass = JavaFileObjects.forSourceLines(
Expand Down

0 comments on commit f2cb224

Please sign in to comment.