bugfix: go to implementations for multi-module projects (#6211)

scalameta · Mar 14, 2024 · fe430b2 · fe430b2
1 parent b278ef9
commit fe430b2
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 37 deletions.
diff --git a/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala b/metals/src/main/scala/scala/meta/internal/implementation/ImplementationProvider.scala
@@ -15,6 +15,7 @@ import scala.meta.internal.metals.BuildTargets
 import scala.meta.internal.metals.Compilers
 import scala.meta.internal.metals.DefinitionProvider
 import scala.meta.internal.metals.MetalsEnrichments._
+import scala.meta.internal.metals.Report
 import scala.meta.internal.metals.ReportContext
 import scala.meta.internal.metals.ScalaVersionSelector
 import scala.meta.internal.metals.ScalaVersions
@@ -25,9 +26,7 @@ import scala.meta.internal.mtags.Mtags
 import scala.meta.internal.mtags.OverriddenSymbol
 import scala.meta.internal.mtags.ResolvedOverriddenSymbol
 import scala.meta.internal.mtags.Semanticdbs
-import scala.meta.internal.mtags.SymbolDefinition
 import scala.meta.internal.mtags.UnresolvedOverriddenSymbol
-import scala.meta.internal.mtags.{Symbol => MSymbol}
 import scala.meta.internal.parsing.Trees
 import scala.meta.internal.pc.PcSymbolInformation
 import scala.meta.internal.search.SymbolHierarchyOps._
@@ -155,23 +154,42 @@ final class ImplementationProvider(
           )
           .toIterable
     } yield {
-      // 1. Search locally for symbol
-      // 2. Search inside workspace
-      // 3. Search classpath via GlobalSymbolTable
       val sym = symbolOccurrence.symbol
       val dealiased =
         if (sym.desc.isType) {
           symbolInfo(currentDocument, source, sym).map(
             _.map(_.dealiasedSymbol).getOrElse(sym)
           )
         } else Future.successful(sym)
+
       dealiased.flatMap { dealisedSymbol =>
-        val isWorkspaceSymbol =
-          (source.isWorkspaceSource(workspace) &&
-            currentDocument.definesSymbol(dealisedSymbol)) ||
-            findSymbolDefinition(dealisedSymbol).exists(
-              _.path.isWorkspaceSource(workspace)
+        val currentContainsDefinition =
+          currentDocument.definesSymbol(dealisedSymbol)
+        val sourceFiles: Set[AbsolutePath] =
+          if (currentContainsDefinition) Set(source)
+          else
+            definitionProvider
+              .fromSymbol(dealisedSymbol, Some(source))
+              .asScala
+              .map(_.getUri().toAbsolutePath)
+              .toSet
+
+        if (sourceFiles.isEmpty) {
+          rc.unsanitized.create(
+            Report(
+              "missing-definition",
+              s"""|Missing definition symbol for:
+                  |$dealisedSymbol
+                  |""".stripMargin,
+              s"missing def: $dealisedSymbol",
+              Some(source.toURI.toString()),
             )
+          )
+        }
+
+        val isWorkspaceSymbol =
+          (currentContainsDefinition && source.isWorkspaceSource(workspace)) ||
+            sourceFiles.forall(_.isWorkspaceSource(workspace))
 
         val workspaceInheritanceContext: InheritanceContext =
           InheritanceContext.fromDefinitions(
@@ -193,6 +211,7 @@ final class ImplementationProvider(
           dealisedSymbol,
           currentDocument,
           source,
+          sourceFiles,
           inheritanceContext,
         )
       }
@@ -207,6 +226,7 @@ final class ImplementationProvider(
       dealiased: String,
       textDocument: TextDocument,
       source: AbsolutePath,
+      definitionFiles: Set[AbsolutePath],
       inheritanceContext: InheritanceContext,
   ): Future[Seq[Location]] = {
 
@@ -276,13 +296,20 @@ final class ImplementationProvider(
         locationsByFile: Map[Path, Set[ClassLocation]],
         parentSymbol: PcSymbolInformation,
         classSymbol: String,
-        buildTarget: BuildTargetIdentifier,
+        definitionBuildTargets: Set[BuildTargetIdentifier],
     ) = Future.sequence({
+      def allDependencyBuildTargets(implPath: AbsolutePath) = {
+        val targets = buildTargets.inverseSourcesAll(implPath)
+        buildTargets.buildTargetTransitiveDependencies(targets).toSet ++ targets
+      }
+
       for {
         file <- files
         locations = locationsByFile(file)
         implPath = AbsolutePath(file)
-        if (buildTargets.belongsToBuildTarget(buildTarget, implPath))
+        if (definitionBuildTargets.isEmpty || allDependencyBuildTargets(
+          implPath
+        ).exists(definitionBuildTargets(_)))
         implDocument <- findSemanticdb(implPath).toList
       } yield {
         for {
@@ -331,7 +358,6 @@ final class ImplementationProvider(
         (for {
           symbolInfo <- optSymbolInfo
           symbolClass <- classFromSymbol(symbolInfo)
-          target <- buildTargets.inverseSources(source)
         } yield {
           for {
             locationsByFile <- findImplementation(
@@ -350,7 +376,9 @@ final class ImplementationProvider(
                     locationsByFile,
                     symbolInfo,
                     symbolClass,
-                    target,
+                    definitionFiles.flatMap(
+                      buildTargets.inverseSourcesAll(_).toSet
+                    ),
                   )
                 )
               )
@@ -425,10 +453,6 @@ final class ImplementationProvider(
     })
   }
 
-  private def findSymbolDefinition(symbol: String): Option[SymbolDefinition] = {
-    index.definition(MSymbol(symbol))
-  }
-
   private def classFromSymbol(info: PcSymbolInformation): Option[String] =
     if (classLikeKinds(info.kind)) Some(info.dealiasedSymbol)
     else info.classOwner

diff --git a/metals/src/main/scala/scala/meta/internal/implementation/InheritanceContext.scala b/metals/src/main/scala/scala/meta/internal/implementation/InheritanceContext.scala
@@ -51,8 +51,8 @@ class GlobalInheritanceContext(
     val resolveGlobal =
       implementationsInDependencySources
         .getOrElse(shortName, Set.empty)
-        .collect { case loc @ ClassLocation(sym, _) =>
-          compilers.info(source, sym).map {
+        .collect { case loc @ ClassLocation(sym, Some(filePath)) =>
+          compilers.info(AbsolutePath(filePath), sym).map {
             case Some(symInfo) if symInfo.parents.contains(symbol) => Some(loc)
             case Some(symInfo)
                 if symInfo.dealiasedSymbol == symbol && symInfo.symbol != symbol =>

diff --git a/metals/src/main/scala/scala/meta/internal/metals/BuildTargets.scala b/metals/src/main/scala/scala/meta/internal/metals/BuildTargets.scala
@@ -171,10 +171,15 @@ final class BuildTargets private (
 
   def buildTargetTransitiveDependencies(
       id: BuildTargetIdentifier
+  ): Iterable[BuildTargetIdentifier] =
+    buildTargetTransitiveDependencies(List(id))
+
+  def buildTargetTransitiveDependencies(
+      ids: List[BuildTargetIdentifier]
   ): Iterable[BuildTargetIdentifier] = {
     val isVisited = mutable.Set.empty[BuildTargetIdentifier]
     val toVisit = new java.util.ArrayDeque[BuildTargetIdentifier]
-    toVisit.add(id)
+    ids.foreach(toVisit.add(_))
     while (!toVisit.isEmpty) {
       val next = toVisit.pop()
       if (!isVisited(next)) {
@@ -382,15 +387,6 @@ final class BuildTargets private (
     }
   }
 
-  def belongsToBuildTarget(
-      target: BuildTargetIdentifier,
-      path: AbsolutePath,
-  ): Boolean = {
-    val possibleBuildTargets =
-      buildTargetTransitiveDependencies(target).toSet + target
-    inverseSourcesAll(path).exists(possibleBuildTargets(_))
-  }
-
   def inferBuildTarget(
       source: AbsolutePath
   ): Option[BuildTargetIdentifier] =

diff --git a/tests/unit/src/main/scala/tests/BaseRangesSuite.scala b/tests/unit/src/main/scala/tests/BaseRangesSuite.scala
@@ -22,6 +22,7 @@ abstract class BaseRangesSuite(name: String) extends BaseLspSuite(name) {
       scalaVersion: Option[String] = None,
       additionalLibraryDependencies: List[String] = Nil,
       scalacOptions: List[String] = Nil,
+      customMetalsJson: Option[String] = None,
   )(implicit
       loc: Location
   ): Unit = {
@@ -44,19 +45,24 @@ abstract class BaseRangesSuite(name: String) extends BaseLspSuite(name) {
     }
 
     val actualScalaVersion = scalaVersion.getOrElse(BuildInfo.scalaVersion)
+    val metalsJson =
+      customMetalsJson.getOrElse(
+        s"""|{"a":
+            |  {
+            |    "scalaVersion" : "$actualScalaVersion",
+            |    "libraryDependencies": ${toJsonArray(libraryDependencies ++ additionalLibraryDependencies)},
+            |    "scalacOptions": ${toJsonArray(scalacOptions)}
+            |  }
+            |}
+            |""".stripMargin
+      )
 
     test(name) {
       cleanWorkspace()
       for {
         _ <- initialize(
           s"""/metals.json
-             |{"a":
-             |  {
-             |    "scalaVersion" : "$actualScalaVersion",
-             |    "libraryDependencies": ${toJsonArray(libraryDependencies ++ additionalLibraryDependencies)},
-             |    "scalacOptions": ${toJsonArray(scalacOptions)}
-             |  }
-             |}
+             |$metalsJson
              |${input
               .replaceAll("(<<|>>|@@)", "")}""".stripMargin
         )

diff --git a/tests/unit/src/test/scala/tests/ImplementationLspSuite.scala b/tests/unit/src/test/scala/tests/ImplementationLspSuite.scala
@@ -659,6 +659,33 @@ class ImplementationLspSuite extends BaseImplementationSuite("implementation") {
        |""".stripMargin,
   )
 
+  check(
+    "multi-module",
+    """|/a/src/main/scala/com/example/foo/Foo.scala
+       |package com.example.foo
+       |trait F@@oo {
+       |  def transform(input: Int): Int
+       |}
+       |/b/src/main/scala/com/example/bar/Bar.scala
+       |package com.example.bar
+       |
+       |import com.example.foo.Foo
+       |
+       |class <<Bar>> extends Foo {
+       |  override def transform(input: Int): Int = input * 2
+       |}
+       |""".stripMargin,
+    customMetalsJson = Some(
+      """|{
+         |  "a":{ },
+         |  "b":{
+         |    "dependsOn": ["a"]
+         |  }
+         |}
+         |""".stripMargin
+    ),
+  )
+
   override protected def libraryDependencies: List[String] =
     List("org.scalatest::scalatest:3.2.16", "io.circe::circe-generic:0.12.0")