Make dependencies keep their positions when fetching (#2266)

MaciejG604 · web-flow · commit 9177edf836fe · 2023-07-06T10:50:28.000Z
Add ResolutionError handling
diff --git a/modules/build/src/main/scala/scala/build/Bloop.scala b/modules/build/src/main/scala/scala/build/Bloop.scala
@@ -71,7 +71,7 @@ object Bloop {
     either {
       val res = value {
         Artifacts.artifacts(
-          Positioned.none(Seq(dep)),
+          Seq(Positioned.none(dep)),
           Nil,
           Some(params),
           logger,
diff --git a/modules/build/src/main/scala/scala/build/ReplArtifacts.scala b/modules/build/src/main/scala/scala/build/ReplArtifacts.scala
@@ -53,14 +53,14 @@ object ReplArtifacts {
       addScalapy.map(ver => dep"${Artifacts.scalaPyOrganization(ver)}::scalapy-core::$ver").toSeq
     val allDeps = dependencies ++ Seq(dep"com.lihaoyi:::ammonite:$ammoniteVersion") ++ scalapyDeps
     val replArtifacts = Artifacts.artifacts(
-      Positioned.none(allDeps),
+      allDeps.map(Positioned.none),
       extraRepositories,
       Some(scalaParams),
       logger,
       cache.withMessage(s"Downloading Ammonite $ammoniteVersion")
     )
     val replSourceArtifacts = Artifacts.artifacts(
-      Positioned.none(allDeps),
+      allDeps.map(Positioned.none),
       extraRepositories,
       Some(scalaParams),
       logger,
@@ -95,7 +95,7 @@ object ReplArtifacts {
     val allDeps = dependencies ++ Seq(replDep) ++ scalapyDeps
     val replArtifacts =
       Artifacts.artifacts(
-        Positioned.none(allDeps),
+        allDeps.map(Positioned.none),
         repositories,
         Some(scalaParams),
         logger,
diff --git a/modules/build/src/test/scala/scala/build/tests/DirectiveTests.scala b/modules/build/src/test/scala/scala/build/tests/DirectiveTests.scala
@@ -9,7 +9,12 @@ import scala.build.tests.util.BloopServer
 import build.Ops.EitherThrowOps
 import dependency.AnyDependency
 
-import scala.build.errors.{DependencyFormatError, ToolkitDirectiveMissingVersionError}
+import scala.build.errors.{
+  CompositeBuildException,
+  DependencyFormatError,
+  FetchingDependenciesError,
+  ToolkitDirectiveMissingVersionError
+}
 
 class DirectiveTests extends munit.FunSuite {
 
@@ -421,4 +426,50 @@ class DirectiveTests extends munit.FunSuite {
         }
     }
   }
+
+  test("separate dependency resolution errors for each dependency") {
+    val testInputs = TestInputs(
+      os.rel / "simple.sc" ->
+        """//> using dep org.xyz::foo:0.0.1
+          |//> using dep com.lihaoyi::os-lib:0.9.1 org.qwerty::bar:0.0.1
+          |""".stripMargin
+    )
+    testInputs.withBuild(baseOptions, buildThreads, bloopConfigOpt) {
+      (root, _, maybeBuild) =>
+        expect(maybeBuild.isLeft)
+        val errors = maybeBuild.left.toOption.get
+
+        errors match {
+          case error: CompositeBuildException =>
+            expect(error.exceptions.length == 2)
+            expect(error.exceptions.forall(_.isInstanceOf[FetchingDependenciesError]))
+            expect(error.exceptions.forall(_.positions.length == 1))
+
+            {
+              val xyzError = error.exceptions.find(_.message.contains("org.xyz")).get
+              expect(xyzError.message.startsWith("Error downloading org.xyz:foo"))
+              expect(!xyzError.message.contains("com.lihaoyi"))
+              expect(!xyzError.message.contains("org.qwerty"))
+              expect(xyzError.positions.head == Position.File(
+                Right(root / "simple.sc"),
+                (0, 14),
+                (0, 32)
+              ))
+            }
+
+            {
+              val qwertyError = error.exceptions.find(_.message.contains("org.qwerty")).get
+              expect(qwertyError.message.contains("Error downloading org.qwerty:bar"))
+              expect(!qwertyError.message.contains("com.lihaoyi"))
+              expect(!qwertyError.message.contains("org.xyz"))
+              expect(qwertyError.positions.head == Position.File(
+                Right(root / "simple.sc"),
+                (1, 40),
+                (1, 61)
+              ))
+            }
+          case _ => fail("unexpected BuildException type")
+        }
+    }
+  }
 }
diff --git a/modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala b/modules/cli/src/main/scala/scala/cli/commands/ScalaCommand.scala
@@ -217,7 +217,7 @@ abstract class ScalaCommand[T <: HasGlobalOptions](implicit myParser: Parser[T],
           if (shared.helpGroups.helpScaladoc) {
             val docArtifacts = value {
               Artifacts.fetch(
-                Positioned.none(Seq(dep"org.scala-lang::scaladoc:${scalaParams.scalaVersion}")),
+                Seq(Positioned.none(dep"org.scala-lang::scaladoc:${scalaParams.scalaVersion}")),
                 value(buildOptions.finalRepositories),
                 Some(scalaParams),
                 logger,
@@ -246,7 +246,7 @@ abstract class ScalaCommand[T <: HasGlobalOptions](implicit myParser: Parser[T],
           else {
             val fmtArtifacts = value {
               Artifacts.fetch(
-                Positioned.none(Seq(
+                Seq(Positioned.none(
                   dep"${Constants.scalafmtOrganization}:${Constants.scalafmtName}:${Constants.defaultScalafmtVersion}"
                 )),
                 value(buildOptions.finalRepositories),
diff --git a/modules/cli/src/main/scala/scala/cli/commands/doc/Doc.scala b/modules/cli/src/main/scala/scala/cli/commands/doc/Doc.scala
@@ -142,7 +142,7 @@ object Doc extends ScalaCommand[DocOptions] {
       case Some(scalaParams) =>
         val res = value {
           Artifacts.fetch(
-            Positioned.none(Seq(dep"org.scala-lang::scaladoc:${scalaParams.scalaVersion}")),
+            Seq(Positioned.none(dep"org.scala-lang::scaladoc:${scalaParams.scalaVersion}")),
             value(build.options.finalRepositories),
             Some(scalaParams),
             logger,
diff --git a/modules/cli/src/main/scala/scala/cli/commands/new/New.scala b/modules/cli/src/main/scala/scala/cli/commands/new/New.scala
@@ -20,7 +20,7 @@ object New extends ScalaCommand[NewOptions] {
   override def runCommand(options: NewOptions, remainingArgs: RemainingArgs, logger: Logger): Unit =
     val scalaParameters = ScalaParameters(Constants.defaultScala213Version)
     val fetchedGiter8 = Artifacts.fetch(
-      Positioned.none(giter8Dependency),
+      giter8Dependency.map(Positioned.none),
       Seq.empty,
       Some(scalaParameters),
       logger,
diff --git a/modules/cli/src/main/scala/scala/cli/commands/package0/Package.scala b/modules/cli/src/main/scala/scala/cli/commands/package0/Package.scala
@@ -757,7 +757,7 @@ object Package extends ScalaCommand[PackageOptions] with BuildCommandHelpers {
       if (build.options.notForBloopOptions.doSetupPython.getOrElse(false)) {
         val res = value {
           Artifacts.fetch(
-            Positioned.none(Seq(
+            Seq(Positioned.none(
               dep"${Constants.pythonInterfaceOrg}:${Constants.pythonInterfaceName}:${Constants.pythonInterfaceVersion}"
             )),
             Nil,
diff --git a/modules/cli/src/main/scala/scala/cli/commands/pgp/PgpExternalCommand.scala b/modules/cli/src/main/scala/scala/cli/commands/pgp/PgpExternalCommand.scala
@@ -194,7 +194,7 @@ object PgpExternalCommand {
 
       val signingClassPath = value {
         scala.build.Artifacts.fetch0(
-          Positioned.none(Seq(jvmSigningDep.toCs)),
+          Seq(Positioned.none(jvmSigningDep.toCs)),
           extraRepos,
           None,
           Nil,
diff --git a/modules/cli/src/main/scala/scala/cli/internal/ScalaJsLinker.scala b/modules/cli/src/main/scala/scala/cli/internal/ScalaJsLinker.scala
@@ -54,7 +54,7 @@ object ScalaJsLinker {
           case Right(()) =>
             val linkerClassPath = value {
               scala.build.Artifacts.fetch0(
-                Positioned.none(Seq(scalaJsCliDep.toCs)),
+                Seq(Positioned.none(scalaJsCliDep.toCs)),
                 extraRepos,
                 None,
                 forcedVersions.map { case (m, v) => (m.toCs, v) },
diff --git a/modules/cli/src/main/scala/scala/cli/launcher/LauncherCli.scala b/modules/cli/src/main/scala/scala/cli/launcher/LauncherCli.scala
@@ -32,7 +32,7 @@ object LauncherCli {
 
     val fetchedScalaCli =
       Artifacts.fetch(
-        Positioned.none(scalaCliDependency),
+        scalaCliDependency.map(Positioned.none),
         snapshotsRepo,
         Some(scalaParameters),
         logger,
diff --git a/modules/options/src/main/scala/scala/build/Artifacts.scala b/modules/options/src/main/scala/scala/build/Artifacts.scala
@@ -2,6 +2,7 @@ package scala.build
 
 import coursier.cache.FileCache
 import coursier.core.{Classifier, Module, ModuleName, Organization, Repository, Version}
+import coursier.error.{CoursierError, ResolutionError}
 import coursier.parse.RepositoryParser
 import coursier.util.Task
 import coursier.{Dependency => CsDependency, Fetch, Resolution, core => csCore, util => csUtil}
@@ -139,7 +140,7 @@ object Artifacts {
               val posDep0 =
                 posDep.map(dep => dep.copy(userParams = dep.userParams + ("intransitive" -> None)))
               artifacts(
-                posDep0.map(Seq(_)),
+                Seq(posDep0),
                 allExtraRepositories,
                 Some(scalaArtifactsParams.params),
                 logger,
@@ -156,7 +157,7 @@ object Artifacts {
 
         val compilerArtifacts = value {
           artifacts(
-            Positioned.none(compilerDependencies),
+            compilerDependencies.map(Positioned.none),
             allExtraRepositories,
             Some(scalaArtifactsParams.params),
             logger,
@@ -185,7 +186,7 @@ object Artifacts {
             Some(
               value {
                 fetch0(
-                  Positioned.none(dependency),
+                  dependency.map(Positioned.none),
                   allExtraRepositories,
                   None,
                   forcedVersions,
@@ -212,7 +213,7 @@ object Artifacts {
             Some(
               value {
                 fetch0(
-                  Positioned.none(dependency),
+                  dependency.map(Positioned.none),
                   allExtraRepositories,
                   None,
                   Nil,
@@ -299,7 +300,7 @@ object Artifacts {
 
     val fetchRes = value {
       fetch(
-        Positioned.sequence(updatedDependencies),
+        updatedDependencies,
         allExtraRepositories,
         scalaArtifactsParamsOpt.map(_.params),
         logger,
@@ -320,9 +321,9 @@ object Artifacts {
               else Nil
             value {
               artifacts(
-                Positioned.none(
-                  Seq(dep"$runnerOrganization::$runnerModuleName:$runnerVersion,intransitive")
-                ),
+                Seq(Positioned.none(
+                  dep"$runnerOrganization::$runnerModuleName:$runnerVersion,intransitive"
+                )),
                 extraRepositories ++ maybeSnapshotRepo,
                 scalaArtifactsParamsOpt.map(_.params),
                 logger,
@@ -343,7 +344,7 @@ object Artifacts {
         .map { posDep =>
           val cache0 = cache.withMessage(s"Downloading javac plugin ${posDep.value.render}")
           artifacts(
-            posDep.map(Seq(_)),
+            Seq(posDep),
             allExtraRepositories,
             scalaArtifactsParamsOpt.map(_.params),
             logger,
@@ -383,7 +384,7 @@ object Artifacts {
   }
 
   private[build] def artifacts(
-    dependencies: Positioned[Seq[AnyDependency]],
+    dependencies: Seq[Positioned[AnyDependency]],
     extraRepositories: Seq[Repository],
     paramsOpt: Option[ScalaParameters],
     logger: Logger,
@@ -407,40 +408,46 @@ object Artifacts {
   }
 
   def fetch(
-    dependencies: Positioned[Seq[AnyDependency]],
+    dependencies: Seq[Positioned[AnyDependency]],
     extraRepositories: Seq[Repository],
     paramsOpt: Option[ScalaParameters],
     logger: Logger,
     cache: FileCache[Task],
     classifiersOpt: Option[Set[String]],
     maybeRecoverOnError: BuildException => Option[BuildException] = e => Some(e)
   ): Either[BuildException, Fetch.Result] = either {
-    val coursierDependenciesWithFallbacks = value {
-      dependencies.value
-        .map(Positioned(dependencies.positions, _))
-        .map(dep => dep.toCs(paramsOpt).map(csDep => (dep.value, csDep.value)))
+    val coursierDependenciesWithFallbacks
+      : Seq[Positioned[(CsDependency, Option[((Module, String), (URL, Boolean))])]] = value {
+      dependencies
+        .map(dep =>
+          dep.toCs(paramsOpt)
+            .map { case Positioned(pos, csDep) => Positioned(pos, (dep.value, csDep)) }
+        )
         .map(_.left.map(maybeRecoverOnError))
         .flatMap {
           case Left(Some(e: NoScalaVersionProvidedError)) => Some(Left(e))
           case Left(_)                                    => None
-          case Right(dep)                                 => Some(Right(dep))
+          case Right(depTuple)                            => Some(Right(depTuple))
         }
         .sequence
         .left.map(CompositeBuildException(_))
-        .map(_.map {
-          case (dep, csDep) =>
-            val maybeUrl = dep.userParams.get("url").flatten.map(new URL(_))
-            val fallback = maybeUrl.map(url => (csDep.module -> csDep.version) -> (url -> true))
-            (csDep, fallback)
-        })
+        .map(positionedDepTupleSeq =>
+          positionedDepTupleSeq.map {
+            case Positioned(positions, (dep, csDep)) =>
+              val maybeUrl = dep.userParams.get("url").flatten.map(new URL(_))
+              val fallback = maybeUrl.map(url => (csDep.module -> csDep.version) -> (url -> true))
+              Positioned(positions, (csDep, fallback))
+          }
+        )
     }
-    val coursierDependenciesWithFallbacks0
-      : Positioned[Seq[(CsDependency, Option[((Module, String), (URL, Boolean))])]] =
-      dependencies.map(_ => coursierDependenciesWithFallbacks)
-    val coursierDependencies: Positioned[Seq[CsDependency]] =
-      coursierDependenciesWithFallbacks0.map(_.map(_._1))
+
+    val coursierDependencies: Seq[Positioned[CsDependency]] =
+      coursierDependenciesWithFallbacks.map(_.map(_._1))
     val fallbacks: Map[(Module, String), (URL, Boolean)] =
-      coursierDependenciesWithFallbacks0.value.flatMap(_._2).toMap
+      coursierDependenciesWithFallbacks.map(_.value)
+        .flatMap(_._2)
+        .toMap
+
     value {
       fetch0(
         coursierDependencies,
@@ -456,7 +463,7 @@ object Artifacts {
   }
 
   def fetch0(
-    dependencies: Positioned[Seq[coursier.Dependency]],
+    dependencies: Seq[Positioned[coursier.Dependency]],
     extraRepositories: Seq[Repository],
     forceScalaVersionOpt: Option[String],
     forcedVersions: Seq[(coursier.Module, String)],
@@ -466,7 +473,7 @@ object Artifacts {
     fallbacks: Map[(Module, String), (URL, Boolean)] = Map.empty
   ): Either[BuildException, Fetch.Result] = either {
     logger.debug {
-      s"Fetching ${dependencies.value}" +
+      s"Fetching ${dependencies.map(_.value)}" +
         (if (extraRepositories.isEmpty) "" else s", adding $extraRepositories")
     }
 
@@ -500,7 +507,7 @@ object Artifacts {
     var fetcher = coursier.Fetch()
       .withCache(cache)
       .addRepositories(extraRepositoriesWithFallback*)
-      .addDependencies(dependencies.value*)
+      .addDependencies(dependencies.map(_.value)*)
       .mapResolutionParams(_.addForceVersion(forceVersion*))
     for (classifiers <- classifiersOpt) {
       if (classifiers("_"))
@@ -513,7 +520,28 @@ object Artifacts {
       fetcher.eitherResult()
     }
     value {
-      res.left.map(ex => new FetchingDependenciesError(ex, dependencies.positions))
+      res.left.map {
+        case ex: ResolutionError.Several =>
+          CompositeBuildException(
+            ex.errors.map(toFetchingDependenciesError(dependencies, _))
+          )
+        case ex: ResolutionError.Simple =>
+          toFetchingDependenciesError(dependencies, ex)
+        case ex => new FetchingDependenciesError(ex, dependencies.flatMap(_.positions))
+      }
     }
   }
+
+  def toFetchingDependenciesError(
+    dependencies: Seq[Positioned[coursier.Dependency]],
+    resolutionError: coursier.error.ResolutionError.Simple
+  ) = resolutionError match {
+    case ex: ResolutionError.CantDownloadModule =>
+      val errorPositions = dependencies.collect {
+        case Positioned(pos, dep)
+            if ex.module == dep.module => pos
+      }.flatten
+      new FetchingDependenciesError(ex, errorPositions)
+    case ex => new FetchingDependenciesError(ex, dependencies.flatMap(_.positions))
+  }
 }
