dependency-analysis-gradle-plugin icon indicating copy to clipboard operation
dependency-analysis-gradle-plugin copied to clipboard

Annotations used to annotate and things (enum constants, class literals, ...) used for annotation member values should not count to ABI

Open Vampire opened this issue 1 year ago • 1 comments

Plugin version 1.32.0

Gradle version 8.8

JDK version 17

reason output for bugs relating to incorrect advice

----------------------------------------
You asked about the dependency 'com.fasterxml.jackson.core:jackson-annotations:2.17.1'.
There is no advice regarding this dependency.
----------------------------------------

Shortest path from root project to com.fasterxml.jackson.core:jackson-annotations:2.17.1 for compileClasspath:
:
\--- com.fasterxml.jackson.core:jackson-annotations:2.17.1

There is no path from root project to com.fasterxml.jackson.core:jackson-annotations:2.17.1 for runtimeClasspath


There is no path from root project to com.fasterxml.jackson.core:jackson-annotations:2.17.1 for testCompileClasspath


There is no path from root project to com.fasterxml.jackson.core:jackson-annotations:2.17.1 for testRuntimeClasspath


Source: main
------------
* Exposes 1 class: com.fasterxml.jackson.annotation.JsonIgnore (implies api).

Source: test
------------
(no usages)

Describe the bug Annotations used to annotate and things (enum constants, class literals, ...) used for annotation member values should not be part of the ABI / always be compileOnly, no matter what retention the annotation has.

Annotations are never necessary to be in the compile classpath or runtime classpath of any downstream projects just for being present in the class file / at runtime. The annotation parser that processes annotations in the class file just skips annotations for which the class is not present.

If some code wants to retrieve the annotation from a piece of code, then that "some code" is responsible to also have the annotation added to the classpath.

To Reproduce

./build.gradle.kts
plugins {
    `java-library`
    id("com.autonomousapps.dependency-analysis") version "1.32.0"
}

repositories {
    mavenCentral()
}

dependencies {
    compileOnly("com.fasterxml.jackson.core:jackson-annotations:2.17.1")
}

./gradle/wrapper/gradle-wrapper.properties
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-8.8-bin.zip
networkTimeout=10000
validateDistributionUrl=true
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists

./settings.gradle.kts
rootProject.name = "foo"

./src/main/java/foo/Foo.java
package foo;

import com.fasterxml.jackson.annotation.JsonIgnore;

public class Foo {
    @JsonIgnore
    public Object foo() {
        return null;
    }
}

Expected behavior Annotations that are only used to annotate stuff, and things only used for annotation member values like class literaly, enum constants, ... should always be considered and recommended compileOnly.

Currently in the given example the advice is to change to api because a public method is annotated. If a private method or field is annotated, implementation is advised.

If an annotation is of course used in some other way, like as class literal outside an annotation member value, or as variable type, or as return type, or as parameter type and so on, then advising as implementation or api is perfectly correct of course.

Additional context Another example would be, if you develop a library that works stand-alone but is also DI friendly, supporting Spring, CDI, and Guice. The library developer adds annotations for all three frameworks to this class. All those annotations are runtime retention. But you do not want to enforce any specific or any at all on downstream projects.

Imho annotations that are only used to annotate things (and of course then also the types used for the annotation member values) should always be compileOnly. In case you strongly disagree with this and without any chance to be convinced to change the logic, please at least provide some feature-switch to treat it like described here.

Vampire avatar Jun 27 '24 23:06 Vampire

Thanks for the issue.

I don't strongly disagree with you. What I would like, however, is some canonical reference to a JVM spec that discusses this. The behavior as it currently exists is the result of someone filing issues over and over again and convincing me of that logic. I don't want to change the behavior based on reasonable-seeming arguments that are (1) in conflict with other reasonable-seeming arguments and (2) might be in opposition to some formal spec (assuming one exists).

I am opposed to adding a switch, because maintaining that is complex and I have limited time.

autonomousapps avatar Jul 13 '24 05:07 autonomousapps

Hey there, I'm sorry I forgot to share the information I collected.

What I would like, however, is some canonical reference to a JVM spec that discusses this

It seems there is no crystal-clear clarification in a spec currently. There is a minor provision in the JVMS. It is missing in the JavaDoc of AnnotatedElement which is the specification that should mention it. https://bugs.openjdk.org/browse/JDK-6322301 also shows that they should be ignored but it was forgotton to be added to the specification.

I asked about this on the jdk-dev mailing list and here is the answer that tells the above and also says that skipping is the right way and also ensured by tests in the reference implementation which nowadays should be the only implementation: https://mail.openjdk.org/pipermail/jdk-dev/2024-October/009459.html

Also, as I said, the behavior of the JVM supports it. Annotations for classes that are not present in the classpath are simply skipped. If you do getAnnotations or getDeclaredAnnotations they are simply not present.

Actually, there is a bug in the Java compiler in case an enum value is used as an argument to an annotation. In that case downstream projects show a warning on compilation. But imho the producer project should not mitigate that bug. After all it is just a warning, and if the downstream project decides to promote warnings to errors, they can as well add the dependency as compileOnly dependency to mitigate the problem until it is fixed in the Java compiler.

So to summarize, even after collecting more information about it, I still think annotations that are only used to annotate something and classes / enum constants that are only used as arguments to annotations that annotate something should always only be compileOnly, nothing else. (If the annotation classes or classes / enum constants are also used in another way like in an implementation or returned from a method or as parameter of a method and so on, then in those cases the recommendation should be accordingly of course.)

Vampire avatar Jul 16 '25 10:07 Vampire

Thank you, I appreciate the research and detailed information you've provided. I'm inclined to agree with you and open to changing this behavior. However, I also have limited time right now and (of course) can provide no promise as to a timeline when this might change. I have more time for PR reviews, so if you or someone wants to contribute a change, that would be welcome.

autonomousapps avatar Jul 16 '25 19:07 autonomousapps

While having a first stab, I realized that there indeed is one exception to the "everything is compileOnly". Luckily that case is present in the tests. If you use a class literal as argument to a runtime-retention-annotation that is in another "jar", it should indeed be implementation. The example is the JUnit runner annotation. JUnit provides the @RunWith runtime retention annotation. Spock 1.x provides a Runner subclass that can be given as argument to @RunWith. If you now have Spock as compileOnly, then at runtime you have the RunWith class because you have JUnit in your runtime classpath, but the Spock class is missing and test execution fails.

With member-annotations and enums it is different, because you cannot subclass them. So if the annotation "jar" will have those as parameters, it will have an api dependency on them, so the one providing the annotation will also provide those transitively and in the project annotating they can stay compileOnly.

So to recap current mind model:

  • annotations => always compileOnly no matter which retention
  • enums and member annotations used as annotation arguments => always compileOnly, the annotation will bring them as transitive dependency
  • class literals used as annotation arguments =>
    • compileOnly if annotation was non-runtime-retention
    • compileOnly if annotation was runtime-retention but is contained in the same "jar"
    • implementation if annotation was runtime-retention and the class is from a different "jar"

Vampire avatar Jul 17 '25 11:07 Vampire

I've played some more and hopefully have fixed it in #1506 :-)

Vampire avatar Jul 31 '25 23:07 Vampire