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

Mark autovalue-annotations optional (Maven) or compileOnly (gradle) #765

Closed
wants to merge 1 commit into from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Oct 2, 2019

@@ -83,6 +83,7 @@ Maven users should add the following to the project's `pom.xml` file:
<groupId>com.google.auto.value</groupId>
<artifactId>auto-value-annotations</artifactId>
<version>1.6.6</version>
<optional>true</optional>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a completely safe recommendation. Since @AutoValue has CLASS retention, when you compile @AutoValue class Foo there is a reference to com.google.auto.value.AutoValue in Foo.class, even if that reference is not visible to reflection. If a tool, such as another annotation processor, starts looking at the annotations referenced in Foo.class it may be perturbed by the missing reference.

Meanwhile, auto-value-annotations is a pretty tiny dependency with no transitive dependencies. So I'm not sure there's much incentive to making people have to think about whether to mark it optional or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto-value-annotations does seem to be causing us trouble in javacore and possibly pulling in autoservice, so retaining this in the classpath isn't guaranteede4ly safe. Why does this have class retention? This would seem to contradict the claim that "Your choice to use AutoValue is essentially API-invisible."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Possibly pulling in autoservice", that's surprising to me. I don't see where that reference would come from. AutoService is referenced by the AutoValue processor but not by the annotations in this jar.

We did have SOURCE retention initially, but it turns out that some tools want to see whether a class is an @AutoValue. For example, AutoValue itself does not allow one @AutoValue class to extend another, but with SOURCE retention it would only be able to check that if the two classes were being compiled at the same time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a completely safe recommendation.

💯
This is similar to ErrorProne annotations, Checker Framework annotations, etc. that had been causing issues when absent from the classpath (generally when the downstream project is using ErrorProne IIRC), and led to Guava (among others) to declare them as "normal" dependencies.

We did have SOURCE retention initially, but it turns out that some tools want to see whether a class is an @AutoValue.

FWIW, ErrorProne is one of those tools: https://errorprone.info/bugpattern/ExtendsAutoValue

TL;DR: @AutoValue is somehow part of your API, and should be included in the classpath by default. Feel free to exclude it from your transitive dependencies if you don't want it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I haven't figured that out either. It's a weird one and I may be guessing wrong about the root cause. However I did find that marking autovalue-annotations optional made the AutoService error go away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be explicit: I'm pretty sure that putting the annotations as a dependency of the plugin does not let the plugin read them through the plugin APIs. The annotations need to be on the compile classpath. So that leaves us with this:

user should place those annotations on their own classpath either as optionally scoped

That way should work.

(I'm still a bigger fan of avoiding optional here, but the main thing I want is to make sure we're clear about what users need to do to get the behavior they want.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did give it a try; and it certainly didn't work for error-prone-annotations.

for autovalue, i haven't been able to trigger ExtendsAutoValue violation w/ or w/o the auto-value-annotations. will test more out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExtendsAutoValue is a warning by default. I'm not sure how that's surfaced in open-source builds. Internally, you'd probably need to open a CL in Critique after the analyzer has run.

Alternatively, have a look at my CL 273372038, in which I promoted it to an error. You can build java/com/google/bar to see a failure (even though I've hidden the internal equivalent of auto-value-annotations from that compilation).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more things:

  • javac itself will complain about missing annotation .class files, albeit under relatively unusual circumstances: The user has to pass -Xlint:all to see a warning (and then also pass -Werror to make it fail the build). Plus, the annotation has to have elements. @AutoValue itself does not, but @AutoOneOf does, as does @AutoValue.CopyAnnotations. That can lead to: Cannot find annotation method 'value()' in type 'AutoOneOf': class file for com.google.auto.value.AutoOneOf not found
  • Another approach to this is for AutoValue to recognize custom annotations. @elharo took advantage of similar support in Animal Sniffer in switch to an internal annotation for ignoring JRE requirements guava#3652. Animal Sniffer's approach is to let users pass a list of annotations to recognize. This is somewhat contrary to AutoValue's normal principle of not offering configuration, but at least it doesn't affect output (other than by affecting whether AutoValue works at all for a given input :)). Another possibility is to recognize any annotation with the simple name "AutoValue." This is an approach that I've seen in Error Prone checkers. That approach makes a lot of sense when there are multiple existing annotations with roughly the right semantics. It's less compelling here, where there's only one @AutoValue, but maybe it's still worth it for dependency-management reasons. (It might get more complex, though, to write generic code in the processor to cover annotations that have elements? I'm not sure.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these docs changed in 43ff5f2, maybe we can now close this PR?

Or maybe it can be revised to still update the Gradle section? Nowadays, we can even recommend compileOnlyApi.

@elharo
Copy link
Contributor Author

elharo commented Feb 26, 2021

#995

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants