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

Make animal sniffer annotations optional #3649

Closed
wants to merge 2 commits into from
Closed

Make animal sniffer annotations optional #3649

wants to merge 2 commits into from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Oct 9, 2019

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

@cpovirk

@cpovirk
Copy link
Member

cpovirk commented Oct 9, 2019

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:

@elharo
Copy link
Contributor Author

elharo commented Oct 9, 2019

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
Copy link
Contributor Author

elharo commented Oct 9, 2019

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
Copy link
Member

cpovirk commented Oct 9, 2019

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
Copy link
Contributor Author

elharo commented Oct 9, 2019

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
Copy link
Member

cpovirk commented Oct 9, 2019

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
Copy link
Member

cpovirk commented Oct 9, 2019

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
Copy link
Contributor Author

elharo commented Oct 9, 2019

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

@cpovirk
Copy link
Member

cpovirk commented Oct 9, 2019

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

@elharo
Copy link
Contributor Author

elharo commented Oct 10, 2019

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

@cpovirk
Copy link
Member

cpovirk commented Oct 10, 2019

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

@elharo
Copy link
Contributor Author

elharo commented Oct 10, 2019

Closing this in favor of #3652

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

Successfully merging this pull request may close these issues.

4 participants