Make animal sniffer annotations optional

[elharo]

Per https://www.mojohaus.org/animal-sniffer/animal-sniffer-annotations/ this should not be inherited by dependents

@cpovirk

[cpovirk]

I think the case for this is a little better than the case for AutoValue's annotations, but I still have my doubts.

I think the overall consensus that annotation-only packages should be optional or provided is wrong, as discussed in the AutoValue thread. There are two main cases:

• Omitting an annotation-only dependency can cause failures with -Xlint:all -Werror. The good news is that I haven't been able to produce this failure unless the annotation has elements. And the single Animal Sniffer annotation has no elements -- yet, anyway :) That doesn't mean that success is guaranteed, though, only that it happens to work now. I don't think that javac makes guarantees about incomplete classpaths in general, though I could be wrong.
• Omitting an annotation-only dependency can cause problems with plugins, annotation processors, etc. For example, I could imagine static analysis that checks, if any annotation is declared on both a class and its superclass, whether that annotation is @Inherited (so that it can suggest removing it from the subclass). This static analysis would need to load all annotations. Now, it can just skip checking an annotation if the annotation class isn't available, but I'm not sure we can say that we're doing the right thing if we force that. And maybe the static analysis would be more important than my example, involving security or something. For example, there are systems that let you define that code annotated with one annotation "implies" one or more other annotations ("meta-annotations"). So if security-critical static analysis sees @MyFoo on a method, then it might need to look at @MyFoo to make sure that it doesn't imply something like @CompileTimeConstant. (Possible example, albeit non-security-critical: @IncompatibleModifiers.)

So I think that including annotations is still the right thing to do by default. The typical guidance I've seen to make them provided or optional doesn't even consider these issues, so I don't think we should put much stock in it.

I assume that the goal here is to avoid version conflicts. Is this a case in which the Linkage Checker can detect that it's harmless to have different but identical versions of the class file present?

---

edit: an additional concern:

• If users look up annotations with reflection at runtime, missing annotations can lead to exceptions in some cases, even though the JDK has gone to some effort to avoid that in some cases.

[elharo]

The purpose of animal sniffer is for compile time checking of the project with animal sniffer. That's it. These annotations don't have any use post-compile.

[elharo]

I also think it's important that a fundamental library such as Guava minimize its dependency tree, ideally to the JDK, full stop. However between animal sniffer, error prone, and others Guava has slowly been accumulating a lot of additional dependencies that are transitively appearing in other projects. Every dependency is a liability.

[cpovirk]

They have class retention, though, so they'll be in the output. Maybe they shouldn't be, but arbitrary tools don't know that :( Maybe file a bug against Animal Sniffer?

[elharo]

Good idea. Done: mojohaus/animal-sniffer#73

We'll see what the animal sniffer devs say, but I'm guessing the plugin is analyzing the .class files rather than the .java files, which would explain why they want class retention with an optional dependency. The linkage checker operates similarly. We read .class files, not .java files.

[cpovirk]

Ah, you're probably right.

I found the following in some docs (the same ones as you linked above):

Animal sniffer also allows you to specify your own custom annotation if needed. Check the configuration of the relevant task for more information.

So maybe we can provide our own.

[cpovirk]

http://www.mojohaus.org/animal-sniffer/animal-sniffer-maven-plugin/check-mojo.html#annotations

[cpovirk]

I think that the impact of annotation-only packages is fairly small as liabilities go. Or at the very least, if it's safe to omit the packages entirely, then it's likely to be safe to have the wrong version present.

Still, I think it would be fine to migrate our one usage of @IgnoreJRERequirement to a custom, package-private implementation, configuring Animal Sniffer to recognize that, even if that feels like primarily a symbolic victory to me.

[elharo]

Should that approach (moving to a package private custom implementation) start in google3 or here?

[cpovirk]

google3 is marginally easier for us, but I'm fine with whatever you prefer.

[elharo]

It seems the pom.xml only lives here so probably easier to work from github.

[cpovirk]

A search like f:guava/pom.xml should turn it up in google3, but again, here is fine if that's easier.

[elharo]

Closing this in favor of #3652