Extensibility API for AutoValue

[eamonnmcmanus]

Issues #87, #162, and #201 are examples of use cases that are a bit too specialized to put into the core of AutoValue, but which are still important when they apply. We should define an extensibility API so that users can build extensions that run at the same time as AutoValue. The general idea would be that there would be an interface, say AutoValueExtension, and that the AutoValue processor would use ServiceLoader to find implementations of this interface. So if you include on your -processorpath a jar that contains such an implementation, AutoValue will invoke it when it runs.

A possible sketch of the interface is this:

public interface AutoValueExtension {
  interface Context {
    ProcessingEnvironment processingEnvironment();
    String packageName();
    TypeElement autoValueClass();
    Map<String, ExecutableElement> properties();
  }

  boolean applicable(Context context);

  String generateClass(Context context, String classToExtend, String classToImplement);
}

The idea is that every extension gets its applicable method called, and those that return true get their generateClass method called. That method can return a String that is an implementation of the class called classToImplement that extends the class called classToExtend; or it can return null (for example if it has generated some related code but doesn't need to intervene in the AutoValue hierarchy). So if you have two extensions, @AutoValue class Foo might result in a hierarchy AutoValue_Foo extends $AutoValue_Foo extends $$AutoValue_Foo extends Foo, where AutoValue_Foo is generated by one of the extensions, $AutoValue_Foo is generated by the other, and $$AutoValue_Foo contains the logic generated by the AutoValue processor. This means that extensions can override the implementations of toString() etc.

[PhilippWendler]

As far as I understand, this would allow me to customize the object creation, too, because I can insert code into the constructor of the class that my extension generates, correct? This would be good.

The use case I have is that I want to automatically create an @AutoValue instance from something that wraps a Map<String, String>. We use something like this for configuration options (currently based on reflection instead of AutoValue).

So my constructor would not have parameters for all properties but only for the map.
In the constructor, I could retrieve the value for each property from the map, convert it to the expected type, and pass them to the super constructor. The down side would be that yet another class gets created that is actually not necessary if I could intercept the passing of values from the factory method to the AutoValue-generated constructor somehow without it (because my subclass would not add or override any methods, only the constructor).

[rharter]

I've started playing around with implementing this feature, and one thing that I've come across is this error:

@AutoValue classes cannot have abstract methods other than property getters and Builder converters

In my implementation, I'm creating an auto-extension that implements Android Parcelable, but the fact that my class implements an interface means there are extra abstract methods (from the interface).

It seems reasonable to me that other extensions might also want to add abstract methods that they can act on, so the question is, should this error simply go away? The problem with that is that you then lose the compile time error reporting in the case where there are any abstract methods that a generator didn't implement.

One possible solution I can see is to record all abstract methods on the annotated class, track which ones have been implemented in the processing step, and throw an error if there are any abstract methods not implemented by autovalue or it's extensions. Thoughts?

[eamonnmcmanus]

Good point. It would make sense for the extension API to include a way for extensions to see and claim abstract methods. Then the error above would be produced only after extensions have run if there are still methods that were not claimed by any of them.

[kevinb9n]

(Does it keep things simpler to just not care, and let the generated
concrete class just fail to compile if something is missing?)

On Mon, May 4, 2015 at 7:56 AM, Éamonn McManus notifications@github.com
wrote:

Good point. It would make sense for the extension API to include a way for
extensions to see and claim abstract methods. Then the error above would be
produced only after extensions have run if there are still methods that
were not claimed by any of them.

—
Reply to this email directly or view it on GitHub
#202 (comment).

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

[rharter]

That's also a fair point. Since the unclaimed methods are abstract, there
would still be a compile time error, so is this error message much more
helpful than that, or are we just duplicating compiler functionality there?

On Mon, May 4, 2015, 10:02 AM kevinb9n notifications@github.com wrote:

(Does it keep things simpler to just not care, and let the generated
concrete class just fail to compile if something is missing?)

On Mon, May 4, 2015 at 7:56 AM, Éamonn McManus notifications@github.com
wrote:

Good point. It would make sense for the extension API to include a way
for
extensions to see and claim abstract methods. Then the error above would
be
produced only after extensions have run if there are still methods that
were not claimed by any of them.

—
Reply to this email directly or view it on GitHub
#202 (comment).

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

—
Reply to this email directly or view it on GitHub
#202 (comment).

[eamonnmcmanus]

I should note that the message in question is actually a warning rather than an error, and does not in itself cause compilation to fail. The reason for that is that the unhelpful Eclipse compiler sometimes fails to realize that a concrete method overrides an abstract one.

The philosophy has been for AutoValue never to generate code that doesn't compile, unless it is accompanied by at least one error message that points to the area in the source code where the problem was. That's particularly important for IDE integration, because you really want an error message that is attached to the source you are looking at, so it can be redlined.

[kevinb9n]

Ah, got it.

On Mon, May 4, 2015 at 8:14 AM, Éamonn McManus notifications@github.com
wrote:

I should note that the message in question is actually a warning rather
than an error, and does not in itself cause compilation to fail. The reason
for that is that the unhelpful Eclipse compiler sometimes fails to realize
that a concrete method overrides an abstract one.

The philosophy has been for AutoValue never to generate code that doesn't
compile, unless it is accompanied by at least one error message that points
to the area in the source code where the problem was. That's particularly
important for IDE integration, because you really want an error message
that is attached to the source you are looking at, so it can be redlined.

—
Reply to this email directly or view it on GitHub
#202 (comment).

Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com

[rharter]

Those are some really good points, @eamonnmcmanus.

I've got a start on the extensibility support. Would you mind taking a look at my branch and tell me if you think I'm on the right track. This is sort of in the POC stage right now and could use a little bit of cleanup (and still has problems with builders), but I just wanted to make sure I'm on the right track before moving too far.

I wasn't really sure how to test the extensions, so I added a constructor so that I could inject the extensions. I left it package private so it won't be a part of the public API, but perhaps there is a better way to do this.

[eamonnmcmanus]

@rharter, I had a quick look through your branch and it does look pretty much like what I had in mind. I don't have time right now to do a more thorough review but I hope to be able to get to it within the next week. Please nag me if you don't hear from me soon.

[rharter]

After discussion with @eamonnmcmanus, I think we've come up with a working final architecture. I'm documenting it here.

In the new architecture the AutoValue generated class will be the only direct subclass of the @AutoValue class Foo class, while each extension will have the opportunity to generate a subclass of that. The classes will have $ prepended to their name accordingly.

@AutoValue class Foo {
  public abstract String bar();
}

@Generated(AutoValue.class)
class $$AutoValue_Foo extends Foo {
  public String bar() {...}
  @Override public String toString() {...}
}

@Generated(CustomToStringExtension.class)
class $AutoValue_Foo extends $$AutoValue_Foo {
  @Override public String toString() {...}
}

@Generated(ParcelableExtension.class)
class AutoValue_Foo extends $AutoValue_Foo {
  // parcelable bits...
}

The special case here is that for any given @AutoValue class, there can only be one extends that requires itself be at the end of this chain. In the case of Android Parcelable, a static factory property is required on the implementing class, so it must remain at the end of the chain.

A couple of questions I have for this architecture:

• How to handle the Builder. Normally, a user can create a static builder() method that will return new AutoValue_Foo.Builder(), but we can no longer guarantee that the builder is available at that point in the chain, since it might be $$AutoValue.Builder().

[cgruber]

Ok - not going to lie, the million superclasses doesn't make me feel very
happy, but I think it's not the worst thing in the world.

Having only rudimentarily scanned the solution, has any consideration been
given to possible ordering needs of the extensions?

On Mon, 1 Jun 2015 at 15:43 Ryan Harter notifications@github.com wrote:

After discussion with @eamonnmcmanus https://github.com/eamonnmcmanus,
I think we've come up with a working final architecture. I'm documenting it
here.

In the new architecture the AutoValue generated class will be the only
direct subclass of the @autovalue class Foo class, while each extension
will have the opportunity to generate a subclass of that. The classes will
have $ prepended to their name accordingly.

@autovalue class Foo {
public abstract String bar();
}

@generated(AutoValue.class)
class $$AutoValue_Foo extends Foo {
public String bar() {...}
@OverRide public String toString() {...}
}

@generated(CustomToStringExtension.class)
class $AutoValue_Foo extends $$AutoValue_Foo {
@OverRide public String toString() {...}
}

@generated(ParcelableExtension.class)class AutoValue_Foo extends $AutoValue_Foo {
// parcelable bits...
}

The special case here is that for any given @autovalue class, there can
only be one extends that requires itself be at the end of this chain. In
the case of Android Parcelable, a static factory property is required on
the implementing class, so it must remain at the end of the chain.

A couple of questions I have for this architecture:

• How to handle the Builder. Normally, a user can create a static
  builder() method that will return new AutoValue_Foo.Builder(), but we
  can no longer guarantee that the builder is available at that point in the
  chain, since it might be $$AutoValue.Builder().

—
Reply to this email directly or view it on GitHub
https://github.com/google/auto/issues/202#issuecomment-107738976.

[rharter]

The extent of consideration for the order if extensions has been that one can require itself be the final class in the chain, but that's about it.

The current solution places the AutoValue generated class as the basest of the concrete base classes, and allows a single extension to require its subclass be the final implementation.

Are you suggesting allowing some extensions to be dependent on others?

[eamonnmcmanus]

I think builders should Just Work? The code generated for build() today calls the AutoValue_Foo constructor, which it should continue to do, though AutoValue_Foo will no longer be the AutoValue implementation but will be the last extension. User code that does new AutoValue_Foo.Builder() will instantiate the Builder class inherited from $$AutoValue_Foo or whatever.

[cgruber]

I'm not precisely "suggesting" it, so much as asking how much consideration
has been given to the question, since I can imagine extensions that need to
happen after others.

I know we are going to face this in Dagger, since some processor extensions
add to the analysis, some generate, and some do both, and these may need
the information form other extensions into account. Like a visualizer
would need to come after anything that mutates the graph.

AutoValue is a smaller problem, but I have a hunch that people will find
ways to push this.

On Mon, 1 Jun 2015 at 17:43 Éamonn McManus notifications@github.com wrote:

I think builders should Just Work? The code generated for build() today
calls the AutoValue_Foo constructor, which it should continue to do,
though AutoValue_Foo will no longer be the AutoValue implementation but
will be the last extension. User code that does new
AutoValue_Foo.Builder() will instantiate the Builder class inherited from
$$AutoValue_Foo or whatever.

—
Reply to this email directly or view it on GitHub
#202 (comment).

[eamonnmcmanus]

I can imagine some sort of ordering constraint, of which the must-be-lowest constraint would be a special case. But I propose that we should make an API without one first, if only because I don't see a very clean way of doing it while maintaining the property that extensions are independent of each other and mostly opaque to each other too.

[cgruber]

SGTM.

On Mon, 1 Jun 2015 at 17:59 Éamonn McManus notifications@github.com wrote:

I can imagine some sort of ordering constraint, of which the
must-be-lowest constraint would be a special case. But I propose that we
should make an API without one first, if only because I don't see a very
clean way of doing it while maintaining the property that extensions are
independent of each other and mostly opaque to each other too.

—
Reply to this email directly or view it on GitHub
#202 (comment).

[rharter]

It sounds like making the builder Just Work (which I completely agree with) adds a few rules to which extensions must adhere.

1. Extensions must not add additional constructors, unless they also provide the AutoValue generated constructor. (So that the Builder can call AutoValue_Foo(...) appropriately)
2. The Builder class must not be static, so that it can be inherited and referred to as AutoValue_Foo.Builder.

Does that sound about right?

[eamonnmcmanus]

Rule 1 looks necessary. Rule 2 doesn't, though. Static classes are inherited, so new AutoValue_Foo.Builder() will work even if the Builder class is static and defined in an ancestor of AutoValue_Foo.

[akanter]

Aside from ordering, what other validation concerns would we need to address? Certainly the API could include a validate(AutoValueExtensionContext). For the ordering issue specifically, the actual application of the extensions could happen through a wrapper processor like @OrderedAutoValueExtender({Extension1.class, Extension2.class,...}).

Then it just becomes a matter of determining how much contextual information to give to each Extension. I imagine Context#hasNext (whether there are more extensions to deal with) and Context#implements(Class) (to verify if the end-result object implements a certain interface) could be useful. I could potentially even see a map of properties that could be set to allow limited amounts of context to be shared between extensions (uni-directionally, potentially namespaced) if there was a strong usecase for it.

[eamonnmcmanus]

The idea of a wrapper extension is intriguing. Since the plan is for AutoValue to find extensions using ServiceLoader, it would need to have a way to preempt that if there is a MetaAutoValueExtension present (itself loaded through ServiceLoader). However, again, I think we should postpone thinking about this level of complexity at first.

[rharter]

@eamonnmcmanus I've got this just about ready to go into a PR, just have to add some documentation.

[rharter]

One thing I'm wrestling with now is the idea that all generated classes (AutoValue generated and Extension generated) will be abstract, and the final one will be final. This allows the annotated class to declare an interface that and extension will fulfill.

The problem here is that the extension classes then have to mirror the constructor of the AutoValue generated class, since it will be abstract and there is no default constructor. This seems unfortunate to me, as every extension will have to mirror some functionality of AutoValue, but I'm not sure of a better way. Any ideas @eamonnmcmanus?

You can check out where things are here: https://github.com/rharter/auto/commits/extensibility

[eamonnmcmanus]

I think part of the contract of an extension is that the class it generates must have a constructor whose parameters correspond to Map<String, ExecutableElement> properties();. Since user code can write new AutoValue_Foo(p1, p2) I don't see how that can be avoided.

[rharter]

Well my initial thought was that you would just not add a constructor and it would inherit one, but that doesn't work if the AutoValue class is abstract. The problem with just stepping through the keys, I believe, is that in the constructor the user can write order is important. Map doesn't guarantee order of it's keys, right? Perhaps if we used a SortedMap?

[eamonnmcmanus]

Constructors aren't inherited. Map iteration order is unspecified in general but we would obviously want to specify that the properties map is iterated in the correct order. (In practice it would be a LinkedHashMap or an ImmutableMap, both of which iterate in the order the entries were added.)

[rharter]

Okay, I'll just make that a requirement and get this in.

[rharter]

I know it's kind of late to bring this up, but does anyone else think boolean mustBeAtEnd(Context context); is a stupid name for a method? Initially I was going to call it mustBeFinal but thought it might be confused with the final modifier as opposed to the final class in the chain, but now that I'm working with it, everything else in the chain should be abstract and the final class should actually be final, so I don't think that's an issue anymore.

[eamonnmcmanus]

Yes, I think that would be a better name. I'd be fine with just changing it.

[cgruber]

PR #237 created all sorts of practical problems in our build inside google, and probably shouldn't have been put in the com.google.auto.value package, but maybe in a com.google.auto.value.processor.api package or something. :( I can fix the build issues, and that's not THAT big a deal, but I actaully don't think this should be in the same package as @AutoValue. It's not public api, it's processor api - an SPI (service provider interface, if you will).

Thoughts? Can we consider moving it there?

[JakeWharton]

How about com.google.auto.value.processor.extension?

On Fri, Aug 14, 2015 at 11:23 PM Christian Edward Gruber <
notifications@github.com> wrote:

PR #237 #237 created all sorts of
practical problems in our build inside google, and probably shouldn't have
been put in the com.google.auto.value package, but maybe in a
com.google.auto.value.processor.api package or something. :( I can fix the
build issues, and that's not THAT big a deal, but I actaully don't think
this should be in the same package as @autovalue. It's not public api,
it's processor api - an SPI (service provider interface, if you will).

Thoughts? Can we consider moving it there?

—
Reply to this email directly or view it on GitHub
#202 (comment).

[cgruber]

SGTM.

[eamonnmcmanus]

Sounds good to me too. Since I'm on vacation, perhaps @cgruber can take care of it?

[cgruber]

Shall do. I expected to have to wait to hear from you until you were done
vacating. Go enjoy! Stop internetting for work purposes!

On Sat, 15 Aug 2015 at 08:54 Éamonn McManus notifications@github.com
wrote:

Sounds good to me too. Since I'm on vacation, perhaps @cgruber
https://github.com/cgruber can take care of it?

—
Reply to this email directly or view it on GitHub
#202 (comment).

[rharter]

Now that we have our RC1, do you think this is completed and can be closed?

[eamonnmcmanus]

Yes. As you know, there's further work to be done on the API. I'm not forgetting the work you have already done, but I'd like to get 1.2 out before integrating it.