fix(yarn2): Avoid failing on ambiguously resolved locators

plugins/package-managers/node/src/main/kotlin/yarn2/Yarn2.kt

@@ -160,7 +160,7 @@ class Yarn2(override val descriptor: PluginDescriptor = Yarn2Factory.descriptor,
 
         val scopes = Scope.entries.filterNot { scope -> scope.isExcluded(excludes, includes) }
 
-        return workspaceModuleDirs.map { projectDir ->
+        val results = workspaceModuleDirs.map { projectDir ->
             val packageJsonFile = projectDir / NodePackageManagerType.DEFINITION_FILE
             val packageJson = parsePackageJson(packageJsonFile)
             val project = parseProject(packageJsonFile, analysisRoot)
@@ -175,12 +175,26 @@ class Yarn2(override val descriptor: PluginDescriptor = Yarn2Factory.descriptor,
                 graphBuilder.addDependencies(project.id, scope.descriptor, dependencies)
             }
 
-            ProjectAnalyzerResult(
+            projectDir to ProjectAnalyzerResult(
                 project = project.copy(scopeNames = scopes.getNames()),
                 packages = emptySet(),
                 issues = issues
             )
         }
+
+        // Both the resolution cache and the dependency graph builder's deduplication span all projects of the
+        // definition file, so ambiguities cannot reliably be attributed to individual projects. Report them once,
+        // on the root project.
+        val ambiguityIssues = handler.drainAmbiguityIssues()
+        val rootDir = workingDir.realFile
+
+        return results.map { (projectDir, result) ->
+            if (projectDir == rootDir && ambiguityIssues.isNotEmpty()) {
+                result.copy(issues = result.issues + ambiguityIssues)
+            } else {
+                result
+            }
+        }
     }
 
     private fun installDependencies(workingDir: File) {

plugins/package-managers/node/src/main/kotlin/yarn2/Yarn2DependencyHandler.kt

@@ -27,6 +27,8 @@ import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.Issue
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageLinkage
+import org.ossreviewtoolkit.model.Severity
+import org.ossreviewtoolkit.model.createAndLogIssue
 import org.ossreviewtoolkit.model.utils.DependencyHandler
 import org.ossreviewtoolkit.plugins.packagemanagers.node.ModuleInfoResolver
 import org.ossreviewtoolkit.plugins.packagemanagers.node.NodePackageManagerType
@@ -44,6 +46,19 @@ internal class Yarn2DependencyHandler(
     private val packageJsonForModuleId = mutableMapOf<String, PackageJson>()
     private val packageInfoForLocator = mutableMapOf<String, PackageInfo>()
 
+    /**
+     * A map from the real locator of a dependency to an explanation, for dependencies that [resolvePackageInfo] could
+     * only resolve ambiguously by picking the highest of multiple remaining candidates. Drained by
+     * [drainAmbiguityIssues].
+     */
+    private val ambiguousResolutions = mutableMapOf<String, String>()
+
+    /**
+     * A cache for the results of [resolvePackageInfo], as the same dependency is resolved repeatedly while the
+     * dependency graph is built, and the fallback resolution is expensive.
+     */
+    private val packageInfoForDependency = mutableMapOf<PackageInfo.Dependency, PackageInfo>()
+
     fun setContext(
         workingDir: File,
         packageJsonForModuleId: Map<String, PackageJson>,
@@ -60,6 +75,12 @@ internal class Yarn2DependencyHandler(
             clear()
             putAll(packageJsonForModuleId)
         }
+
+        // Cached resolutions are not reusable across definition files, as they are resolved against
+        // `packageInfoForLocator`, whose content is replaced here.
+        packageInfoForDependency.clear()
+
+        ambiguousResolutions.clear()
     }
 
     override fun identifierFor(dependency: PackageInfo): Identifier =
@@ -76,6 +97,24 @@ internal class Yarn2DependencyHandler(
     override fun linkageFor(dependency: PackageInfo): PackageLinkage =
         if (dependency.isProject) PackageLinkage.PROJECT_DYNAMIC else PackageLinkage.DYNAMIC
 
+    /**
+     * Return the issues for all ambiguously resolved dependencies recorded since the last call (or [setContext]),
+     * and reset them. This is meant to be called once per definition file, after all of its projects were processed:
+     * both the resolution cache and the dependency graph builder's deduplication span all projects of a definition
+     * file, so ambiguities cannot reliably be attributed to individual projects. Note that these issues are also not
+     * reported via [DependencyHandler.issuesFor], because it only associates issues with a package when it is added
+     * to the dependency graph for the first time. A package that was already added via an unambiguously resolved
+     * dependency would silently drop the issue.
+     */
+    fun drainAmbiguityIssues(): List<Issue> =
+        ambiguousResolutions.values.map { message ->
+            createAndLogIssue(
+                source = Yarn2Factory.descriptor.displayName,
+                message = message,
+                severity = Severity.WARNING
+            )
+        }.also { ambiguousResolutions.clear() }
+
     override fun createPackage(dependency: PackageInfo, issues: MutableCollection<Issue>): Package? {
         val packageJson = packageJsonForModuleId[dependency.moduleId]?.takeUnless { dependency.isProject }
             ?: return null
@@ -84,7 +123,14 @@ internal class Yarn2DependencyHandler(
     }
 
     /**
-     * Obtain the [PackageInfo] object for the given [dependency].
+     * Obtain the [PackageInfo] object for the given [dependency], see [resolvePackageInfo]. Results are cached in
+     * [packageInfoForDependency].
+     */
+    internal fun packageInfoFor(dependency: PackageInfo.Dependency): PackageInfo =
+        packageInfoForDependency.getOrPut(dependency) { resolvePackageInfo(dependency) }
+
+    /**
+     * Resolve the given [dependency] to the [PackageInfo] object it refers to.
      *
      * Try the `realLocator` first to correctly handle virtual packages. If that fails, try to construct the real
      * locator from the virtual package's actual resolved version (handles virtual packages whose `children.version`
@@ -93,11 +139,19 @@ internal class Yarn2DependencyHandler(
      * If both targeted lookups fail, fall back to searching the map for all installed non-virtual, non-project
      * versions of the same module by name. This handles the case where Yarn's `resolutions` feature (or similar
      * mechanisms) cause a non-virtual dependency locator to reference a version that is not present in the map,
-     * while a different version of the same module was actually installed. If exactly one candidate is found, it
-     * is used. If multiple candidates are found, the semver range from the [dependency]'s descriptor is used to
-     * narrow down the candidates. If after all fallbacks the result is still not unique, an exception is thrown.
+     * while a different version of the same module was actually installed. The candidates found by name are first
+     * narrowed down to those matching the semver range from the [dependency]'s descriptor (keeping all versions if
+     * the range cannot be determined or nothing matches it), and then to those of the locator's patch or non-patch
+     * type (keeping the other type if none matches). The version is checked before the type, as attributing the
+     * correct version has priority. The type check handles packages that exist both as a plain npm package and as a
+     * compat-patched variant (e.g. via `@yarnpkg/plugin-compat`), which would otherwise lead to ambiguous
+     * resolution.
+     *
+     * If the result is still not unique after all fallbacks, the highest version among the remaining candidates is
+     * picked, and a warning explaining the pick is recorded in [ambiguousResolutions]. An exception is only thrown
+     * if no candidate for the module exists at all.
      */
-    internal fun packageInfoFor(dependency: PackageInfo.Dependency): PackageInfo {
+    private fun resolvePackageInfo(dependency: PackageInfo.Dependency): PackageInfo {
         packageInfoForLocator[dependency.realLocator]?.let { return it }
 
         // Fallback for virtual packages: derive the real locator from the virtual package's resolved version.
@@ -108,32 +162,48 @@ internal class Yarn2DependencyHandler(
 
         // Fallback for version mismatches caused by Yarn's `resolutions` feature: find installed versions of the
         // same module by name, ignoring the exact version in the locator.
-        val moduleName = Locator.parse(dependency.realLocator).moduleName
-        val candidates = packageInfoForLocator.values.filter {
+        val realLocator = Locator.parse(dependency.realLocator)
+        val moduleName = realLocator.moduleName
+        val installed = packageInfoForLocator.values.filter {
             it.moduleName == moduleName && !it.isProject && !it.isVirtual
         }
 
+        if (installed.isEmpty()) {
+            error(
+                "Could not find a PackageInfo for locator '${dependency.realLocator}'. No entry for module " +
+                    "'$moduleName' exists in ${packageInfoForLocator.keys}."
+            )
+        }
+
+        // Narrow down by the descriptor's semver range first, as attributing the correct version has priority over
+        // matching the locator's patch or non-patch type.
+        val versionMatches = installed.matchingVersionRange(dependency)?.ifEmpty { null } ?: installed
+
+        // Prefer candidates of the locator's patch or non-patch type, falling back to the other type if none match.
+        val candidates = versionMatches.filter { it.isPatch == realLocator.isPatch }.ifEmpty { versionMatches }
+
         candidates.singleOrNull()?.also {
             logger.debug {
-                "Resolved locator '${dependency.realLocator}' to '${it.value}' via module name lookup."
+                "Resolved locator '${dependency.realLocator}' to '${it.value}' via module name lookup on " +
+                    "descriptor '${dependency.descriptor}'."
             }
 
             return it
         }
 
-        if (candidates.isEmpty()) {
-            error(
-                "Could not find a PackageInfo for locator '${dependency.realLocator}'. No entry for module " +
-                    "'$moduleName' exists in ${packageInfoForLocator.keys}."
+        // Still ambiguous, so pick the highest version among the remaining candidates and record a warning.
+        val resolved = candidates.highestVersion()
+            ?: error(
+                "Could not unambiguously resolve locator '${dependency.realLocator}'. Found ${installed.size} " +
+                    "installed versions of module '$moduleName': ${installed.map { it.value }}."
             )
-        }
 
-        candidates.matchVersionRange(dependency)?.let { return it }
+        ambiguousResolutions[dependency.realLocator] = "Could not unambiguously map the locator " +
+            "'${dependency.realLocator}' to one of the installed versions of '$moduleName': " +
+            "${candidates.map { it.value }}. Picked '${resolved.value}', the candidate with the highest version. " +
+            "The dependency graph may therefore attribute this dependency to the wrong version."
 
-        error(
-            "Could not unambiguously resolve locator '${dependency.realLocator}'. Found ${candidates.size} " +
-                "installed versions of module '$moduleName': ${candidates.map { it.value }}."
-        )
+        return resolved
     }
 }
 
@@ -143,10 +213,10 @@ internal val PackageInfo.isProject: Boolean
 internal val PackageInfo.isVirtual: Boolean
     get() = Locator.parse(value).isVirtual
 
+internal val PackageInfo.isPatch: Boolean
+    get() = Locator.parse(value).isPatch
+
 internal val PackageInfo.moduleName: String
-    // TODO: Handle patched packages different than non-patched ones.
-    // Patch packages have locators as e.g. the following, where the first component ends with "@patch".
-    // resolve@patch:resolve@npm%3A1.22.8#optional!builtin<compat/resolve>::version=1.22.8&hash=c3c19d
     get() = Locator.parse(value).moduleName
 
 internal val PackageInfo.moduleId: String
@@ -177,26 +247,44 @@ internal data class Locator(
         (remainder.startsWith("virtual:") && "#workspace:" in remainder)
 
     val isVirtual: Boolean = remainder.startsWith("virtual:") && !isProject
+
+    val isPatch: Boolean = remainder.startsWith("patch:")
 }
 
 /**
- * Try to find a single [PackageInfo] from this collection that matches the given [dependency] taking semantic
- * version ranges into account.
+ * Narrow this collection down to the entries whose version matches the semver range of the given [dependency]'s
+ * `npm:` or `patch:` descriptor. Return `null` if the range cannot be determined, or the (possibly empty or still
+ * not unique) list of matching entries otherwise.
  */
-private fun Collection<PackageInfo>.matchVersionRange(dependency: PackageInfo.Dependency): PackageInfo? {
+private fun Collection<PackageInfo>.matchingVersionRange(dependency: PackageInfo.Dependency): List<PackageInfo>? {
     val descriptorRemainder = Locator.parse(dependency.descriptor).remainder
-    return descriptorRemainder.withoutPrefix("npm:")?.let { rangeSpec ->
-        runCatching { RangeListFactory.create(rangeSpec) }.getOrNull()?.let { range ->
-            val matchingCandidates = filter { candidate ->
-                Semver.coerce(candidate.children.version)?.let { range.isSatisfiedBy(it) } == true
-            }
+    val rangeSpec = descriptorRemainder.withoutPrefix("npm:")
+        ?: extractNpmRangeFromPatchRemainder(descriptorRemainder)
+        ?: return null
 
-            matchingCandidates.singleOrNull()?.also {
-                logger.debug {
-                    "Resolved locator '${dependency.realLocator}' to '${it.value}' via semver range " +
-                        "matching on descriptor '${dependency.descriptor}'."
-                }
-            }
-        }
+    return runCatching { RangeListFactory.create(rangeSpec) }.getOrNull()?.let { range ->
+        filter { candidate -> Semver.coerce(candidate.children.version)?.let { range.isSatisfiedBy(it) } == true }
+    }
+}
+
+/**
+ * Return the entry from this collection with the highest [PackageInfo.Children.version], or `null` if this
+ * collection is empty or none of its entries have a version that can be parsed as a [Semver].
+ */
+private fun Collection<PackageInfo>.highestVersion(): PackageInfo? =
+    mapNotNull { info -> Semver.coerce(info.children.version)?.let { it to info } }.maxByOrNull { it.first }?.second
+
+/**
+ * Extract the npm version range embedded in a [patchRemainder] like
+ * `patch:resolve@npm%3A^2.0.0-next.5#optional!builtin<compat/resolve>`, returning `^2.0.0-next.5`, or `null` if
+ * [patchRemainder] is not a patch descriptor or the range cannot be extracted.
+ */
+private fun extractNpmRangeFromPatchRemainder(patchRemainder: String): String? {
+    if (!patchRemainder.startsWith("patch:")) return null
+    // The npm version range follows "@npm%3A" (URL-encoded "@npm:") or "@npm:" and ends at "#".
+    val afterNpmPrefix = patchRemainder.substringAfter("@npm%3A", "").ifEmpty {
+        patchRemainder.substringAfter("@npm:", "")
     }
+
+    return afterNpmPrefix.takeIf { it.isNotEmpty() }?.substringBefore("#")
 }