Skip to content

Commit

Permalink
Add support for external init annotations in constructors (#725)
Browse files Browse the repository at this point in the history
NullAway has existing support for `-XepOpt:NullAway:ExternalInitAnnotations=...` as configuration option.
Before this PR, that option allows listing a set of class-level annotations with the following semantics:

> A list of annotations for classes that are "externally initialized." Tools like the [Cassandra Object Mapper](https://docs.datastax.com/en/developer/java-driver/3.2/manual/object_mapper/) do their own field initialization of objects with a certain annotation (like [@table](https://docs.datastax.com/en/drivers/java/3.2/com/datastax/driver/mapping/annotations/Table.html)), after invoking the zero-argument constructor. For any class annotated with an external-init annotation, we don't check that the zero-arg constructor initializes all non-null fields.

This PR extends that configuration option to also allow annotations that are added directly to the zero-arguments
constructor of such an externally initialized class. The reason for this is that it's often desired to document why
the empty constructor exists, and doing so at the declaration site for the constructor makes code more readable than
doing so at the class declaration level.

The canonical example here is GSON serialization, which requires classes to have a private zero-arguments constructor
which is called for deserialization, in addition to their normal initializing constructors.

As a follow up to this PR landing, we will update the Wiki docs accordingly.
  • Loading branch information
lazaroclapp committed Jan 31, 2023
1 parent 4cabc3d commit d2e4a49
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 5 deletions.
3 changes: 3 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ public interface Config {
* Checks if annotation marks an "external-init class," i.e., a class where some external
* framework initializes object fields after invoking the zero-argument constructor.
*
* <p>Note that this annotation can be on the class itself, or on the zero-arguments constructor,
* but will be ignored anywhere else.
*
* @param annotationName fully-qualified annotation name
* @return true if classes with the annotation are external-init
*/
Expand Down
13 changes: 8 additions & 5 deletions nullaway/src/main/java/com/uber/nullaway/NullAway.java
Original file line number Diff line number Diff line change
Expand Up @@ -1761,7 +1761,8 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) {
} else if (constructorInitInfo == null) {
// report it on the field, except in the case where the class is externalInit and
// we have no initializer methods
if (!(isExternalInit(classSymbol) && entities.instanceInitializerMethods().isEmpty())) {
if (!(symbolHasExternalInitAnnotation(classSymbol)
&& entities.instanceInitializerMethods().isEmpty())) {
errorBuilder.reportInitErrorOnField(
uninitField, state, buildDescription(getTreesInstance(state).getTree(uninitField)));
}
Expand Down Expand Up @@ -1868,12 +1869,14 @@ private SetMultimap<MethodTree, Symbol> checkConstructorInitialization(
SetMultimap<MethodTree, Symbol> result = LinkedHashMultimap.create();
Set<Symbol> nonnullInstanceFields = entities.nonnullInstanceFields();
Trees trees = getTreesInstance(state);
boolean isExternalInit = isExternalInit(entities.classSymbol());
boolean isExternalInitClass = symbolHasExternalInitAnnotation(entities.classSymbol());
for (MethodTree constructor : entities.constructors()) {
if (constructorInvokesAnother(constructor, state)) {
continue;
}
if (constructor.getParameters().size() == 0 && isExternalInit) {
if (constructor.getParameters().size() == 0
&& (isExternalInitClass
|| symbolHasExternalInitAnnotation(ASTHelpers.getSymbol(constructor)))) {
// external framework initializes fields in this case
continue;
}
Expand All @@ -1888,8 +1891,8 @@ private SetMultimap<MethodTree, Symbol> checkConstructorInitialization(
return result;
}

private boolean isExternalInit(Symbol.ClassSymbol classSymbol) {
return StreamSupport.stream(NullabilityUtil.getAllAnnotations(classSymbol).spliterator(), false)
private boolean symbolHasExternalInitAnnotation(Symbol symbol) {
return StreamSupport.stream(NullabilityUtil.getAllAnnotations(symbol).spliterator(), false)
.map((anno) -> anno.getAnnotationType().toString())
.anyMatch(config::isExternalInitClassAnnotation);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.uber.nullaway;

import java.util.Arrays;
import org.junit.Test;

public class NullAwayInitializationTests extends NullAwayTestsBase {
Expand Down Expand Up @@ -68,6 +69,59 @@ public void externalInitSupport() {
.doTest();
}

@Test
public void externalInitSupportConstructors() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:ExternalInitAnnotations=com.uber.ExternalInitConstructor"))
.addSourceLines(
"ExternalInitConstructor.java",
"package com.uber;",
"import java.lang.annotation.ElementType;",
"import java.lang.annotation.Retention;",
"import java.lang.annotation.RetentionPolicy;",
"import java.lang.annotation.Target;",
"@Retention(RetentionPolicy.CLASS)",
"@Target({ElementType.METHOD, ElementType.CONSTRUCTOR})",
"public @interface ExternalInitConstructor {}")
.addSourceLines(
"Test.java",
"package com.uber;",
"class Test {",
" Object f;",
// no error here due to external init
" @ExternalInitConstructor",
" public Test() {}",
" // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field",
" public Test(int x) {}",
" public Test(Object o) { this.f = o; }",
"}")
.addSourceLines(
"Test2.java",
"package com.uber;",
"class Test2 {",
" // BUG: Diagnostic contains: @NonNull field f not initialized",
" Object f;",
// must be on a constructor!
" @ExternalInitConstructor",
" public void init() {}",
"}")
.addSourceLines(
"Test3.java",
"package com.uber;",
"class Test3 {",
" Object f;",
// Must be zero-args constructor!
" @ExternalInitConstructor",
" // BUG: Diagnostic contains: initializer method does not guarantee @NonNull field",
" public Test3(int x) {}",
"}")
.doTest();
}

@Test
public void externalInitSupportFields() {
defaultCompilationHelper
Expand Down

0 comments on commit d2e4a49

Please sign in to comment.