AutoValue.Builder generated code is not correct for null-analysis

[frankbenoit]

Using AutoValue 1.7.4

Given the Animal example class from the documentation, with the added Eclipse JDT annotation @NonNullByDefault:

@NonNullByDefault
@AutoValue
public abstract class Animal {
    public abstract String name();

    public abstract int numberOfLegs();

    public static Builder builder() {
        return new AutoValue_Animal.Builder();
    }

    @AutoValue.Builder
    public abstract static class Builder {
        public abstract Builder setName(String value);

        public abstract Builder setNumberOfLegs(int value);

        public abstract Animal build();
    }
}

Now the AutoValue_Animal class is generated, but with errors.
Those are in the category "optional problems" and can be deactivated in the project settings. However, they might lead to other problems.

Problem 1:
The generated Builder fields:

    private @NonNull String name;
    private Integer numberOfLegs;

Both should be @nullable instead

Problem 2:
All Builder method return types should be annotated as @nonnull

Problem 3:
If those would be annotated as @nullable, the Builder.build() must copy values to locals and make it possible for compiler flow analysis to verify the null-correctness. Hence I would suggest this generation pattern:

    @Override
    public Animal build() {
      String missing = "";
// first copy to locals
      String name = this.name;
      Integer numberOfLegs = this.numberOfLegs;
// have all error paths with else-if
      if (name == null) {
        missing += " name";
      }
      else if (numberOfLegs == null) {
        missing += " numberOfLegs";
      }
      else {
// now in the else, all locals must be non-null
          return new AutoValue_Animal(
                  name,
                  numberOfLegs);
      }
      throw new IllegalStateException("Missing required properties:" + missing);
    }

Note:
Using @AutoValue.CopyAnnotation does not solve the problem either.
As this has problems as well:

• equals(Object) arg must be annotated with @nullable, or the method with @NonNullByDefault({})

[eamonnmcmanus]

Thanks for the very detailed analysis!

I think we'd be reluctant to add things to AutoValue just for Eclipse, but we are definitely interested in nullness analysis more generally, and the Eclipse annotations might be addressed as a special case of that.

I'm surprised that the Builder.name field gets the @NonNull annotation in your example. Is that because the Eclipse compiler reports the return type of Animal.getName() as @NonNull String due to the @NonNullByDefault annotation?

The question has arisen before, and I continue to claim that it is a bug for a nullness analyser not to know that the parameter to equals(Object) is always @Nullable. Regardless, it's a bit unclear to me what you are saying. This only causes a problem when @AutoValue.CopyAnnotations is present on the @AutoValue class? Is that because it copies @NonNullByDefault?

I like your suggestion for using local variables in the build() method. That should be easy enough, though we'd have to be slightly careful to avoid a conflict in case there is a property called missing. :-)

Meanwhile as a possible workaround for Eclipse it is apparently possible to disable warnings in generated code which might be appropriate until we have a better solution.

[eamonnmcmanus]

I've spent a fair amount of time investigating this, partly because we might also want AutoValue code to be null-safe as part of the JSpecify project. However, making it work with Eclipse specifically seems as if it would be hard.

In what follows, I used Eclipse JDT 4.19 for my experiments.

By far the largest problem is that Eclipse takes it on itself to add a fictitious @NonNull annotation to every type in the scope of a @NonNullByDefault annotation. That is, if an annotation processor asks for the type of a method parameter of type String, for example, and the package the method is in has @NonNullByDefault, then the Eclipse compiler will report that its type is @NonNull String. That is an egregious violation of the API specification, which says:

Returns the annotations that are directly present on this construct.

(italics in the original).

AutoValue faithfully copies type annotations everywhere the type is used, so this originally-String parameter will appear in the generated source code as @NonNull String. This is mostly not too bad, because we want to be able to deal with types that are explicitly annotated @NonNull anyway. We track whether properties are set in a builder by having one field per property, each of which starts off as null. For type correctness, then, these fields should be declared as @Nullable, and of course if the original type has @NonNull then that should be removed. So far so good.

But we run into lots of problems after that. The constructor parameters in the generated AutoValue_Foo class are all either annotated @Nullable (because the original abstract getter method returned @Nullable Something) or @NonNull (because Eclipse lied about the type having that annotation). Then when the constructor is called we get warnings like this:

        immutableListOfStrings,
        ^^^^^^^^^^^^^^^^^^^^^^
Null type safety (type annotations): The expression of type 'ImmutableList<String>' needs unchecked conversion to conform to '@NonNull ImmutableList<String>'

This despite carefully copying the value in question into a local variable in order to prove to Eclipse that it can't be null. The code in question is essentially this:

ImmutableList<String> immutableListOfStrings = this.immutableListOfStrings;
if (immutableListOfStringsBuilder$ != null) {
  immutableListOfStrings = immutableListOfStringsBuilder$.build();
} else if (immutableListOfStrings == null) {
  immutableListOfStrings = ImmutableList.of();
}

You might think that Eclipse can't know that these methods never return null, but I get the same warnings if I replace ImmutableList with a custom type that is compiled at the same time as my test and with the same scope of @NonNullByDefault, meaning that those methods end up returning implicit @NonNull.

I also get various warnings like this:

        @NonNull ImmutableList<String> immutableListOfStrings,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The nullness annotation is redundant with a default that applies to this location

Well yes, but the nullness annotation is there because you added it, Eclipse.

And I get this:

      if (immutableListOfStrings == null) {
        throw new NullPointerException("Null immutableListOfStrings");
      }
                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Dead code

This kind of makes sense, since this is in a method that is (thanks the inserted annotation) setImmutableListOfStrings(@NonNull ImmutableList<String> immutableListOfStrings). So indeed it can't be null, assuming everything is being checked for nullness at compile time. That's a big assumption, though.

So on the whole I think with the current state of the world it isn't feasible to have the @AutoValue.Builder code pass Eclipse's wonky nullness analysis, and the best we can do is to exclude generated code from this analysis, as mentioned earlier.

[frankbenoit]

I submitted this bug:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=572354

[frankbenoit]

While this may or maybe not a bug in JDT compiler....
If I add @NonNullByDefault it is a convenience thing, instead of attaching @NonNull everywhere manually. In fact, @NonNullByDefault can be attached to the package via package-info.java.

So what if the example is changed with @NonNull on every element?

@AutoValue
public abstract class Animal {

    @NonNull
    public abstract String name();

    @NonNull
    public abstract List<@NonNull String> friends();

    public abstract int numberOfLegs();

    @NonNull
    public static Builder builder() {
        return new AutoValue_Animal.Builder();
    }

    @AutoValue.Builder
    public abstract static class Builder {
        @NonNull
        public abstract Builder name(@NonNull String value);

        @NonNull
        public abstract Builder friends(@NonNull List<@NonNull String> value);

        @NonNull
        public abstract Builder numberOfLegs(int value);

        @NonNull
        public abstract Animal build();
    }
}

Now I would want the generated Build to look like this:

1. have the @NonNull removed from Builder fields. Probably have them set as @Nullable because this would be the correct annotation in the Builder.
2. have the @NonNull not removed from generic param types.
3. Change the setter null-check ifs to java.lang.Objects.requireNonNull, so they are not marked with warning due to redundancy.
4. Optional: Have boxed types use the explicit boxing/unboxing (Integer.valueOf, .intValue). In my projects, the compiler flags those as warning, if boxing/unboxing done implicitly
5. In build(): Have a prefix for local variables, e.g. 'local', and have the build() method use them.
6. In build(): Do not use the missing String to ensure all non-null, instead have ORed condition, so the compiler can understand those variable to be now non-null.

    static final class Builder extends Animal.Builder {
        private String name;
        private List<@NonNull String> friends;
        private Integer numberOfLegs;

        Builder() {
        }

        @NonNull
        @Override
        public Animal.Builder name(@NonNull final String name) {
            Objects.requireNonNull(name, "Null name");
            this.name = name;
            return this;
        }

        @NonNull
        @Override
        public Animal.Builder friends(@NonNull final List<@NonNull String> friends) {
            Objects.requireNonNull(name, "Null friends");
            this.friends = friends;
            return this;
        }

        @NonNull
        @Override
        public Animal.Builder numberOfLegs(final int numberOfLegs) {
            this.numberOfLegs = Integer.valueOf(numberOfLegs);
            return this;
        }

        @NonNull
        @Override
        public Animal build() {
            // prefix 'local' choosen by first testing if it collides with an existing element name
            final String localname = name;
            final List<@NonNull String> localfriends = friends;
            final Integer localnumberOfLegs = numberOfLegs;
            if ((localname == null) || (localfriends == null) || (localnumberOfLegs == null)) {
                String missing = "";
                if (localname == null) {
                    missing += " name";
                }
                if (localfriends == null) {
                    missing += " friends";
                }
                if (localnumberOfLegs == null) {
                    missing += " numberOfLegs";
                }
                throw new IllegalStateException("Missing required properties:" + missing);
            }
            return new AutoValue_Animal(
                    localname,
                    localfriends,
                    localnumberOfLegs.intValue());
        }
    }

[eamonnmcmanus]

As part of my experimental work I did in fact do several of those things:

• have the @NonNull removed from Builder fields. Probably have them set as @Nullable because this would be the correct annotation in the Builder.

  Yes, I did both of those.

• have the @NonNull not removed from generic param types.

  Nothing to do here. We never have any reason to modify type arguments.

• Change the setter null-check ifs to java.lang.Objects.requireNonNull, so they are not marked with warning due to redundancy.

  That's a good idea, which we might want to do anyway. The main wrinkle is that Objects.requireNonNull was introduced in Java 8 while AutoValue generates code that is compatible with Java 7. However we can detect whether java.util.Objects exists in the target environment and fall back on the existing code if not.

• Optional: Have boxed types use the explicit boxing/unboxing (Integer.valueOf, .intValue). In my projects, the compiler flags those as warning, if boxing/unboxing done implicitly

  I don't think we'll be doing that. You could always use @CopyAnnotations to copy the appropriate @SuppressWarnings into the generated code.

• In build(): Have a prefix for local variables, e.g. 'local', and have the build() method use them.

  I didn't find a prefix necessary; I just used shadowed the fields with local variables of the same name. (But now that I think about it, that might draw warnings too.) I also only copied fields into local variables when it was necessary. For example @Nullable fields don't need to be copied.

• In build(): Do not use the missing String to ensure all non-null, instead have ORed condition, so the compiler can understand those variable to be now non-null.

  Yes, I used code very similar to what you show. The drawback is that it's slightly more code, but environments that are that sensitive to code size should probably be using the (undocumented!) -Acom.google.auto.value.OmitIdentifiers option.

We can definitely do at least some of these, and probably will. But my impression after doing the existing experiments is that it's a lot of work just to get rid of warnings from generated code, which nobody should really care about anyway.

[cpovirk]

I just came across this issue while working on something else, and I figured I'd point out a few commits I know of that did parts of the work discussed above:

• 4506804
• 1b58cff
• fa809b2
• 91d5f32