AutoValue: Support for extending a class.

[dballesteros7]

Suppose I have a set of base arguments that I need to share with two classes, and I define the following base class.

abstract class BaseArgs {
  abstract Object getFoo();
  abstract Object getBar();

  abstract static class Builder<T extends Builder<T>>{
    abstract T setFoo(Object foo);
    abstract T setBar(Object bar);
  }
}

Now I want to add baz for class A and qux for class B.

@AutoValue
abstract class ArgsA extends BaseArgs {
  abstract Object getBaz();

  @AutoValue.Builder
  abstract static class Builder extends BaseArgs.Builder<Builder> {
    abstract Builder setBaz(Object baz);

    abstract build();
  }
}

And similarly for ArgsB. However this fails with: Setter methods must return ArgsA.Builder, despite the generic annotation.

Is this something sane to do? Could it be fixed in the way the types are being compared to support the generic annotation? I could have optional parameters in BaseArgs but I would like to keep separate definitions of these arguments, and reuse any validations in the base class.

For now my only solution is to rewrite the builder setters in each of the two "concrete" classes.

[jbgi]

See also #269
aka: you should use composition instead of inheritance.

[eamonnmcmanus]

Yes, this is a known restriction. The code example above should work but does not.

[dballesteros7]

@jbgi I usually agree with composition over inheritance but in this case all three classes are on the same package and I don't want to define an interface but rather make sure that it is clear where ArgsA and ArgsB differ for each specialized usage, by class A and B. It is cumbersome to have to always do something like ArgsA.getCommonArgs().getTheActualArgIWant() instead of being able to do ArgsA.getTheActualArgIWant().

Thanks @eamonnmcmanus, I think the use of a Builder<T> is common (accompanied by reflection, which autovalue save us from) and it would be great to have this working.

[jbgi]

@dballesteros7 not always, just once:

class ArgsA {
   ...
   abstract CommonArgs getCommonArgs();

   TheActualArgIWant getTheActualArgIWant{
       return getCommonArgs().getTheActualArgIWant();
   }
   ...
}

[dballesteros7]

But then I would be re-writing a method for every arg from CommonArgs that I want exposed in ArgsA, something that inheritance gives me for free.

[jbgi]

yes. for free. as well as headaches as soon as your domain model diverges from being a nice hierarchy.

[jbgi]

What I mean is that inheritance may be fine in some isolated cases like yours but I'd rather not see support for it in auto-value, which would promote this "bad" practice.

[PhilippWendler]

Is inheritance for value types really such a bad idea if the super class is abstract? As far as I remember Effective Java (I don't have it at hand), this is fine and problems only arise if there exist instance of both the super class and sub classes (because then for example equals() cannot be implemented sanely).

[eamonnmcmanus]

Having an ancestor shared between different @AutoValue classes is definitely a supported use case. We don't allow one @AutoValue class to extend another, though, for the reasons @PhilippWendler alludes to.

[jbgi]

@PhilippWendler @eamonnmcmanus @dballesteros7 Algebraic Data Types are a more flexible solution than inheritance for this kind of problem. Have a look at https://github.com/derive4j/derive4j to implement them in Java easily.

[eamonnmcmanus]

@jbgi very nice!

[facboy]

@eamonnmcmanus I think the constructors are not generated correctly when an ancestor class is used in 1.1, or at least the behaviour is unintuitive. In the docs it states that the generated constructor should match the order that the abstract accessors are declared in, but if i have

public abstract class BaseArgs {
    public abstract Object getFoo();
    public abstract Object getBar();
}

and

@AutoValue
public abstract class SameArgs extends BaseArgs {
}

...

@AutoValue
public abstract class MoreArgs extends BaseArgs {
    public abstract Object getMoreFoo();
}

The generated constructor I get is

AutoValue_MoreArgs(
      Object bar,
      Object foo,
      Object moreFoo) {
    if (bar == null) {
      throw new NullPointerException("Null bar");
    }
    this.bar = bar;
    if (foo == null) {
      throw new NullPointerException("Null foo");
    }
    this.foo = foo;
    if (moreFoo == null) {
      throw new NullPointerException("Null moreFoo");
    }
    this.moreFoo = moreFoo;
  }

which doesn't match what I would expect. Shouldn't it be AutoValue_MoreArgs(foo, bar, moreFoo)? Or alternatively AutoValue_MoreArgs(moreFoo, foo, bar)?

I think I've seen it randomly change the order of the arguments too (ie there were no changes to BaseArgs but the order of foo and bar in AutoValue_MoreArgs() changed when the class was re-generated).

I'm not sure whether having a child that adds no additional accessors (SameArgs) is necessary to reproduce, but that is the structure that I was using at the time.

[eamonnmcmanus]

I tried using those exact classes and I see the constructor with parameters in the order I would expect: foo, bar, moreFoo. Are you using Eclipse by any chance? We've had problems with parameter ordering there because it didn't follow the annotation processing spec. Those should be fixed as of Eclipse 4.5, though.

[facboy]

Intellij...with the eclipse compiler actually, i think, so that would make sense.

[eamonnmcmanus]

IntelliJ uses javac by default but can be configured to use ECJ (the Eclipse compiler).
https://confluence.jetbrains.com/display/IntelliJIDEA/Compiler
Personally I would recommend against that because ECJ is not very well maintained.

[facboy]

True, I use javac for 'real' builds but for IDE dev work ECJ is quite convenient.

[eamonnmcmanus]

Feel free to log a separate bug about the behaviour you are seeing. If you have a way of knowing which ECJ version IntelliJ is using that would be very helpful. With versions prior to the one shipped with Eclipse 4.5, there was a bug where methods were sorted into alphabetical order within each class, instead of the source-code order. That looks like what you are seeing; you could try some experiments to confirm. AutoValue has code to try to undo the sorting, but it's possible that in the IntelliJ/ECJ situation it doesn't work for some reason.

[facboy]

OK I raised #292.

[heutschibogdan]

@eamonnmcmanus any update on this? Is it planned to be implemented or not?

[eamonnmcmanus]

It's still on my list, and has moved nearer to the top after finishing a number of other tasks that had been languishing (coming soon to a PR near you). I don't know when I'll get to it, though.

FTR fixing it is not completely straightforward because of a longstanding bug in the Eclipse compiler. I was able to work around that bug in another context, though, so it should be doable.

[heutschibogdan]

@eamonnmcmanus sorry but I wasn't clear, I was asking about allowing a builder to extend another class. Do you know anything about this?

[eamonnmcmanus]

A fix is in the works that should allow the example at the top of this issue to compile. Meanwhile, it is always possible to override the inherited methods in the final Builder subclass with their explicit types. So in the original example you could declare Builder setFoo(Object foo) etc.

Were you asking about something else?

[heutschibogdan]

This is what I was asking, I know you can override, but in my case I have common base class for like 20-30 other classes and it has around 10 fields and this will add a big overhead, also it will create a big overhead for maintaining. Thank you for the answer. If it is possible, do you know an estimation when the fix would be finished?

[eamonnmcmanus]

I'd expect it to be in 1.3-SNAPSHOT within the next week.

[anicolas]

As hinted by @eamonnmcmanus, this issue was indeed fixed in the 1.3 release (see commit c689fc9) and can be closed.

Thanks for the hard work !

[eamonnmcmanus]

This issue became a little confused, with different topics being discussed, but I think we can indeed consider it closed.