diff --git a/src/functionalTest/groovy/com/autonomousapps/android/ExceptionsAreSpecialSpec.groovy b/src/functionalTest/groovy/com/autonomousapps/android/ExceptionsAreSpecialSpec.groovy new file mode 100644 index 000000000..c5b78b7d1 --- /dev/null +++ b/src/functionalTest/groovy/com/autonomousapps/android/ExceptionsAreSpecialSpec.groovy @@ -0,0 +1,35 @@ +// Copyright (c) 2026. Tony Robalik. +// SPDX-License-Identifier: Apache-2.0 +package com.autonomousapps.android + +import com.autonomousapps.android.projects.ExceptionsAreSpecialProject +import com.autonomousapps.utils.Colors + +import static com.autonomousapps.advice.truth.BuildHealthSubject.buildHealth +import static com.autonomousapps.kit.GradleBuilder.build +import static com.google.common.truth.Truth.assertAbout +import static com.google.common.truth.Truth.assertThat + +class ExceptionsAreSpecialSpec extends AbstractAndroidSpec { + + @SuppressWarnings('GroovyAssignabilityCheck') + def "exception types are correctly mapped to the providing dependencies (#gradleVersion AGP #agpVersion)"() { + given: + def project = new ExceptionsAreSpecialProject(agpVersion) + gradleProject = project.gradleProject + + when: + def result = build(gradleVersion, gradleProject.rootDir, ':buildHealth', ':consumer:reason', '--id', 'androidx.navigation:navigation-common:') + + then: + assertAbout(buildHealth()) + .that(project.actualProjectAdvice()) + .isEquivalentIgnoringModuleAdviceAndWarnings(project.expectedProjectAdvice()) + + and: + assertThat(Colors.decolorize(result.output)).contains(project.expectedReason()) + + where: + [gradleVersion, agpVersion] << gradleAgpMatrix() + } +} diff --git a/src/functionalTest/groovy/com/autonomousapps/android/projects/AndroidTestSourceProject.groovy b/src/functionalTest/groovy/com/autonomousapps/android/projects/AndroidTestSourceProject.groovy index 0ac27fb56..a8bfe6d73 100644 --- a/src/functionalTest/groovy/com/autonomousapps/android/projects/AndroidTestSourceProject.groovy +++ b/src/functionalTest/groovy/com/autonomousapps/android/projects/AndroidTestSourceProject.groovy @@ -6,7 +6,6 @@ import com.autonomousapps.kit.GradleProject import com.autonomousapps.kit.Source import com.autonomousapps.kit.SourceType import com.autonomousapps.kit.android.AndroidColorRes -import com.autonomousapps.kit.android.AndroidManifest import com.autonomousapps.kit.android.AndroidStyleRes import com.autonomousapps.kit.gradle.Dependency import com.autonomousapps.kit.gradle.Plugin @@ -86,16 +85,6 @@ final class AndroidTestSourceProject extends AbstractAndroidProject { private List appSources() { def sources = [ - new Source( - SourceType.KOTLIN, 'App', 'com/example', - """\ - package com.example - - class App { - fun magic() = 42 - } - """.stripIndent() - ), new Source( SourceType.KOTLIN, 'Test', 'com/example', """\ diff --git a/src/functionalTest/groovy/com/autonomousapps/android/projects/ExceptionsAreSpecialProject.groovy b/src/functionalTest/groovy/com/autonomousapps/android/projects/ExceptionsAreSpecialProject.groovy new file mode 100644 index 000000000..042f3e225 --- /dev/null +++ b/src/functionalTest/groovy/com/autonomousapps/android/projects/ExceptionsAreSpecialProject.groovy @@ -0,0 +1,87 @@ +// Copyright (c) 2026. Tony Robalik. +// SPDX-License-Identifier: Apache-2.0 +package com.autonomousapps.android.projects + +import com.autonomousapps.kit.GradleProject +import com.autonomousapps.kit.Source +import com.autonomousapps.kit.gradle.Dependency +import com.autonomousapps.model.Advice +import com.autonomousapps.model.ProjectAdvice + +import static com.autonomousapps.AdviceHelper.* +import static com.autonomousapps.kit.gradle.Dependency.implementation +import static com.autonomousapps.kit.gradle.Dependency.testImplementation + +final class ExceptionsAreSpecialProject extends AbstractAndroidProject { + + private static final Dependency HILT_IMPL = implementation('androidx.hilt:hilt-navigation-compose:1.2.0') + private static final Dependency LAYOUTLIB_IMPL = testImplementation('com.android.tools.layoutlib:layoutlib:15.2.3') + + final GradleProject gradleProject + + ExceptionsAreSpecialProject(String agpVersion) { + super(agpVersion) + this.gradleProject = build() + } + + private GradleProject build() { + return newAndroidGradleProjectBuilder(agpVersion) + .withAndroidLibProject('consumer') { s -> + s.sources = consumerSources() + s.withBuildScript { bs -> + bs.android = defaultAndroidLibBlock(false, 'com.example.consumer') + bs.plugins(androidLib(false)) + bs.dependencies( + HILT_IMPL, + LAYOUTLIB_IMPL, + ) + } + } + .write() + } + + private static final List consumerSources() { + return [ + Source.java( + '''\ + package mutual.aid.consumer; + + // layoutlib bundles this class + import android.content.Context; + + public abstract class ConsumerTest { + public abstract Context getContext(); + } + '''.stripIndent() + ) + .withSourceSet('test') + .build() + ] + } + + Set actualProjectAdvice() { + return actualProjectAdvice(gradleProject) + } + + private final Set consumerAdvice() { + [ + Advice.ofRemove(moduleCoordinates(HILT_IMPL), HILT_IMPL.configuration), + + // These two both provide runtime capabilities and would be removed with the removal of hilt, so we advise they + // be added directly to minimize changes to the runtime classpath. + Advice.ofAdd(moduleCoordinates('com.google.dagger:dagger-lint-aar:2.49'), 'runtimeOnly'), + Advice.ofAdd(moduleCoordinates('org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4'), 'runtimeOnly') + ] + } + + Set expectedProjectAdvice() { + return [projectAdviceForDependencies(':consumer', consumerAdvice())] + } + + String expectedReason() { + return '''\ + Source: debug, test + ------------------- + (no usages)'''.stripIndent() + } +} diff --git a/src/functionalTest/groovy/com/autonomousapps/jvm/ExceptionsAreSpecialSpec.groovy b/src/functionalTest/groovy/com/autonomousapps/jvm/ExceptionsAreSpecialSpec.groovy index 7ac4a00f7..598313f15 100644 --- a/src/functionalTest/groovy/com/autonomousapps/jvm/ExceptionsAreSpecialSpec.groovy +++ b/src/functionalTest/groovy/com/autonomousapps/jvm/ExceptionsAreSpecialSpec.groovy @@ -11,7 +11,7 @@ import static com.google.common.truth.Truth.assertThat class ExceptionsAreSpecialSpec extends AbstractJvmSpec { @SuppressWarnings('GroovyAssignabilityCheck') - def "annotations on public classes are part of the abi (#gradleVersion isBroken=#isBroken)"() { + def "exception types are required at runtime (#gradleVersion isBroken=#isBroken)"() { given: def project = new ExceptionsAreSpecialProject(isBroken) gradleProject = project.gradleProject diff --git a/src/main/kotlin/com/autonomousapps/internal/transform/StandardTransform.kt b/src/main/kotlin/com/autonomousapps/internal/transform/StandardTransform.kt index 8b950a7bb..2f6fc82f3 100644 --- a/src/main/kotlin/com/autonomousapps/internal/transform/StandardTransform.kt +++ b/src/main/kotlin/com/autonomousapps/internal/transform/StandardTransform.kt @@ -209,22 +209,29 @@ internal class StandardTransform( sourceSetName: String, ): MutableSet { fun MutableSet.maybeReduceFurther(): MutableSet { - return if (projectType == ProjectType.ANDROID && sourceSetName == SourceKind.TEST_NAME) { + return if (projectType == ProjectType.ANDROID && sourceSetName == SourceKind.TEST_NAME) { // TODO: `&& SourceKind.ANDROID_TEST_NAME`? reduceUsages(this) } else { this } } - // TODO(tsr): is there a case where both forCompile and forRuntime are true? What then? - return if (visibility.forCompile && !explicitFor(sourceSetName)) { - asSequence() - .filterNot { usage -> usage.bucket != Bucket.RUNTIME_ONLY } - .toMutableSet() - .maybeReduceFurther() - } else if (visibility.forRuntime && !explicitFor(sourceSetName)) { - asSequence() - .filterNot { usage -> usage.bucket == Bucket.RUNTIME_ONLY } + return if (visibility.forEither && !explicitFor(sourceSetName)) { + // TODO(tsr): add test for this case. + // 1. a dep that should be moved from impl->api + // 2. dep has runtime capabilities as well and is on test runtime classpath + // 3. previously we'd see advice to both move to api and also add to testRuntimeOnly. The latter is undesirable. + + // Yes, if something is visible forCompile and forRuntime, then we filter out all usages. + var seq = asSequence() + if (visibility.forCompile) { + seq = seq.filterNot { usage -> usage.bucket != Bucket.RUNTIME_ONLY } + } + if (visibility.forRuntime) { + seq = seq.filterNot { usage -> usage.bucket == Bucket.RUNTIME_ONLY } + } + + seq .toMutableSet() .maybeReduceFurther() } else { diff --git a/src/main/kotlin/com/autonomousapps/internal/utils/collections.kt b/src/main/kotlin/com/autonomousapps/internal/utils/collections.kt index 8be758482..324a12c7e 100644 --- a/src/main/kotlin/com/autonomousapps/internal/utils/collections.kt +++ b/src/main/kotlin/com/autonomousapps/internal/utils/collections.kt @@ -146,7 +146,9 @@ internal inline fun Iterable.mapNotNullToOrderedSet(transform: ( return mapNotNullTo(TreeSet(), transform) } -internal inline fun Iterable.mapSecondNotNull(transform: (T) -> Pair): List> { +internal inline fun Iterable.mapSecondNotNull( + transform: (T) -> Pair +): List> { return mapNotNull { val pair = transform(it) if (pair.second != null) { @@ -158,6 +160,26 @@ internal inline fun Iterable.mapSecondNotNull(transform } } +internal inline fun Sequence.mapSecondNotNull( + crossinline transform: (T) -> Pair +): Sequence> { + return mapNotNull { + val pair = transform(it) + if (pair.second != null) { + @Suppress("UNCHECKED_CAST") + pair as Pair + } else { + null + } + } +} + +internal inline fun Sequence.associateNotNull( + crossinline transform: (T) -> Pair? +): Map { + return mapNotNull { transform(it) }.toMap() +} + /** * Sort elements keeping Comparable-equal elements (stable sorting). * This method has different semantics with standard toSortedSet(Comparator). diff --git a/src/main/kotlin/com/autonomousapps/model/internal/declaration/Bucket.kt b/src/main/kotlin/com/autonomousapps/model/internal/declaration/Bucket.kt index 7f88efceb..b7f8be21e 100644 --- a/src/main/kotlin/com/autonomousapps/model/internal/declaration/Bucket.kt +++ b/src/main/kotlin/com/autonomousapps/model/internal/declaration/Bucket.kt @@ -129,5 +129,7 @@ internal enum class Bucket(val value: String) { } } - class Visibility(val forCompile: Boolean, val forRuntime: Boolean) + class Visibility(val forCompile: Boolean, val forRuntime: Boolean) { + val forEither: Boolean get() = forCompile || forRuntime + } } diff --git a/src/main/kotlin/com/autonomousapps/model/internal/intermediates/Reason.kt b/src/main/kotlin/com/autonomousapps/model/internal/intermediates/Reason.kt index a7d8322de..ad81f7ef5 100644 --- a/src/main/kotlin/com/autonomousapps/model/internal/intermediates/Reason.kt +++ b/src/main/kotlin/com/autonomousapps/model/internal/intermediates/Reason.kt @@ -268,7 +268,7 @@ internal sealed class Reason(open val reason: String) { @TypeLabel("lint") @JsonClass(generateAdapter = false) data class LintJar(override val reason: String) : Reason(reason) { - override val configurationName: String = "implementation" + override val configurationName: String = "runtimeOnly" internal companion object { fun of(lintRegistry: String) = LintJar( diff --git a/src/main/kotlin/com/autonomousapps/tasks/ComputeUsagesTask.kt b/src/main/kotlin/com/autonomousapps/tasks/ComputeUsagesTask.kt index 1174e3346..d1a266c63 100644 --- a/src/main/kotlin/com/autonomousapps/tasks/ComputeUsagesTask.kt +++ b/src/main/kotlin/com/autonomousapps/tasks/ComputeUsagesTask.kt @@ -698,21 +698,43 @@ private class GraphVisitor( capability: ExceptionCapability, context: GraphViewVisitor.Context, ): Boolean { + // TODO(tsr): extract this to a testable function val codeSource = context.project.codeSource if (codeSource.isEmpty()) { - // nothing to do here + // Nothing to do here. We don't have any code, so we can't be referring to any exception types, by definition. + // This is an optimization. return false } - // These are all the exception types referenced by `coordinates`, which is a specific dependency of `this` project - // (i.e., `context.project`). - val exceptions = capability.exceptions.values.flatten().toSet() + // These are all the exception types referenced by `this` dependency. + val referencedExceptions = capability.exceptions.values.flatten().toSet() + + // Ideally this would be a single item, but it could be multiple if we have duplicates on the classpath. + val exceptionProviders: Map> = context.dependencies + // We only care about dependencies that have classes + .mapSecondNotNull { dependency -> dependency.coordinates to dependency.findCapability() } + // Find the dependencies that have a class that matches any exception referenced by `this` dependency. + .mapNotNull { (coordinates, binaryClass) -> + val relevantTypes = binaryClass.classes.filterToOrderedSet { it in referencedExceptions } + if (relevantTypes.isEmpty()) { + null + } else { + coordinates to relevantTypes + } + } + .toMap() + + if (exceptionProviders.isEmpty()) { + // Nothing to do here. + return false + } - // TODO(tsr): extract this to testable function // These are all the dependencies of this project that use the exception types provided by `coordinates`. - val users: Map>> = context.dependencies + val users: Map>> = context.dependencies.asSequence() // The dependency can see all its own exceptions .filterNot { dependency -> dependency.coordinates == coordinates } + // Don't scan the exception providers, that doesn't make sense and leads to incorrect results + .filterNot { dependency -> dependency.coordinates in exceptionProviders.keys } // If the dependency's runtime graph can reach `this` dependency, then we can skip analysis .filterNot { dependency -> val graph = context.graphRuntime.graph @@ -728,7 +750,7 @@ private class GraphVisitor( .associateNotNull { (coordinates, exceptionCapability) -> val classToExceptionType = exceptionCapability.exceptions .mapNotNull { (className, types) -> - val relevantTypes = types.filterToOrderedSet { type -> type in exceptions } + val relevantTypes = types.filterToOrderedSet { type -> type in referencedExceptions } if (relevantTypes.isEmpty()) { null } else { @@ -736,9 +758,7 @@ private class GraphVisitor( } } // It only matters if the code of `this` project refers to a class that itself references a relevant type - .filter { (className, _) -> - codeSource.any { source -> className in source.usedNonAnnotationClasses } - } + .filter { (className, _) -> codeSource.any { source -> className in source.usedNonAnnotationClasses } } .toMap() if (classToExceptionType.isNotEmpty()) { diff --git a/src/test/kotlin/com/autonomousapps/internal/reason/DependencyAdviceExplainerTest.kt b/src/test/kotlin/com/autonomousapps/internal/reason/DependencyAdviceExplainerTest.kt index 2204ee466..f4136f01d 100644 --- a/src/test/kotlin/com/autonomousapps/internal/reason/DependencyAdviceExplainerTest.kt +++ b/src/test/kotlin/com/autonomousapps/internal/reason/DependencyAdviceExplainerTest.kt @@ -85,7 +85,7 @@ class DependencyAdviceExplainerTest { * Uses 6 classes, 5 of which are shown: One, Two, Three, Four, Five (implies implementation). * Imports 6 classes, 5 of which are shown: One, Two, Three, Four, Five (implies implementation). * Imports 6 inline members, 5 of which are shown: One, Two, Three, Four, Five (implies implementation). - * Provides 1 lint registry: LintRegistry (implies implementation). + * Provides 1 lint registry: LintRegistry (implies runtimeOnly). * Provides 2 native binaries: foo, bar (implies runtimeOnly). * Imports 2 resources: drawable, string (implies implementation). * Uses 2 resources: StyleParentRef(styleParent=AppCompat), AttrRef(type=drawable, id=logo) (implies implementation).