Builders should not enforce null checks on primitive types

[petedoyle]

Related to #254

It looks like builders internally turn primitives into their boxed versions (i.e. double -> Double). Because primitives cannot have @Nullable (#254), this leads to null checks on primitive types, such that failing to call, e.g. Builder.setLatitude(double latitude) will lead to Missing required properties: latitude. It'd be nice to have primitive types default to their default values (e.g. 0.0 for a double).

We're currently working around this by declaring everything as boxed types (e.g. Double) that are @Nullable, but that makes client code messy since we have to check for null everywhere.

Example Auto Value class:

import android.support.annotation.Nullable;

import com.google.auto.value.AutoValue;
import com.google.gson.annotations.SerializedName;

import org.threeten.bp.ZonedDateTime;

@AutoValue
public abstract class GpsRecord {
    @SerializedName("latitude") public abstract double getLatitude();  // primitive 'double'
    @Nullable @SerializedName("longitude") public abstract Double getLongitude(); // 'Double' object
    @Nullable @SerializedName("timestamp") public abstract ZonedDateTime getTimestamp();

    /** Makes a new instance using a builder */
    public static Builder builder() {
        return new AutoValue_GpsRecord.Builder();
    }

    /** Makes a copy of this instance to a builder for further editing */
    public abstract Builder toBuilder();

    @AutoValue.Builder
    public abstract static class Builder {
        public abstract Builder latitude(double latitude);
        public abstract Builder longitude(Double longitude);
        public abstract Builder timestamp(ZonedDateTime timestamp);
        public abstract GpsRecord build();
    }
}

Generated class:

import android.support.annotation.Nullable;
import com.google.gson.annotations.SerializedName;
import javax.annotation.Generated;
import org.threeten.bp.ZonedDateTime;

@Generated("com.google.auto.value.processor.AutoValueProcessor")
 final class AutoValue_GpsRecord extends GpsRecord {

  private final double latitude;
  private final Double longitude;
  private final ZonedDateTime timestamp;

  private AutoValue_GpsRecord(
      double latitude,
      @Nullable Double longitude,
      @Nullable ZonedDateTime timestamp) {
    this.latitude = latitude;
    this.longitude = longitude;
    this.timestamp = timestamp;
  }

  @SerializedName(value = "latitude")
  @Override
  public double getLatitude() {
    return latitude;
  }

  @Nullable
  @SerializedName(value = "longitude")
  @Override
  public Double getLongitude() {
    return longitude;
  }

  @Nullable
  @SerializedName(value = "timestamp")
  @Override
  public ZonedDateTime getTimestamp() {
    return timestamp;
  }

  @Override
  public String toString() {
    return "GpsRecord{"
        + "latitude=" + latitude + ", "
        + "longitude=" + longitude + ", "
        + "timestamp=" + timestamp
        + "}";
  }

  @Override
  public boolean equals(Object o) {
    if (o == this) {
      return true;
    }
    if (o instanceof GpsRecord) {
      GpsRecord that = (GpsRecord) o;
      return (Double.doubleToLongBits(this.latitude) == Double.doubleToLongBits(that.getLatitude()))
           && ((this.longitude == null) ? (that.getLongitude() == null) : this.longitude.equals(that.getLongitude()))
           && ((this.timestamp == null) ? (that.getTimestamp() == null) : this.timestamp.equals(that.getTimestamp()));
    }
    return false;
  }

  @Override
  public int hashCode() {
    int h = 1;
    h *= 1000003;
    h ^= (Double.doubleToLongBits(this.latitude) >>> 32) ^ Double.doubleToLongBits(this.latitude);
    h *= 1000003;
    h ^= (longitude == null) ? 0 : this.longitude.hashCode();
    h *= 1000003;
    h ^= (timestamp == null) ? 0 : this.timestamp.hashCode();
    return h;
  }

  @Override
  public GpsRecord.Builder toBuilder() {
    return new Builder(this);
  }

  static final class Builder extends GpsRecord.Builder {
    private Double latitude;  //  <-- generated as 'Double', originally declared 'double'
    private Double longitude;
    private ZonedDateTime timestamp;
    Builder() {
    }
    Builder(GpsRecord source) {
      this.latitude = source.getLatitude();
      this.longitude = source.getLongitude();
      this.timestamp = source.getTimestamp();
    }
    @Override
    public GpsRecord.Builder latitude(double latitude) { // <- primitive, as declared
      this.latitude = latitude; //  <-- autoboxing occurs here (double -> Double)
      return this;
    }
    @Override
    public GpsRecord.Builder longitude(@Nullable Double longitude) {
      this.longitude = longitude;
      return this;
    }
    @Override
    public GpsRecord.Builder timestamp(@Nullable ZonedDateTime timestamp) {
      this.timestamp = timestamp;
      return this;
    }
    @Override
    public GpsRecord build() {
      String missing = "";
      if (latitude == null) { // <-- Null check on what was declared as a primitive 'double'
        missing += " latitude";
      }
      if (!missing.isEmpty()) {
        throw new IllegalStateException("Missing required properties:" + missing);
      }
      return new AutoValue_GpsRecord(
          this.latitude,
          this.longitude,
          this.timestamp);
    }
  }

}

[petedoyle]

The above is just an example (latitude as double, longitude as Double). Useful for seeing what code gets generated.

[petedoyle]

Seems to only affect the Builder

[eamonnmcmanus]

This is deliberate. Sometimes it might make sense for primitives to default to their "zero" values, and sometimes it won't. Since we can't guess, we require the value to be specified. It's easy to make the default explicit, like this:

    public static Builder builder() {
        return new AutoValue_GpsRecord.Builder()
            .latitude(0);
    }

[petedoyle]

Awesome, I think that makes sense. Checking now.

Thank you for the quick reply!

[petedoyle]

Yes, that get's the job done, like a boss. Thank you!

[petedoyle]

Entirely unrelated... Is there a way to make certain generated fields transient?

Wanting to keep Gson from serializing a few fields (and the gson @Expose(serialize = false, deserialize = false) annotation doesn't work on methods, e.g. this won't work:

@AutoValue
public abstract class MyModelClass {
    @Expose(serialize = false, deserialize = false)
    public abstract long databaseId();

    // other fields parsed via gson:
    @SerializedName("first_name")
    public abstract String firstName();

    @SerializedName("last_name")
    public abstract String lastName();
}

[eamonnmcmanus]

Possibly https://github.com/rharter/auto-value-gson could help? Paging @rharter.

[nikclayton]

@eamonnmcmanus -- any objection to expanding on this (the autoboxing) in the Builders howto? Something like:

*   ... [use *primitive types* as buildable properties?](#primitives)

...

## <a name="primitive"></a>... use primitive types as buildable properties?

AutoValue supports primitive types as properties, but the generated builder class uses
the corresponding object wrapper classes. The default Builder behaviour is to check
that all the properties are non-null, and this will cause the `build` method to throw an
exception if you do not provide a default value.

For example, if you had declared a boolean property like so:

<pre>
@AutoValue
abstract class Foo {
  abstract boolean someProperty()

  static Builder builder() {
    return new AutoValue Foo.Builder();
  }

  @AutoValue.Builder
  abstract static class Builder {
    abstract Builder someProperty(final boolean someProperty);
    abstract Foo build();
  }
}
</pre>

then you might expect that `Foo.builder().build().someProperty();` would return false.
Instead this throws an exception.

You must explicitly specify the desired default value in the `builder()` method, as shown in
the [specify a **default** value for a property?](#default) question.

At the moment it's a parenthetical comment that doesn't show up with GitHub's search for "primitive", so it might help to make it slightly more prominent.

[eamonnmcmanus]

I'm not sure what you mean by doesn't show up with GitHub's search for "primitive"?

[Jintin]

This is deliberate. Sometimes it might make sense for primitives to default to their "zero" values, and sometimes it won't. Since we can't guess, we require the value to be specified. It's easy to make the default explicit, like this:

Hi, isn't it a bit too verbose for a primitives? If the builder also use the same primitive type the issue wouldn't exist? Null for a primitive type is wired since we expect the default from that primitive as Java original does, adding the default value in builder is also a like a workaround thing for me. Auto value probably don't have to guess if it's an error or not. It's an old issue though so if there is nothing gonna change is there any way we can opt-out of the rule? Otherwise the migration from older to newer will bump into unexpect crash in our case. Thanks. ^^

[eamonnmcmanus]

the migration from older to newer will bump into unexpect crash in our case

I guess you mean migrating from code that doesn't use AutoValue to code that does?

There are a few things you could do to avoid problems:

1. Carefully scan each migrated @AutoValue.Builder looking for primitive values being set, and ensure that those values are explicitly given defaults in the builder() method, as described above.

2. Use a class like the one below to apply these default values through reflection. The idea is that, in the builder() method, instead of return new AutoValue_Foo.Builder(); you write return Defaulter.defaultPrimitives(Builder.class, new AutoValue_Foo.Builder());

3. Write an AutoValue extension that subclasses the generated AutoValue_Foo class (called $AutoValue_Foo or the like when there are extensions) and the generated $AutoValue_Foo.Builder. The constructor in the AutoValue_Foo.Builder class that it generates can then call the various setBar(...) methods with default values.

---

public class Defaulter {
  private static final Map<Class<?>, Object> DEFAULT_VALUES =
      Map.of(boolean.class, false, char.class, '\0');

  public static <T> T defaultPrimitives(Class<T> type, T instance) {
    for (Method m : type.getDeclaredMethods()) {
      Class<?>[] parameters = m.getParameterTypes();
      if (Modifier.isAbstract(m.getModifiers())
          && parameters.length == 1
          && parameters[0].isPrimitive()) {
        m.setAccessible(true);
        Class pType = parameters[0];
        Object defaultValue = DEFAULT_VALUES.getOrDefault(pType, (byte) 0);
        try {
          m.invoke(instance, defaultValue);
        } catch (Exception e) {
          String err = String.format(
              "Calling %s with %s (%s) failed", m, defaultValue, defaultValue.getClass().getName());
          throw new RuntimeException(err, e);
        }
      }
    }
    return instance;
  }
}

[Jintin]

@eamonnmcmanus Thanks for your help