Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moves method verification after extensions have had the chance to consume properties. #299

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion value/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
<dependency>
<groupId>com.google.testing.compile</groupId>
<artifactId>compile-testing</artifactId>
<version>0.6</version>
<version>0.9</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
*
* <p>Extensions can extend the AutoValue implementation by generating subclasses of the AutoValue
* generated class. It is not guaranteed that an Extension's generated class will be the final
* class in the inheritance hierarchy, unless its {@link #mustBeFinal(Context)} method returns true.
* class in the inheritance hierarchy, unless its
* {@link com.google.auto.value.extension.AutoValueExtension#mustBeFinal(Context)} method returns true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing the purpose of these {@link} rewrites. The previous versions work for me. Is there a context where they don't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure why those got in there. Works fine for me, too. I've reverted them.

* Only one Extension can return true for a given context. Only generated classes that will be the
* final class in the inheritance hierarchy can be declared final. All others should be declared
* abstract.
Expand Down Expand Up @@ -81,18 +82,20 @@ public boolean applicable(Context context) {
* in the inheritance hierarchy. Only one extension may be the final class, so
* this should be used sparingly.
*
* @param context The Context of the code generation for this class.
* @param context The {@link com.google.auto.value.extension.AutoValueExtension.Context} of the
* code generation for this class.
* @return True if the resulting class must be the final class in the inheritance hierarchy.
*/
public boolean mustBeFinal(Context context) {
return false;
}

/**
* Returns a non-null set of property names from {@link Context#properties()} that this extension
* intends to implement. This will prevent AutoValue from generating an implementation, and remove
* the supplied properties from builders, constructors, {@code toString}, {@code equals},
* and {@code hashCode}. The default set returned by this method is empty.
* Returns a non-null set of property names from {@link AutoValueExtension.Context#properties()}
* that this extension intends to implement. This will prevent AutoValue from generating an
* implementation, and remove the supplied properties from builders, constructors,
* {@code toString}, {@code equals}, and {@code hashCode}. The default set returned by this
* method is empty.
*
* <p>For example, Android's {@code Parcelable} interface includes a
* <a href="http://developer.android.com/reference/android/os/Parcelable.html#describeContents()">method</a>
Expand All @@ -105,7 +108,8 @@ public boolean mustBeFinal(Context context) {
* implementation and return a set containing {@code "describeContents"}. Then
* {@code describeContents} will be omitted from builders and the rest.
*
* @param context The Context of the code generation for this class.
* @param context The {@link com.google.auto.value.extension.AutoValueExtension.Context} of the code
* generation for this class.
* @return A collection of property names that this extension intends to implement.
*/
public Set<String> consumeProperties(Context context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ private void processType(TypeElement type) {

ImmutableSet<ExecutableElement> methods =
getLocalAndInheritedMethods(type, processingEnv.getElementUtils());
ImmutableSet<ExecutableElement> methodsToImplement = methodsToImplement(type, methods);
ImmutableSet<ExecutableElement> methodsToImplement = methodsToImplement(methods);
ImmutableBiMap<String, ExecutableElement> properties =
propertyNameToMethodMap(methodsToImplement);

Expand Down Expand Up @@ -395,6 +395,7 @@ private void processType(TypeElement type) {
methods = ImmutableSet.copyOf(newMethods);
}
}
verifyMethods(type, methods);

String finalSubclass = generatedSubclassName(type, 0);
String subclass = generatedSubclassName(type, appliedExtensions.size());
Expand Down Expand Up @@ -450,7 +451,7 @@ private void defineVarsForType(
Set<ExecutableElement> methods) {
Types typeUtils = processingEnv.getTypeUtils();
determineObjectMethodsToGenerate(methods, vars);
ImmutableSet<ExecutableElement> methodsToImplement = methodsToImplement(type, methods);
ImmutableSet<ExecutableElement> methodsToImplement = methodsToImplement(methods);
Set<TypeMirror> types = new TypeMirrorSet();
types.addAll(returnTypesOf(methodsToImplement));
TypeElement generatedTypeElement =
Expand Down Expand Up @@ -643,21 +644,31 @@ private static void determineObjectMethodsToGenerate(
}
}

private ImmutableSet<ExecutableElement> methodsToImplement(
TypeElement autoValueClass, Set<ExecutableElement> methods) {
private ImmutableSet<ExecutableElement> methodsToImplement(Set<ExecutableElement> methods) {
ImmutableSet.Builder<ExecutableElement> toImplement = ImmutableSet.builder();
Set<Name> toImplementNames = Sets.newHashSet();
boolean ok = true;
for (ExecutableElement method : methods) {
if (method.getModifiers().contains(Modifier.ABSTRACT)
&& objectMethodToOverride(method) == ObjectMethodToOverride.NONE) {
if (method.getParameters().isEmpty() && method.getReturnType().getKind() != TypeKind.VOID) {
ok &= checkReturnType(autoValueClass, method);
if (toImplementNames.add(method.getSimpleName())) {
// If an abstract method with the same signature is inherited on more than one path,
// we only add it once.
toImplement.add(method);
}
}
}
}
return toImplement.build();
}

private void verifyMethods(TypeElement autoValueClass, ImmutableSet<ExecutableElement> methods) {
boolean ok = true;
for (ExecutableElement method : methods) {
if (method.getModifiers().contains(Modifier.ABSTRACT)
&& objectMethodToOverride(method) == ObjectMethodToOverride.NONE) {
if (method.getParameters().isEmpty() && method.getReturnType().getKind() != TypeKind.VOID) {
ok &= checkReturnType(autoValueClass, method);
} else {
// This could reasonably be an error, were it not for an Eclipse bug in
// ElementUtils.override that sometimes fails to recognize that one method overrides
Expand All @@ -671,7 +682,6 @@ && objectMethodToOverride(method) == ObjectMethodToOverride.NONE) {
if (!ok) {
throw new AbortProcessingException();
}
return toImplement.build();
}

private boolean checkReturnType(TypeElement autoValueClass, ExecutableElement getter) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static com.google.common.truth.Truth.assertAbout;
import static com.google.testing.compile.JavaSourceSubjectFactory.javaSource;
import static com.google.testing.compile.JavaSourcesSubjectFactory.javaSources;

import com.google.auto.value.extension.AutoValueExtension;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -120,12 +121,49 @@ public void testExtensionConsumesProperties() throws Exception {
);
assertAbout(javaSource())
.that(javaFileObject)
.processedWith(
new AutoValueProcessor(ImmutableList.<AutoValueExtension>of(new FooExtension())))
.processedWith(new AutoValueProcessor(ImmutableList.of(new FooExtension())))
.compilesWithoutError()
.and().generatesSources(expectedExtensionOutput);
}

public void testDoesntRaiseWarningOnConsumedProperties() {
JavaFileObject impl = JavaFileObjects.forSourceLines("foo.bar.Baz",
"package foo.bar;",
"import com.google.auto.value.AutoValue;",
"@AutoValue public abstract class Baz {",
" abstract String foo();",
" abstract String dizzle();",
"}");
assertAbout(javaSource())
.that(impl)
.processedWith(new AutoValueProcessor(ImmutableList.of(new FooExtension())))
.compilesWithoutError()
// expected warnings: @AutoValue and @Generated annotations not consumed
.withWarningCount(2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had success passing .withCompilerOptions("-Xlint:-processing") to turn off the warnings about @AutoValue and @Generated not being consumed by any processor. However, when I do that in this test, it passes without any other changes to the code. I think we need a test that fails before the rest of the modifications in this PR are made and passes afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that would pass since you're suppressing all processor warnings. The point of this test is to ensure that those two expected warnings, and no others, are thrown. In the error case, a warning is thrown for every property, but .withCompilerOptions("-Xlint:-processing") would suppress those.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er, no, -Xlint:-processing is only about the warning wrt annotations that haven't been claimed by a processor (and most processors, including Auto ones, don't claim annotations at all).
https://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javac.html#xlintwarnings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, yes, good point. My misunderstanding.

}

public void testExtensionWithoutConsumedPropertiesFails() throws Exception {
JavaFileObject javaFileObject = JavaFileObjects.forSourceLines(
"foo.bar.Baz",
"package foo.bar;",
"",
"import com.google.auto.value.AutoValue;",
"",
"@AutoValue",
"public abstract class Baz {",
" abstract String foo();",
" abstract String dizzle();",
" abstract Double[] bad();",
"}");
assertAbout(javaSource())
.that(javaFileObject)
.processedWith(
new AutoValueProcessor(ImmutableList.<AutoValueExtension>of(new FooExtension())))
.failsToCompile()
.withErrorContaining("An @AutoValue class cannot define an array-valued property unless "
+ "it is a primitive array");
}

public void testExtensionWithBuilderCompilation() throws Exception {

JavaFileObject javaFileObject = JavaFileObjects.forSourceLines(
Expand Down