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

Change generated AutoValue builder code to be more null-safe. #1030

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ public class CompileWithEclipseTest {
@BeforeClass
public static void setSourceRoot() {
assertWithMessage("basedir property must be set - test must be run from Maven")
.that(SOURCE_ROOT).isNotNull();
.that(SOURCE_ROOT)
.isNotNull();
}

public @Rule TemporaryFolder tmp = new TemporaryFolder();
Expand All @@ -69,14 +70,16 @@ public static void setSourceRoot() {
private static final Predicate<File> JAVA_FILE =
f -> f.getName().endsWith(".java") && !IGNORED_TEST_FILES.contains(f.getName());

private static final Predicate<File> JAVA8_TEST =
f -> f.getName().equals("AutoValueJava8Test.java")
|| f.getName().equals("AutoOneOfJava8Test.java")
|| f.getName().equals("EmptyExtension.java");
private static final ImmutableSet<String> JAVA8_FILE_NAMES =
ImmutableSet.of(
"AutoOneOfJava8Test.java",
"AutoValueJava8Test.java",
"EclipseNullable.java",
"EmptyExtension.java");

@Test
public void compileWithEclipseJava6() throws Exception {
compileWithEclipse("6", JAVA_FILE.and(JAVA8_TEST.negate()));
compileWithEclipse("6", JAVA_FILE.and(f -> !JAVA8_FILE_NAMES.contains(f.getName())));
}

@Test
Expand All @@ -103,10 +106,11 @@ private void compileWithEclipse(String version, Predicate<File> predicate) throw
// fileManager.getLocation(SYSTEM_MODULES).
File rtJar = new File(JAVA_HOME.value() + "/lib/rt.jar");
if (rtJar.exists()) {
List<File> bootClassPath = ImmutableList.<File>builder()
.add(rtJar)
.addAll(fileManager.getLocation(StandardLocation.PLATFORM_CLASS_PATH))
.build();
List<File> bootClassPath =
ImmutableList.<File>builder()
.add(rtJar)
.addAll(fileManager.getLocation(StandardLocation.PLATFORM_CLASS_PATH))
.build();
fileManager.setLocation(StandardLocation.PLATFORM_CLASS_PATH, bootClassPath);
}
Iterable<? extends JavaFileObject> sourceFileObjects =
Expand All @@ -122,7 +126,16 @@ private void compileWithEclipse(String version, Predicate<File> predicate) throw
version,
"-target",
version,
"-warn:-warningToken,-intfAnnotation");
"-warn:-warningToken,-intfAnnotation,+nullAnnot("
+ "com.google.auto.value.nullness.Nullable|"
+ "com.google.auto.value.nullness.NonNull|"
+ "com.google.auto.value.nullness.NullMarked)");
// +nullAnnot turns on nullability analysis, and the class-names in parentheses specify what
// the annotations are for nullable, not-nullable, and default-non-null, respectively. We will
// switch to using the JSpecify annotations when available.
// The purpose of turning on +nullAnnot is just to see what warnings it produces. We don't
// currently try to avoid all those warnings, which appears impossible anyway because of Eclipse
// bugs.
JavaCompiler.CompilationTask task =
compiler.getTask(null, fileManager, null, options, null, sourceFileObjects);
// Explicitly supply an empty list of extensions for AutoValueProcessor, because otherwise this
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.auto.value.eclipse;

import com.google.auto.value.AutoValue;
import com.google.auto.value.nullness.Nullable;
import com.google.common.collect.ImmutableList;
import java.util.List;
import java.util.Map;

/**
* A class that exercises Eclipse's null analysis. In {@code CompileWithEclipseTest}, we configure
* the compiler to use nullness annotations that are not Eclipse's own but custom alternatives
* defined here. Eventually we will use the <a href="http://jspecify.org">JSpecify</a> annotations
* when those are public.
*/
public final class EclipseNullable {
@AutoValue
abstract static class Nullables {
abstract int primitiveInt();

abstract Integer nonNullableInteger();

abstract @Nullable Integer nullableInteger();

abstract List<@Nullable Integer> listOfNullableIntegers();

abstract Map.@Nullable Entry<String, Integer> nullableMapEntry();

@SuppressWarnings("mutable")
abstract int @Nullable [] nullableArrayOfInts();

abstract ImmutableList<String> immutableListOfStrings();

@AutoValue.Builder
interface Builder {
Builder setPrimitiveInt(int x);

Builder setNonNullableInteger(Integer x);

Builder setNullableInteger(@Nullable Integer x);

Builder setListOfNullableIntegers(List<@Nullable Integer> x);

Builder setNullableMapEntry(Map.@Nullable Entry<String, Integer> x);

Builder setNullableArrayOfInts(int @Nullable [] x);

Builder setImmutableListOfStrings(ImmutableList<String> x);

ImmutableList.Builder<String> immutableListOfStringsBuilder();

Nullables build();
}
}

private EclipseNullable() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
@NullMarked
package com.google.auto.value.eclipse;

import com.google.auto.value.nullness.NullMarked;
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.auto.value.nullness;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target(ElementType.TYPE_USE)
@Retention(RetentionPolicy.RUNTIME)
public @interface NonNull {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.auto.value.nullness;

/**
* A temporary annotation that we use in the {@code package-info.java} for the {@code
* EclipseNullable} test class. Once <a href="http://jspecify.org">JSpecify</a> annotations become
* available, we can switch to using them and delete this file.
*/
public @interface NullMarked {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2021 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.auto.value.nullness;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Target(ElementType.TYPE_USE)
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,10 @@ void processType(TypeElement autoOneOfType) {
AutoOneOfTemplateVars vars = new AutoOneOfTemplateVars();
vars.generatedClass = TypeSimplifier.simpleNameOf(subclass);
vars.propertyToKind = propertyToKind;
defineSharedVarsForType(autoOneOfType, methods, vars);
defineVarsForType(autoOneOfType, vars, propertyMethodsAndTypes, kindGetter);
Optional<DeclaredType> nullableAnnotationType = Nullables.nullableMentionedInMethods(methods);
defineSharedVarsForType(autoOneOfType, methods, vars, nullableAnnotationType);
defineVarsForType(
autoOneOfType, vars, propertyMethodsAndTypes, kindGetter, nullableAnnotationType);

String text = vars.toText();
text = TypeEncoder.decode(text, processingEnv, vars.pkg, autoOneOfType.asType());
Expand Down Expand Up @@ -264,9 +266,11 @@ private void defineVarsForType(
TypeElement type,
AutoOneOfTemplateVars vars,
ImmutableMap<ExecutableElement, TypeMirror> propertyMethodsAndTypes,
ExecutableElement kindGetter) {
ExecutableElement kindGetter,
Optional<DeclaredType> nullableAnnotationType) {
vars.props = propertySet(
propertyMethodsAndTypes, ImmutableListMultimap.of(), ImmutableListMultimap.of());
propertyMethodsAndTypes, ImmutableListMultimap.of(), ImmutableListMultimap.of(),
nullableAnnotationType);
vars.kindGetter = kindGetter.getSimpleName().toString();
vars.kindType = TypeEncoder.encode(kindGetter.getReturnType());
TypeElement javaIoSerializable = elementUtils().getTypeElement("java.io.Serializable");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ public static class Property {
private final String identifier;
private final ExecutableElement method;
private final String type;
private final String builderFieldType;
private final ImmutableList<String> fieldAnnotations;
private final ImmutableList<String> methodAnnotations;
private final Optional<String> nullableAnnotation;
Expand All @@ -162,13 +163,15 @@ public static class Property {
String identifier,
ExecutableElement method,
String type,
String builderFieldType,
ImmutableList<String> fieldAnnotations,
ImmutableList<String> methodAnnotations,
Optional<String> nullableAnnotation) {
this.name = name;
this.identifier = identifier;
this.method = method;
this.type = type;
this.builderFieldType = builderFieldType;
this.fieldAnnotations = fieldAnnotations;
this.methodAnnotations = methodAnnotations;
this.nullableAnnotation = nullableAnnotation;
Expand Down Expand Up @@ -258,6 +261,15 @@ public boolean isNullable() {
return nullableAnnotation.isPresent();
}

/**
* The spelling for a builder field to set this property. If the property has primitive type,
* this will be the corresponding boxed type. If the property is not already {@code @Nullable},
* and a {@code @Nullable} is available, it will be applied to the (possibly boxed) type.
*/
public String getBuilderFieldType() {
return builderFieldType;
}

public String getAccess() {
return SimpleMethod.access(method);
}
Expand Down Expand Up @@ -369,11 +381,13 @@ public final boolean process(Set<? extends TypeElement> annotations, RoundEnviro
* specifically. Type annotations do not appear because they are considered part of the return
* type and will appear when that is spelled out. Annotations that are excluded by {@code
* AutoValue.CopyAnnotations} also do not appear here.
* @param nullableAnnotationType
*/
final ImmutableSet<Property> propertySet(
ImmutableMap<ExecutableElement, TypeMirror> propertyMethodsAndTypes,
ImmutableListMultimap<ExecutableElement, AnnotationMirror> annotatedPropertyFields,
ImmutableListMultimap<ExecutableElement, AnnotationMirror> annotatedPropertyMethods) {
ImmutableListMultimap<ExecutableElement, AnnotationMirror> annotatedPropertyMethods,
Optional<DeclaredType> nullableAnnotationType) {
ImmutableBiMap<ExecutableElement, String> methodToPropertyName =
propertyNameToMethodMap(propertyMethodsAndTypes.keySet()).inverse();
Map<ExecutableElement, String> methodToIdentifier = new LinkedHashMap<>(methodToPropertyName);
Expand All @@ -382,23 +396,37 @@ final ImmutableSet<Property> propertySet(
ImmutableSet.Builder<Property> props = ImmutableSet.builder();
propertyMethodsAndTypes.forEach(
(propertyMethod, returnType) -> {
Set<TypeMirror> excludedAnnotationTypes = getExcludedAnnotationTypes(propertyMethod);
String propertyType =
TypeEncoder.encodeWithAnnotations(
returnType, ImmutableList.of(), getExcludedAnnotationTypes(propertyMethod));
returnType, ImmutableList.of(), excludedAnnotationTypes);
Optional<String> nullableAnnotation = nullableAnnotationForMethod(propertyMethod);
// Use the boxed type for builder fields (if the type is primitive) and add a
// @Nullable annotation if one is available and the type is not already @Nullable
// or Optional.
ImmutableList<AnnotationMirror> extraAnnotations =
(nullableAnnotation.isPresent()
|| !nullableAnnotationType.isPresent()
|| Optionalish.isOptional(returnType))
? ImmutableList.of()
: ImmutableList.of(annotationMirror(nullableAnnotationType.get()));
String builderFieldType =
TypeEncoder.encodeWithAnnotations(
box(returnType), extraAnnotations, excludedAnnotationTypes);
String propertyName = methodToPropertyName.get(propertyMethod);
String identifier = methodToIdentifier.get(propertyMethod);
ImmutableList<String> fieldAnnotations =
annotationStrings(annotatedPropertyFields.get(propertyMethod));
ImmutableList<AnnotationMirror> methodAnnotationMirrors =
annotatedPropertyMethods.get(propertyMethod);
ImmutableList<String> methodAnnotations = annotationStrings(methodAnnotationMirrors);
Optional<String> nullableAnnotation = nullableAnnotationForMethod(propertyMethod);
Property p =
new Property(
propertyName,
identifier,
propertyMethod,
propertyType,
builderFieldType,
fieldAnnotations,
methodAnnotations,
nullableAnnotation);
Expand All @@ -412,11 +440,18 @@ final ImmutableSet<Property> propertySet(
return props.build();
}

private TypeMirror box(TypeMirror type) {
return type.getKind().isPrimitive()
? typeUtils().boxedClass(MoreTypes.asPrimitiveType(type)).asType()
: type;
}

/** Defines the template variables that are shared by AutoValue and AutoOneOf. */
final void defineSharedVarsForType(
TypeElement type,
ImmutableSet<ExecutableElement> methods,
AutoValueOrOneOfTemplateVars vars) {
AutoValueOrOneOfTemplateVars vars,
Optional<DeclaredType> nullableAnnotationType) {
vars.pkg = TypeSimplifier.packageNameOf(type);
vars.origClass = TypeSimplifier.classNameOf(type);
vars.simpleClassName = TypeSimplifier.simpleNameOf(vars.origClass);
Expand All @@ -433,8 +468,7 @@ final void defineSharedVarsForType(
vars.toString = methodsToGenerate.containsKey(ObjectMethod.TO_STRING);
vars.equals = methodsToGenerate.containsKey(ObjectMethod.EQUALS);
vars.hashCode = methodsToGenerate.containsKey(ObjectMethod.HASH_CODE);
Optional<DeclaredType> nullable = Nullables.nullableMentionedInMethods(methods);
vars.equalsParameterType = equalsParameterType(methodsToGenerate, nullable);
vars.equalsParameterType = equalsParameterType(methodsToGenerate, nullableAnnotationType);
vars.serialVersionUID = getSerialVersionUID(type);
}

Expand Down Expand Up @@ -1128,6 +1162,7 @@ static boolean hasAnnotationMirror(Element element, String annotationName) {
}

final void writeSourceFile(String className, String text, TypeElement originatingType) {
if (className.contains("EclipseNullable")) System.err.println(text);
try {
JavaFileObject sourceFile =
processingEnv.getFiler().createSourceFile(className, originatingType);
Expand Down
Loading