Skip to content

Commit

Permalink
Prioritize build targets with supported Scala versions.
Browse files Browse the repository at this point in the history
Previously, if two build targets in the repo were eligible for a source
file, Metals would sometimes prioritize a target with an unsupported
Scala version over a target with a supported Scala version. This
resulted in completions/hover/parameterHints not to work while editing
cross-built sources.

This PR introduces a change to the logic where we select a build target
for a given source file. Now we sort the elibible candidates by two
criteria: the Scala version and the platform (JVM/JS/Native). This
change takes the first step towards
scalameta/metals-feature-requests#13, which is
about making this logic configurable.
  • Loading branch information
Olafur Pall Geirsson committed Sep 27, 2019
1 parent 807ca28 commit 468f3ce
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ch.epfl.scala.bsp4j.BuildTarget
import ch.epfl.scala.bsp4j.BuildTargetIdentifier
import ch.epfl.scala.bsp4j.ScalacOptionsItem
import ch.epfl.scala.bsp4j.ScalacOptionsResult
import ch.epfl.scala.bsp4j.ScalaBuildTarget
import ch.epfl.scala.bsp4j.WorkspaceBuildTargetsResult
import java.util.concurrent.ConcurrentLinkedQueue
import scala.annotation.tailrec
Expand Down Expand Up @@ -175,6 +176,10 @@ final class BuildTargets() {
buildTarget: BuildTargetIdentifier
): Option[BuildTarget] =
buildTargetInfo.get(buildTarget)
def scalaInfo(
buildTarget: BuildTargetIdentifier
): Option[ScalaBuildTarget] =
info(buildTarget).flatMap(_.asScalaBuildTarget)

def scalacOptions(
buildTarget: BuildTargetIdentifier
Expand All @@ -188,11 +193,25 @@ final class BuildTargets() {
source: AbsolutePath
): Option[BuildTargetIdentifier] = {
val buildTargets = sourceBuildTargets(source)
buildTargets // prioritize JVM targets over JS/Native
.find(x => scalacOptions(x).exists(_.isJVM))
.orElse(buildTargets.headOption)
.orElse(tables.flatMap(_.dependencySources.getBuildTarget(source)))
.orElse(inferBuildTarget(source))
if (buildTargets.isEmpty) {
tables
.flatMap(_.dependencySources.getBuildTarget(source))
.orElse(inferBuildTarget(source))
} else {
Some(buildTargets.maxBy { t =>
var score = 1

val isSupportedScalaVersion = scalaInfo(t).exists(
t => ScalaVersions.isSupportedScalaVersion(t.getScalaVersion())
)
if (isSupportedScalaVersion) score <<= 2

val isJVM = scalacOptions(t).exists(_.isJVM)
if (isJVM) score <<= 1

score
})
}
}

/**
Expand Down
12 changes: 9 additions & 3 deletions tests/unit/src/main/scala/tests/QuickBuild.scala
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import java.nio.file.Paths
import java.security.MessageDigest
import scala.meta.internal.io.FileIO
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.metals.ScalaVersions
import scala.meta.internal.metals.Time
import scala.meta.internal.metals.Timer
import scala.meta.internal.metals.{BuildInfo => V}
Expand Down Expand Up @@ -89,12 +90,14 @@ case class QuickBuild(
.resolve(s"scala-$binaryVersion")
.resolve(s"${testPrefix}classes")
}
val sources = (List(
val extraSources =
additionalSources.map(relpath => workspace.resolve(relpath).toNIO).toList
val sources = extraSources ::: List(
"src/main/java",
"src/main/scala",
s"src/main/scala-$binaryVersion",
s"src/main/scala-$binaryVersion"
) ++ additionalSources).map(relpath => baseDirectory.resolve(relpath))
).map(relpath => baseDirectory.resolve(relpath))
val allDependencies = Array(
s"org.scala-lang:scala-library:$scalaVersion",
s"org.scala-lang:scala-reflect:$scalaVersion"
Expand All @@ -107,7 +110,10 @@ case class QuickBuild(
)
val (dependencySources, classpath) =
allJars.partition(_.getFileName.toString.endsWith("-sources.jar"))
val allPlugins = s"org.scalameta:::semanticdb-scalac:${V.semanticdbVersion}" :: compilerPlugins.toList
val allPlugins =
if (ScalaVersions.isSupportedScalaVersion(scalaVersion))
s"org.scalameta:::semanticdb-scalac:${V.semanticdbVersion}" :: compilerPlugins.toList
else compilerPlugins.toList
val pluginDependencies = allPlugins.map(
plugin =>
QuickBuild
Expand Down
36 changes: 36 additions & 0 deletions tests/unit/src/test/scala/tests/BuildTargetsSlowSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package tests

object BuildTargetsSlowSuite
extends BaseSlowSuite("build-targets")
with TestHovers {

testAsync("scala-priority") {
for {
_ <- server.initialize(
s"""/metals.json
|{
| "a": {
| "scalaVersion": "2.10.6",
| "libraryDependencies": ["com.lihaoyi::sourcecode:0.1.7"],
| "additionalSources": [ "shared/Main.scala" ]
| },
| "b": {
| "scalaVersion": "${BuildInfo.scalaVersion}",
| "libraryDependencies": ["com.lihaoyi::sourcecode:0.1.7"],
| "additionalSources": [ "shared/Main.scala" ]
| }
|}
""".stripMargin
)
// Assert that a supported Scala version target is picked over 2.10.
_ <- server.assertHover(
"shared/Main.scala",
"""
|object Main {
| sourcecode.Line(1).val@@ue
|}""".stripMargin,
"""val value: Int""".hover
)
} yield ()
}
}
2 changes: 1 addition & 1 deletion tests/unit/src/test/scala/tests/DiagnosticsSlowSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ object DiagnosticsSlowSuite extends BaseSlowSuite("diagnostics") {
|/metals.json
|{
| "a": {
| "additionalSources" : [ "weird/path/A.scala" ]
| "additionalSources" : [ "a/weird/path/A.scala" ]
| }
|}
|/a/weird/path/A.scala
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/src/test/scala/tests/FileWatcherSlowSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ object FileWatcherSlowSuite extends BaseSlowSuite("file-watcher") {
"""
|/metals.json
|{
| "a": { "additionalSources" : ["weird/path/d/D.scala", "non/existent/one/e/E.scala"] },
| "a": { "additionalSources" : ["a/weird/path/d/D.scala", "non/existent/one/e/E.scala"] },
| "b": { },
| "c": { "dependsOn": [ "a" ] }
|}
Expand Down

0 comments on commit 468f3ce

Please sign in to comment.