fix: merge dependency usages by identifier, not gav.

This resolves an issue when a dependency is available at more than one version,
and different usages at each version.

src/functionalTest/groovy/com/autonomousapps/jvm/BundleSpec.groovy

@@ -71,7 +71,7 @@ final class BundleSpec extends AbstractJvmSpec {
 
     // TODO reason needs to be updated to show that the -jvm variant is used
     expect:
-    build(gradleVersion, gradleProject.rootDir, ':consumer:reason', '--id', 'com.squareup.okio:okio')
+    build(gradleVersion, gradleProject.rootDir, ':consumer:reason', '--id', 'com.squareup.okio:okio:3.0.0')
 
     where:
     gradleVersion << [GradleVersion.current()]

src/functionalTest/groovy/com/autonomousapps/jvm/TestDuplicatedDependenciesSpec.groovy

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 package com.autonomousapps.jvm
 
+import com.autonomousapps.jvm.projects.ClasspathConfusionProject
 import com.autonomousapps.jvm.projects.DuplicatedDependenciesProject
 
 import static com.autonomousapps.utils.Runner.build
@@ -23,4 +24,24 @@ final class TestDuplicatedDependenciesSpec extends AbstractJvmSpec {
     where:
     gradleVersion << gradleVersions()
   }
+
+  def "don't suggest removing test dependency which is also on main classpath (#gradleVersion)"() {
+    given:
+    def project = new ClasspathConfusionProject()
+    gradleProject = project.gradleProject
+
+    when:
+    build(
+      gradleVersion,
+      gradleProject.rootDir,
+      'buildHealth',
+      ':consumer:reason', '--id', 'org.apache.commons:commons-collections4:4.4'
+    )
+
+    then:
+    assertThat(project.actualBuildHealth()).containsExactlyElementsIn(project.expectedBuildHealth)
+
+    where:
+    gradleVersion << gradleVersions()
+  }
 }

src/functionalTest/groovy/com/autonomousapps/jvm/projects/ClasspathConfusionProject.groovy

@@ -0,0 +1,93 @@
+package com.autonomousapps.jvm.projects
+
+import com.autonomousapps.AbstractProject
+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.actualProjectAdvice
+import static com.autonomousapps.AdviceHelper.moduleCoordinates
+import static com.autonomousapps.AdviceHelper.projectAdviceForDependencies
+import static com.autonomousapps.kit.gradle.Dependency.project
+
+final class ClasspathConfusionProject extends AbstractProject {
+
+  private static final oldCommonsCollectionsApi = new Dependency(
+    'api',
+    'org.apache.commons:commons-collections4:4.3'
+  )
+  private static final commonsCollectionsTestImplementation = new Dependency(
+    'testImplementation',
+    'org.apache.commons:commons-collections4:4.4'
+  )
+
+  final GradleProject gradleProject
+
+  ClasspathConfusionProject() {
+    this.gradleProject = build()
+  }
+
+  private GradleProject build() {
+    return newGradleProjectBuilder()
+      .withSubproject('consumer') { s ->
+        s.sources = consumerSources
+        s.withBuildScript { bs ->
+          bs.plugins = javaLibrary
+          bs.dependencies = [
+            // Brings along a different version of commons collections on main classpath
+            project('implementation', ':producer'),
+
+            // We do in fact use it here, and don't want to remove it
+            // The bug is this report of an unused dependency:
+            //   testImplementation 'org.apache.commons:commons-collections4:4.3'
+            // Not only is it used, we're actually using v4.4. So, two bugs!
+            commonsCollectionsTestImplementation,
+          ]
+        }
+      }
+      .withSubproject('producer') { s ->
+        s.withBuildScript { bs ->
+          bs.plugins = javaLibrary
+          // Expose this different version of the dep to consumers
+          bs.dependencies = [oldCommonsCollectionsApi]
+        }
+      }
+      .write()
+  }
+
+  private List<Source> consumerSources = [
+    Source.java(
+      '''\
+      package com.example;
+        
+      import org.apache.commons.collections4.Bag;
+      import org.apache.commons.collections4.bag.HashBag;
+      
+      public class Test {
+        public void compute() {
+          Bag<String> bag = new HashBag<>();
+        }
+      }
+      '''
+    )
+      .withPath('com.example', 'Test')
+      .withSourceSet('test')
+      .build()
+  ]
+
+  Set<ProjectAdvice> actualBuildHealth() {
+    return actualProjectAdvice(gradleProject)
+  }
+
+  private final Set<Advice> consumerAdvice = []
+  private final Set<Advice> producerAdvice = [
+    Advice.ofRemove(moduleCoordinates(oldCommonsCollectionsApi), 'api')
+  ]
+
+  final Set<ProjectAdvice> expectedBuildHealth = [
+    projectAdviceForDependencies(':consumer', consumerAdvice),
+    projectAdviceForDependencies(':producer', producerAdvice),
+  ]
+}

src/functionalTest/groovy/com/autonomousapps/jvm/projects/DuplicatedDependenciesProject.groovy

@@ -60,7 +60,7 @@ final class DuplicatedDependenciesProject extends AbstractProject {
 
   private final Set<Advice> projAdvice = [
     // This test project makes sure that the 'runtimeOnly' dependency does not shadow the 'implementation'
-    // dependency to the same module during analysis. Which in the past let to a wrong advice
+    // dependency to the same module during analysis. Which in the past led to a wrong advice
     // (remove implementation dependency). Reporting duplicated declarations, and recommending removing the ones that
     // do not add anything, is out of scope right now.
     // Advice.ofRemove(moduleCoordinates(commonsCollectionsRuntimeOnly), commonsCollectionsRuntimeOnly.configuration)

src/main/kotlin/com/autonomousapps/internal/utils/coordinatesStrings.kt

@@ -38,18 +38,20 @@ internal fun <T> String.matchesKey(mapEntry: Map.Entry<String, T>): Boolean {
 }
 
 internal fun <T> String.equalsKey(mapEntry: Map.Entry<String, T>): Boolean {
-  val tokens = mapEntry.key.firstCoordinatesKeySegment().split(":")
-  if (tokens.size == 3) {
-    // "groupId:artifactId:version" => "groupId:artifactId"
-    if ("${tokens[0]}:${tokens[1]}" == this) {
-      return true
-    }
+  val firstSegment = mapEntry.key.firstCoordinatesKeySegment()
+  val tokens = firstSegment.split(":")
+
+  // module coordinates, "group:artifact:version"
+  if (tokens.size == 3 && this == firstSegment) {
+    return true
   }
-  return mapEntry.key.firstCoordinatesKeySegment() == this || mapEntry.key.secondCoordinatesKeySegment() == this
+
+  return firstSegment == this || mapEntry.key.secondCoordinatesKeySegment() == this
 }
 
 private fun <T> String.startsWithKey(mapEntry: Map.Entry<String, T>) =
-  mapEntry.key.firstCoordinatesKeySegment().startsWith(this) || mapEntry.key.secondCoordinatesKeySegment()?.startsWith(this) == true
+  mapEntry.key.firstCoordinatesKeySegment().startsWith(this)
+    || mapEntry.key.secondCoordinatesKeySegment()?.startsWith(this) == true
 
 /** First key segment is always 'group:name' coordinates */
 internal fun String.firstCoordinatesKeySegment(): String =

src/main/kotlin/com/autonomousapps/model/declaration/Declaration.kt

@@ -26,6 +26,10 @@ internal data class Declaration(
 ) {
 
   val bucket: Bucket by unsafeLazy { Bucket.of(configurationName) }
-  fun variant(supportedSourceSets: Set<String>, hasCustomSourceSets: Boolean): Variant? =
-    Variant.of(configurationName, supportedSourceSets, hasCustomSourceSets)
+
+  fun gav(): String = if (version != null) "$identifier:$version" else identifier
+
+  fun variant(supportedSourceSets: Set<String>, hasCustomSourceSets: Boolean): Variant? {
+    return Variant.of(configurationName, supportedSourceSets, hasCustomSourceSets)
+  }
 }

src/main/kotlin/com/autonomousapps/model/intermediates/Usage.kt

@@ -4,6 +4,7 @@ package com.autonomousapps.model.intermediates
 
 import com.autonomousapps.model.Advice
 import com.autonomousapps.model.Coordinates
+import com.autonomousapps.model.ModuleCoordinates
 import com.autonomousapps.model.declaration.Bucket
 import com.autonomousapps.model.declaration.Variant
 import com.squareup.moshi.JsonClass
@@ -44,10 +45,10 @@ internal class UsageBuilder(
 
     traces.forEach { report ->
       report.dependencies.forEach { trace ->
-        theDependencyUsages.add(report, trace)
+        theDependencyUsages.merge(report, trace)
       }
       report.annotationProcessors.forEach { trace ->
-        theAnnotationProcessingUsages.add(report, trace)
+        theAnnotationProcessingUsages.merge(report, trace)
       }
     }
 
@@ -79,7 +80,7 @@ internal class UsageBuilder(
     }
   }
 
-  private fun MutableMap<Coordinates, MutableSet<Usage>>.add(
+  private fun MutableMap<Coordinates, MutableSet<Usage>>.merge(
     report: DependencyTraceReport,
     trace: DependencyTraceReport.Trace
   ) {
@@ -90,6 +91,26 @@ internal class UsageBuilder(
       bucket = trace.bucket,
       reasons = trace.reasons
     )
+
+    val other = keys.find { it.identifier == trace.coordinates.identifier }
+    if (other != null) {
+      val otherVersion = (other as? ModuleCoordinates)?.resolvedVersion
+      val thisVersion = (trace.coordinates as? ModuleCoordinates)?.resolvedVersion
+      if (otherVersion != null && thisVersion != null && otherVersion != thisVersion) {
+        // This mutates the existing set in place
+        val usages = get(other)!!.apply { add(usage) }
+
+        // If the new coordinates are "greater than" the other coordinates, add the new and remove the old
+        if (thisVersion > otherVersion) {
+          put(trace.coordinates, usages)
+          remove(other)
+        }
+
+        // We're done
+        return
+      }
+    }
+
     merge(trace.coordinates, mutableSetOf(usage)) { acc, inc ->
       acc.apply { addAll(inc) }
     }

src/main/kotlin/com/autonomousapps/tasks/ComputeAdviceTask.kt

@@ -164,7 +164,7 @@ abstract class ComputeAdviceTask @Inject constructor(
       val annotationProcessorUsages = usageBuilder.annotationProcessingUsages
       val supportedSourceSets = parameters.supportedSourceSets.get()
       val isKaptApplied = parameters.kapt.get()
-      val map = nonTransitiveDependencies(dependencyGraph)
+      val nonTransitiveDependencies = computeNonTransitiveDependenciesMap(dependencyGraph)
 
       val ignoreKtx = parameters.ignoreKtx.get() || parameters.ignoreKtx2.get()
 
@@ -183,7 +183,7 @@ abstract class ComputeAdviceTask @Inject constructor(
         dependencyUsages = dependencyUsages,
         annotationProcessorUsages = annotationProcessorUsages,
         declarations = declarations,
-        nonTransitiveDependencies = map,
+        nonTransitiveDependencies = nonTransitiveDependencies,
         supportedSourceSets = supportedSourceSets,
         isKaptApplied = isKaptApplied,
       )
@@ -210,15 +210,16 @@ abstract class ComputeAdviceTask @Inject constructor(
 
     /**
      * Returns the set of non-transitive dependencies from [dependencyGraph], associated with the source sets
-     * [Variant.variant][com.autonomousapps.model.declaration.Variant.variant] they're used by.
+     * ([Variant.variant][com.autonomousapps.model.declaration.Variant.variant]) they're used by.
      */
-    private fun nonTransitiveDependencies(
+    private fun computeNonTransitiveDependenciesMap(
       dependencyGraph: Map<String, DependencyGraphView>,
     ): SetMultimap<String, Variant> {
       return newSetMultimap<String, Variant>().apply {
         dependencyGraph.values.map { graphView ->
           val root = graphView.graph.root()
           graphView.graph.children(root).forEach { nonTransitiveDependency ->
+            // TODO: just identifier and not gav()?
             put(nonTransitiveDependency.identifier, graphView.variant)
           }
         }
@@ -316,6 +317,7 @@ internal class DependencyAdviceBuilder(
             bundledTraces += BundleTrace.DeclaredParent(parent = parent, child = originalCoordinates)
             null
           }
+
           // Optionally map given advice to "primary" advice, if bundle has a primary
           advice.isAdd() -> {
             val p = bundles.maybePrimary(advice, originalCoordinates)
@@ -330,6 +332,7 @@ internal class DependencyAdviceBuilder(
             bundledTraces += BundleTrace.UsedChild(parent = originalCoordinates, child = child)
             null
           }
+
           // If the advice has a used child, don't change it
           advice.isAnyChange() && bundles.hasUsedChild(originalCoordinates) -> {
             val child = bundles.findUsedChild(originalCoordinates)!!

src/main/kotlin/com/autonomousapps/transform/StandardTransform.kt

@@ -386,11 +386,11 @@ internal class StandardTransform(
 
 private fun Set<Declaration>.forCoordinates(coordinates: Coordinates): Set<Declaration> {
   return asSequence()
-    .filter {
-      it.identifier == coordinates.identifier ||
+    .filter { declaration ->
+      declaration.identifier == coordinates.identifier
         // In the special case of IncludedBuildCoordinates, the declaration might be a 'project(...)' dependency
         // if subprojects inside an included build depend on each other.
-        (coordinates is IncludedBuildCoordinates) && it.identifier == coordinates.resolvedProject.identifier
+        || (coordinates is IncludedBuildCoordinates) && declaration.identifier == coordinates.resolvedProject.identifier
     }
     .filter { it.isJarDependency() && it.gradleVariantIdentification.variantMatches(coordinates) }
     .toSet()

src/test/kotlin/com/autonomousapps/tasks/ReasonTaskTest.kt

@@ -58,7 +58,7 @@ class ReasonTaskTest {
 
   @Test
   fun shouldMatchEqualLibrary() {
-    val key = findFilteredDependencyKey(entries, "com.squareup.okio:okio")
+    val key = findFilteredDependencyKey(entries, "com.squareup.okio:okio:3.0.0")
     assertEquals("com.squareup.okio:okio:3.0.0", key)
   }