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.82k stars 120 forks source link

Incorrect advice to change dependency from implementation to api #1118

Open asuslennikov opened 9 months ago

asuslennikov commented 9 months ago

Plugin version "1.29.0"

Gradle version Gradle-8.2

JDK version openjdk version "17" 2021-09-14 OpenJDK Runtime Environment (build 17+35-2724) OpenJDK 64-Bit Server VM (build 17+35-2724, mixed mode, sharing)

(Optional) Kotlin and Kotlin Gradle Plugin (KGP) version "1.9.0"

(Optional) Android Gradle Plugin (AGP) version 8.2.0

(Optional) reason output for bugs relating to incorrect advice

> Task :feature:reason

----------------------------------------
You asked about the dependency 'androidx.compose.foundation:foundation-layout-android:1.6.0'.
You have been advised to change this dependency to 'api' from 'implementation'.
----------------------------------------

Shortest path from :feature to androidx.compose.foundation:foundation-layout-android:1.6.0 for debugCompileClasspath:
:feature
\--- androidx.compose.foundation:foundation-layout-android:1.6.0

<TRUNC>

Source: debug, main
-------------------
* Exposes 1 class: androidx.compose.foundation.layout.BoxScope (implies api).

Describe the bug As per this article: https://dev.to/autonomousapps/dependency-analysis-gradle-plugin-what-s-an-abi-3l2h this plugin uses application binary interface as a source of advices. Unfortunately, compiler can add some optimizations, such as cache for lambdas:

@Lkotlin/Metadata;
public final class com/github/dependency/analysis/feature/ComposableSingletons$FeatureElementKt {
    public static final field INSTANCE Lcom/github/dependency/analysis/feature/ComposableSingletons$FeatureElementKt;
    public static field lambda-1 Lkotlin/jvm/functions/Function3;
    public fun <init> ()V
    public final fun getLambda-1$feature_debug ()Lkotlin/jvm/functions/Function3;
}

And due to these transformations it is possible, that some classes will be treated as exposed (BoxScope from steps to reproduce section), but in fact they are not a part of actual public module API.

To Reproduce Steps to reproduce the behavior: 1) Create library module, for example uikit with compose elements for android development.

@Composable
fun BaseFooter(subContent: @Composable BoxScope.() -> Unit) {
    Box(modifier = Modifier.fillMaxSize()) {
        subContent()
    }
    Text(text = "footer")
}

It exposes BoxScope, hence has api("androidx.compose.foundation:foundation-layout-android") 2) Create feature module and use uikit as a dependency:

@Composable
fun FeatureElement(text: String) {
    Text(text = text)
    // It triggers incorrect api suggestion due to cached lambda
    BaseFooter {}
    // This variant pass without any advice
    // val someString = remember { "text" }
    // BaseFooter { println(someString) }
}

This module doesn't expose BoxScope directly, just uses it internally, but plugin treats it as public usage.

Expected behavior Plugin shouldn't advice to change from implementation("androidx.compose.foundation:foundation-layout-android") to api("androidx.compose.foundation:foundation-layout-android") for feature module.

Additional context Test project: DependencyAnalysisTest.zip (don't forget to add local.properties with sdk.dir=YOUR_PATH)

Launch the projectHealth task for feature module

./gradlew :feature:projectHealth --no-configuration-cache

and check reason:

./gradlew :feature:reason --no-configuration-cache --id androidx.compose.foundation:foundation-layout-android
autonomousapps commented 9 months ago

Thanks for the issue. This is an interesting bug. I don't have any immediate ideas how to resolve it -- open to suggestions.

Natoshka commented 9 months ago

I've found a similar case in my Java multi module library. Example:

subprojects {
  ext {
    jackson_version = "2.15.2"
  }
  dependencies {
    implementation "com.fasterxml.jackson.core:jackson-databind:${jackson_version}"
  }
}

project(":one") {
  dependencies {
    ...
  }
}

project(":two") {
  dependencies {
    implementation project(":one")
    ...
  }
}

and after running ./gradlew buildHealth I get

Advice for :one
Existing dependencies which should be modified to be as indicated:
  api 'com.fasterxml.jackson.core:jackson-databind:2.15.2' (was implementation)

Advice for :two
Existing dependencies which should be modified to be as indicated:
  api 'com.fasterxml.jackson.core:jackson-databind:2.15.2' (was implementation)
  api project(':one') (was implementation)

I don't understand why plugin tries to change implementation to api in such a case. Plugin has no idea about the subsequent usages of the library, so the advice is more harmful than useful, isn't it? 😢 Is it possible to disable such advice (from impl to api) via configuration?

./gradlew one:reason --id com.fasterxml.jackson.core:jackson-databind
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF8

> Task :one:reason

----------------------------------------
You asked about the dependency 'com.fasterxml.jackson.core:jackson-databind:2.15.2'.
You have been advised to change this dependency to 'api' from 'implementation'.
----------------------------------------

Shortest path from :one to com.fasterxml.jackson.core:jackson-databind:2.15.2 for compileClasspath:
:one
\--- com.fasterxml.jackson.core:jackson-databind:2.15.2

Shortest path from :one to com.fasterxml.jackson.core:jackson-databind:2.15.2 for runtimeClasspath:
:one
\--- com.fasterxml.jackson.core:jackson-databind:2.15.2

Shortest path from :one to com.fasterxml.jackson.core:jackson-databind:2.15.2 for testCompileClasspath:
:one
\--- com.fasterxml.jackson.core:jackson-databind:2.15.2

Shortest path from :one to com.fasterxml.jackson.core:jackson-databind:2.15.2 for testRuntimeClasspath:
:one
\--- com.fasterxml.jackson.core:jackson-databind:2.15.2

Source: main
------------
* Exposes 3 classes: com.fasterxml.jackson.databind.JsonNode, com.fasterxml.jackson.databind.annotation.JsonDeserialize, com.fasterxml.jackson.databind.annotation.JsonSerialize (implie
s api).
* Provides 1 service loader: com.fasterxml.jackson.databind.ObjectMapper (implies runtimeOnly).

Source: test
------------
* Uses 2 classes: com.fasterxml.jackson.databind.ObjectMapper, com.fasterxml.jackson.databind.ObjectWriter (implies testImplementation).
* Provides 1 service loader: com.fasterxml.jackson.databind.ObjectMapper (implies testRuntimeOnly).
autonomousapps commented 9 months ago

@Natoshka yours is a very different case, and I see no bug. Note this text in your reason output:

* Exposes 3 classes: com.fasterxml.jackson.databind.JsonNode, com.fasterxml.jackson.databind.annotation.JsonDeserialize, com.fasterxml.jackson.databind.annotation.JsonSerialize (implies api).

Please see this.

Natoshka commented 9 months ago

@autonomousapps WOW! Thanks for such insight! It seems I have been using Gradle wrong 😳 And thanks for plugin, by the way ❤️

asuslennikov commented 9 months ago

If I understand correctly, this behavior happens due to analyze of some compiler-generated classes, for example for lambdas. For test project Kotlin bytecode tool-window shows this:

final class com/github/dependency/analysis/feature/FeatureElementKt$FeatureElement$1 extends kotlin/jvm/internal/Lambda implements kotlin/jvm/functions/Function1 {

/* some bytecode */

  // access flags 0x11
  public final invoke(Landroidx/compose/foundation/layout/BoxScope;)V

So, may be add some exclusion option like excludeSubClassesOf(@Language("RegExp") vararg regexes: String)? This way, user can configure plugin to ignore classes which extends kotlin.jvm.internal.Lambda:

dependencyAnalysis {
  abi {
    exclusions {
       excludeSubClassesOf("kotlin\\.jvm\\.internal\\.Lambda")
    }
  }
}
autonomousapps commented 9 months ago

Thanks! Not a bad idea. Can you use the existing DSL to add the exclusions you'd need?

asuslennikov commented 9 months ago

Yes, it is possible, but you have to manually add all files where this happens. The easiest way - is to exclude via excludeClasses and put regex with kt file's name in it. But it increases config size and it is not granual, so you can exclude correct advices.