Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensibility #237

Merged
merged 20 commits into from Aug 7, 2015
Merged

Extensibility #237

merged 20 commits into from Aug 7, 2015

Conversation

@rharter
Copy link
Contributor

@rharter rharter commented May 15, 2015

This is a first pass implementation of issue #202, providing two options for extending AutoValue through the use of compile time plugins.

The first method, is by having your extension create an intermediate class between the abstract template class and the final AutoValue implementation of the class. The provides a good mechanism for adding functionality to an AutoValue class, while still keeping the implementation segregated.

The second method is to alter the final AutoValue class itself. This was required to support Android Parcelable implementations, as Parcels are required to have static factory objects, and this isn't achievable via inheritance. Using this method, extensions have to option of adding imports, interfaces and code to the resulting AutoValue class implementation. You can see a working example of one such plugin here.

You can view a test Android app that makes use of the auto-parcel extension here.

rharter added 7 commits May 1, 2015
@googlebot
Copy link

@googlebot googlebot commented May 15, 2015

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.
@googlebot googlebot added the cla: no label May 15, 2015
@rharter
Copy link
Contributor Author

@rharter rharter commented May 15, 2015

I signed it!

@googlebot
Copy link

@googlebot googlebot commented May 15, 2015

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels May 15, 2015
@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented May 15, 2015

I think the Travis failure indicates that you need to merge in recent changes.

*/
public interface AutoValueExtension {

public interface Context {

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

The public modifier is redundant.

public interface GeneratedClass {
String className();
String source();
Collection<String> additionalImports();

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

I think it would make sense for this to be something like Set<TypeElement> referencedTypes(). Also, the methods in this interface that return Collection<Foo> should instead return Set<Foo> since semantically I think that's what's required.

This comment has been minimized.

@rharter

rharter May 15, 2015
Author Contributor

This was implemented as strings to alleviate the need for extra dependencies. For instance, in my Parcelable example, Parcelable and Parcel are both classes in the Android Framework. By adding them as strings here, my plugin isn't dependent on the Android SDK, as these String FQCNs can simply be added to the imports clause. AFAIK I can't create a TypeElement without having access to the represented class.

This comment has been minimized.

@rharter

rharter May 15, 2015
Author Contributor

Great, I'll update it.

Collection<String> additionalImports();
Collection<ExecutableElement> consumedProperties();
Collection<String> additionalInterfaces();
Collection<String> additionalCode();

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

Is this a list of lines?

This comment has been minimized.

@rharter

rharter May 15, 2015
Author Contributor

No, this is a collection of blocks of code. This is a collection because the processor collects all of the additional code from all extensions to be added to the class, and I made it a collection here thinking that an extension implementor might have different possible chunks of code that need to be added based on the AutoValue class specification. This could certainly be changed to simply a string, if that's more straightforward.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

I'm actually not really understanding how this applies in the Parcelable case. Say you have no other extensions. Then the Parcelable extension will generate AutoValue_Foo which extends $AutoValue_Foo which extends Foo. $AutoValue_Foo is the AutoValue implementation class. Isn't it AutoValue_Foo that needs to have the definition of CREATOR and so on?

This comment has been minimized.

@rharter

rharter May 15, 2015
Author Contributor

When I implemented the subclassing version of this, the ordering was actually different. I treated the extension classes as the intermediaries, so AutoValue_Foo is the AutoValue implementation, which extends $AutoValue_Foo which is the AutoParcel implementation, which extends Foo.

The ability to add code to the final concrete class is important, particularly if you have more than one extension that requires things like static factory methods or static properties. Even if you have multiple plugins and only one requires static properties, we'd need to devise a prioritization mechanism for the extensions.

That's why I implemented this with both the ability to add intermediate classes, and the ability to add code to the final concrete AutoValue class.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

It's important for extensions to be subclasses of the generated AutoValue implementation, so they can override the generated toString() etc. One of our use cases is exactly that (being able to omit certain properties from toString()).

There's nothing stopping extensions from having static properties or methods even if they are not at the bottom of the inheritance hierarchy. The issue with Parcelable seems to be that the CREATOR field has to be in the Parcelable class itself and can't be inherited from an ancestor. I personally think that design is wrong, but there it is. We're certainly not the first to encounter it.

So in conclusion I think we probably want a way for an extension to declare that it must be at the bottom of the hierarchy? If you have two such extensions and they both want to generate code for a given class then we'll just have to reject that.

The alternative design that we considered was to switch the AutoValue implementation so it uses JavaPoet instead of templates, and have extensions be able to contribute code to the single generated class by calling JavaPoet methods. I've been reluctant to do that for a number of reasons. First, I like templates better than code-generating-code. Second, it doesn't give a clean solution for the toString() override. Third, it feels kind of anarchic when there is more than one extension contributing code (especially if they can change the already-contributed code). Finally, it risks having extensions know too much about exactly what the AutoValue implementation class looks like.

Despite all that, we may conclude from this exercise that the JavaPoet approach is the way to go.

This comment has been minimized.

@rharter

rharter May 18, 2015
Author Contributor

It sounds like the JavaPoet approach is fairly close to what I had added for the Parcelable support, basically allowing extensions to add code directly to the AutoValue class. I can definitely appreciate the apprehension about that, I was also initially apprehensive, but my inverted inheritance didn't really offer a solution and it didn't seem like ordering extensions was a good solution.

Also, I completely agree that the CREATOR static field is an unfortunate approach. I'd love to see something like an annotation on the class pointing to a factory class. That's kind of what this extension would provide, only generating the factory. Unfortunately, other solutions require wrapping, which I'm hoping this can avoid.

Regarding the inverted inheritance (AutoValue_Foo (from AutoValue) inherits from $AutoValue_Foo (from extension) inherits from abstract Foo), I don't think that causes problems for the toString use case, provided the AutoValue processor waits until after the extensions are processed to determine methods (toString, equals, hashCode) to override. Then, if there is an extension that provides a custom implementation of toString() it would be the same as if the user wrote a custom toString() in the abstract template, causing AutoValue not to generate it's own implementation, right?

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 18, 2015
Member

Well yes, but that would mean AutoValueProcessor would need to parse the code output by extensions to see whether it defines toString(). That can be done (by deferring the generation of the final AutoValue implementation to the next round of annotation processing) but it's not simple. I'm also not keen on a solution that is a mix of generating one class per extension and generating further code to be injected into the original implementation class.

Incidentally another issue with the JavaPoet approach is that extensions would need to apply the same Maven-shading as AutoValue so that their references to JavaPoet APIs would be match the APIs that AutoValue would be using. In other words, if AutoValue renames com.squareup.javapoet.TypeName to autovalue.com.squareup.javapoet.TypeName, then extensions will either have to reference that renamed class in their source code, or apply the same renaming through Maven shading when packaged.

This comment has been minimized.

@rharter

rharter May 18, 2015
Author Contributor

Yeah, I don't like the JavaPoet approach, it also adds an extra layer of requirements around implementation details on the extension developer.

I think this discussion is good, and it seems to point to your original inheritance model, with a mechanism to require an extension be at the end of the chain, and a limit that only one such extension can be present at a time.

Do you think it makes sense for me to note these finding in the issue, cancel this PR, and alter the implementation to remove the additional code and force extensions to create subclasses of the AutoValue generated class, then submit a new PR?

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 18, 2015
Member

Yes, that might be the best approach.

This comment has been minimized.

@kevinb9n

kevinb9n May 18, 2015
Contributor

Ryan, I just wanted to chime in and say how much Eamonn and I appreciate
the work you're doing and the forward momentum you're providing for this
important feature!

On Mon, May 18, 2015 at 12:43 PM, Ryan Harter notifications@github.com
wrote:

In value/src/main/java/com/google/auto/value/AutoValueExtension.java
#237 (comment):

+public interface AutoValueExtension {
+

  • public interface Context {
  • ProcessingEnvironment processingEnvironment();
  • String packageName();
  • TypeElement autoValueClass();
  • Map<String, ExecutableElement> properties();
  • }
  • public interface GeneratedClass {
  • String className();
  • String source();
  • Collection additionalImports();
  • Collection consumedProperties();
  • Collection additionalInterfaces();
  • Collection additionalCode();

Yeah, I don't like the JavaPoet approach, it also adds an extra layer of
requirements around implementation details on the extension developer.

I think this discussion is good, and it seems to point to your original
inheritance model, with a mechanism to require an extension be at the end
of the chain, and a limit that only one such extension can be present at a
time.

Do you think it makes sense for me to note these finding in the issue,
cancel this PR, and alter the implementation to remove the additional code
and force extensions to create subclasses of the AutoValue generated class,
then submit a new PR?


Reply to this email directly or view it on GitHub
https://github.com/google/auto/pull/237/files#r30540505.

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

/**
* Created by rharter on 5/1/15.
*/
public interface AutoValueExtension {

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

The overall structure looks OK but it needs javadoc.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 15, 2015
Member

Actually you might want to hold off on the javadoc. The fact that the proposed model doesn't fit the Parcelable use case all that well makes me wonder whether we should stay with it.

// ElementUtils.override that sometimes fails to recognize that one method overrides
// another, and therefore leaves us with both an abstract method and the subclass method
// that overrides it. This shows up in AutoValueTest.LukesBase for example.
errorReporter.reportWarning("@AutoValue classes cannot have abstract methods other than"

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus May 18, 2015
Member

Is there logic to make this check once extensions have had a chance to claim abstract methods? I didn't see it.

This comment has been minimized.

@rharter

rharter May 18, 2015
Author Contributor

Not at this point. This will probably have to be added back in.

@rharter
Copy link
Contributor Author

@rharter rharter commented Jul 2, 2015

@eamonnmcmanus I think this is in a good state to check out. To see an example of a working extension implementation, check out my auto-parcel code.

I'm still having a problem with my Parcelable extension in that the annotated class has to implement the Parcelable interface, but AutoValue takes the int describeContents() method from the interface to be something that it should generate. I understand the need to include inherited abstract methods in the properties list, but I'm not sure how to tell AutoValue that it doesn't need to worry about this particular interface method with no parameters that returns an int. For now, I'm working around this by simply implementing the super simple version in my annotated class. The generated class also generates an implementation.

* An AutoValueExtension allows for extra functionality to be created during the generation
* of an AutoValue class.
*
* Extensions are discovered at compile time using the {@link java.util.ServiceLoader} APIs,

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus Jul 8, 2015
Member

Can you insert a <p> at the start of the first line of each paragraph here (except the first)? Like this:

 * <p>Extensions are discovered...
/**
* The processing environment of this generation cycle.
*
* @return The {@link javax.annotation.processing.ProcessingEnvironment} of this generation cycle.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus Jul 8, 2015
Member

I think just {@link ProcessingEnvironment} would be fine, or just omit {@link} since it is the return type of the method.

String packageName();

/**
* The annotated class that this generation cycle is based on.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus Jul 8, 2015
Member

Maybe add a sentence to make it crystal clear, like this:

 * Given {@code @AutoValue public class Foo {...}}, this will be {@code Foo}.
@rharter
Copy link
Contributor Author

@rharter rharter commented Jul 31, 2015

Sorry, I guess I didn't comment when those were resolved, but they should all be resolved in the commits from that same day.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Jul 31, 2015

OK, I'll take a look later today. Sorry about the misunderstanding.

*
* <p>Extensions can extend the AutoValue implementation by generating subclasses of the AutoValue
* generated class. It's not guaranteed that an Extension's generated class will be the final
* class in the inheritance hierarchy, unless it's {@link #mustBeAtEnd} method returns true,

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus Jul 31, 2015
Member

Typo: its


/**
* An AutoValueExtension allows for extra functionality to be created during the generation
* of an AutoValue class.

This comment has been minimized.

@eamonnmcmanus

eamonnmcmanus Jul 31, 2015
Member

Can you add a sentence:
NOTE: the design of this interface is not final and subject to change.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Jul 31, 2015

I'm expecting @cgruber to make a PR shortly that will export Google's internal changes. Merging with those changes won't be too hard, I hope, and after that I think we should be good to go.

@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 1, 2015

Changes are in. I think that erroneous empty file is the result of a test and was added through a careless git add .. Seems wrong that tests would create stuff outside the target dir.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 4, 2015

Regarding the erroneous file, it must have been created by AutoAnnotationCompilationTest. That creates the file using File.createTempFile — is it possible that your system creates temporary files in the current directory??

@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 5, 2015

That's certainly possible. My system is a Mac, I haven't tested the default File.createTempFile functionality there.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 5, 2015

Meanwhile we're running into issues merging in the latest Google changes. Those changes are unfortunately more extensive than I thought, so you might be in for a bit of merging work when they go in. I'm reluctant to merge this PR first, though, for fear of making that other merge even worse.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 5, 2015

Could you resolve the merge conflicts? Then I'll have another look. Hopefully Travis issue travis-ci/travis-ci#4621 will be solved soon so we'll be able to get a correct indication of test status.

Conflicts:
	value/src/main/java/com/google/auto/value/processor/AutoValueProcessor.java
	value/src/main/java/com/google/auto/value/processor/BuilderSpec.java
@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 5, 2015

Merged and passing tests locally.

@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 6, 2015

@eamonnmcmanus Does that bug cause Travis to hang? Shows that it's still building after 5 hours.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 6, 2015

Seems Travis has been very slow of late. I waited about as long as that yesterday for a build.

@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 7, 2015

Looks like Travis is back up and all checks pass.

eamonnmcmanus added a commit that referenced this pull request Aug 7, 2015
Add an initial version of the Extensibility API to AutoValue.
@eamonnmcmanus eamonnmcmanus merged commit 068717f into google:master Aug 7, 2015
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 7, 2015

Thanks for your patience with this!

@frankiesardo
Copy link

@frankiesardo frankiesardo commented Aug 9, 2015

Thank you both this CR is well worth the wait!

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Aug 10, 2015

@frankiesardo are you able to obtain the same results with this as with your @AutoParcelable fork?

@frankiesardo
Copy link

@frankiesardo frankiesardo commented Aug 11, 2015

I pushed a branch that uses the new Extension api. So far so good.

The only problem is related to the Parcelable interface method int describeComponents() as @rharter already pointed out here rharter/auto-value-parcel@bfaba14#commitcomment-12614380).

Forcing the user to implement it, even if it's a simple method, it's an annoyance. Not sure if AutoValue can do something about it.

@frankiesardo
Copy link

@frankiesardo frankiesardo commented Aug 11, 2015

In #202 you mention briefly the possibility for an extension to claim a list of abstract methods. That would be helpful in this case.

@rharter
Copy link
Contributor Author

@rharter rharter commented Aug 11, 2015

I'm working on that.

On Tue, Aug 11, 2015, 3:36 PM Frankie Sardo notifications@github.com
wrote:

In #202 #202 you mention briefly
the possibility for an extension to claim a list of abstract methods. That
would be helpful in this case.


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

cgruber added a commit that referenced this pull request Sep 11, 2015
Add an initial version of the Extensibility API to AutoValue.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=100543793
cgruber added a commit that referenced this pull request Sep 11, 2015
*** Reason for rollback ***

Google's internal test rig detected substantial numbers of failures, and so this commit was auto-rolled-back.

*** Original change description ***

Merge pull request #237 from rharter/extensibility

Add an initial version of the Extensibility API to AutoValue.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=100545729
cgruber added a commit that referenced this pull request Sep 11, 2015
*** Reason for rollback ***

Re-attempt original commit, but with internal build fixes to avoid sending the SPI code to android builds, where javac/javax types don't exist.

*** Original change description ***

Automated g4 rollback of changelist 100543793.

*** Reason for rollback ***

Many tests lost their lives securing this commit.

*** Original change description ***

Merge pull request #237 from rharter/extensibility

Add an initial version of the Extensibility API to AutoValue.

***
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=101824430
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants