AutoValue null annotations in Eclipse

[frankbenoit]

Hi,

in Eclipse there is a setting to ignore optional compile errors, that can be configured for the generated sources.
In my projects those are always on, because AutoValue generates problems.

What cause the problems:
See the attached example...
AutoValueNNBD.zip
In this minimal example class, the generated code has 4 errors and 5 warnings.

In Eclipse (JDT) there are 3 null annotations: @NonNull, @Nullable and @NonNullByDefault
The @NonNullByDefault can be put onto field/method/class or event the package. Like in this example I have put it into the package-info.java.

In the example, I created a AutoValue_KeyValueProposal.java, to illustrate how it should look like from my POV to make it compile error free.

1. Local variable instead of accessing the field twice for null-check then use
2. All builder field to be @Nullable
3. No @NonNull ever if the @NonNullByDefault is active

regards
Frank

[eamonnmcmanus]

Hi Frank!

You brought up many of the same issues in #977. Some things have changed since then, but some haven't. One thing I notice that hasn't changed is the Eclipse bug where it synthesizes a @NonNull annotation on every unannotated type in the scope of @NonNullByDefault, in flagrant violation of the spec. You logged an Eclipse bug about this in March 2021 but there has been no activity on it.

One thing that has changed is that we attempt to discover if there is a @Nullable type-use annotation in the code, by looking at parameter and return types of methods in the @AutoValue class, or by consulting a compiler option. If so then we will apply that @Nullable to the parameter of the generated equals method and to the type of each builder field that is not a primitive. For Eclipse, if your class doesn't already have @Nullable in its methods, you could either compile with -Acom.google.auto.value.NullableTypeAnnotation=org.eclipse.jdt.annotation.Nullable or you could add a dummy equals method like this to your AutoValue class:

  @Override public abstract boolean equals(@Nullable Object o);

Then AutoValue will discover and use that @Nullable.

We could indeed copy every property field into a local variable, or perhaps just every field for a property that is neither primitive nor @Nullable. I'll note as an aside that there are two approaches to null checking here, which we might call optimistic and pessimistic. The optimistic approach assumes that if a method has observed a field being non-null then it will remain that way for the duration of the method. The pessimistic approach assumes that even if a field was observed to be non-null, some other thread could have come along and set it back to null in the meantime. Eclipse is pessimistic, which here is counterproductive because the non-null value can't in fact be set back to null: the setFoo methods all have null guards.

Doing this would close off another path, of tracking all non-@Nullable properties with bitmasks, and then using a single bitmask check to ensure they are all set. We already do this for primitive properties, and we do it in AutoBuilder for all properties when building Kotlin classes with default constructor properties.

The other main issue concerns @NonNull. It is exacerbated by the Eclipse bug, but it would still be present anyway: users might annotate their @AutoValue properties with @NonNull, and then that annotation would be copied to the builder fields, yielding private @Nullable @NonNull bar;. We could imagine a special check that would remove the @NonNull here.

I believe you're also saying that the Eclipse compiler issues a warning if you use @NonNull inside @NonNullByDefault. So we get that warning on the generated setBar methods, because the cited Eclipse bug makes it look as if the annotation was there in the source, leading AutoValue to copy it. I don't expect we will try to compensate for this bug.

So, to summarize:

• We probably should subtract @NonNull from the type of builder property fields.
• We might introduce local variables so pessimistic nullness analysis can see that the constructor invocation is null-safe.
• We won't go looking for Eclipse's @NonNullByDefault to work around the buggy behaviour it induces.

[cpovirk]

(@eamonnmcmanus , I'm giving this a priority to satisfy our triage process, but please adjust as appropriate.)