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

[elharo]

@eamonnmcmanus @saturnism

[eamonnmcmanus]

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.

[elharo]

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."

[eamonnmcmanus]

"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.

[tbroyer]

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.

[elharo]

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.

[cpovirk]

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.)

[saturnism]

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.

[cpovirk]

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).

[cpovirk]

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.)

[cpovirk]

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]

#995