autonomousapps / dependency-analysis-gradle-plugin

Gradle plugin for JVM projects written in Java, Kotlin, Groovy, or Scala; and Android projects written in Java or Kotlin. Provides advice for managing dependencies and other applied plugins
Apache License 2.0
1.66k stars 116 forks source link

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

Open Vampire opened 4 days ago

Vampire commented 4 days ago

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.