From 35a7b018158df29669db0772081749244239640b Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Fri, 13 Dec 2024 13:50:45 +0100 Subject: [PATCH 1/7] improvement: use pc for go to def when stale semanticdb --- .../scala/scala/meta/internal/metals/DefinitionProvider.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index fc34c7975a4..00b09268382 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -84,7 +84,7 @@ final class DefinitionProvider( token: CancelToken, ): Future[DefinitionResult] = { val fromSemanticdb = - semanticdbs().textDocument(path).documentIncludingStale + semanticdbs().textDocument(path).toOption val fromSnapshot = fromSemanticdb match { case Some(doc) => definitionFromSnapshot(path, params, doc) From 1d5c920a26cdbc0f0dfe54fe0cdb05a8e027dbe8 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Mon, 16 Dec 2024 13:16:49 +0100 Subject: [PATCH 2/7] improvement: reorder go to definition handlers (start with pc, then semanticdb), add report when missing definition) --- .../internal/metals/DefinitionProvider.scala | 157 +++++++--- .../metals/FallbackDefinitionProvider.scala | 271 +++++++++--------- 2 files changed, 259 insertions(+), 169 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index 00b09268382..5950a686ea2 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -4,9 +4,11 @@ import java.{util => ju} import scala.concurrent.ExecutionContext import scala.concurrent.Future +import scala.util.Try import scala.meta.inputs.Input import scala.meta.inputs.Position.Range +import scala.meta.internal.metals.LoggerReportContext.unsanitized import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.mtags.GlobalSymbolIndex import scala.meta.internal.mtags.Mtags @@ -82,46 +84,45 @@ final class DefinitionProvider( path: AbsolutePath, params: TextDocumentPositionParams, token: CancelToken, - ): Future[DefinitionResult] = { - val fromSemanticdb = - semanticdbs().textDocument(path).toOption - val fromSnapshot = fromSemanticdb match { - case Some(doc) => - definitionFromSnapshot(path, params, doc) - case _ => - DefinitionResult.empty - } - val fromCompilerOrSemanticdb = - fromSnapshot match { - case defn if defn.isEmpty && path.isScalaFilename => - compilers().definition(params, token) - case defn @ DefinitionResult(_, symbol, _, _, querySymbol) - if symbol != querySymbol && path.isScalaFilename => - compilers().definition(params, token).map { compilerDefn => - if (compilerDefn.isEmpty || compilerDefn.querySymbol == querySymbol) - defn - else compilerDefn.copy(semanticdb = defn.semanticdb) - } - case defn => - if (fromSemanticdb.isEmpty) { - warnings().noSemanticdb(path) - } - Future.successful(defn) + ): Future[DefinitionResult] = + for { + fromCompiler <- + if (path.isScalaFilename) compilers().definition(params, token) + else Future.successful(DefinitionResult.empty) + } yield { + if (!fromCompiler.isEmpty) fromCompiler + else { + val reportBuilder = + new DefinitionProviderReportBuilder(path, params, fromCompiler) + val fromSemanticDB = semanticdbs() + .textDocument(path) + .documentIncludingStale + .map(definitionFromSnapshot(path, params, _)) + fromSemanticDB.foreach(reportBuilder.withSemanticDBResult(_)) + val result = fromSemanticDB match { + case Some(definition) + if !definition.isEmpty || definition.symbol.endsWith("/") => + definition + case _ => + val isScala3 = + ScalaVersions.isScala3Version( + scalaVersionSelector.scalaVersionForPath(path) + ) + + val fromScalaDoc = + scaladocDefinitionProvider.definition(path, params, isScala3) + fromScalaDoc.foreach(_ => reportBuilder.withFoundScaladocDef()) + fromScalaDoc + .orElse( + fallback + .search(path, params.getPosition(), isScala3, reportBuilder) + ) + .getOrElse(fromCompiler) + } + reportBuilder.build().foreach(unsanitized.create(_)) + result } - - fromCompilerOrSemanticdb.map { definition => - if (definition.isEmpty && !definition.symbol.endsWith("/")) { - val isScala3 = - ScalaVersions.isScala3Version( - scalaVersionSelector.scalaVersionForPath(path) - ) - scaladocDefinitionProvider - .definition(path, params, isScala3) - .orElse(fallback.search(path, params.getPosition(), isScala3)) - .getOrElse(definition) - } else definition } - } def definition( path: AbsolutePath, @@ -508,3 +509,83 @@ class DestinationProvider( } } } + +class DefinitionProviderReportBuilder( + path: AbsolutePath, + params: TextDocumentPositionParams, + compilerDefn: DefinitionResult, +) { + private var semanticDBDefn: Option[DefinitionResult] = None + + private var fallbackDefn: Option[DefinitionResult] = None + private var nonLocalGuesses: List[String] = List.empty + + private var fundScaladocDef = false + + private var error: Option[Throwable] = None + + def withSemanticDBResult(result: DefinitionResult): this.type = { + semanticDBDefn = Some(result) + this + } + + def withFallbackResult(result: DefinitionResult): this.type = { + fallbackDefn = Some(result) + this + } + + def withError(e: Throwable): this.type = { + error = Some(e) + this + } + + def withNonLocalGuesses(guesses: List[String]): this.type = { + nonLocalGuesses = guesses + this + } + + def withFoundScaladocDef(): this.type = { + fundScaladocDef = true + this + } + + def build(): Option[Report] = + Option.when(!fundScaladocDef) { + Report( + "empty-definition", + s"""|empty definition, found symbol in pc: ${compilerDefn.querySymbol} + |${semanticDBDefn match { + case None => + s"empty definition using semnticdb (not found) " + case Some(defn) if defn.isEmpty => + s"empty definition using semnticdb" + case Some(defn) => + s"found definition using semanticdb; symbol ${defn.symbol}" + }} + |${} + |${fallbackDefn.map { + case defn if defn.isEmpty => + s"""|empty definition using fallback + |non-local guesses: + |${nonLocalGuesses.mkString("\t -", "\n\t -", "")} + |""" + case defn => + s"found definition using fallback; symbol ${defn.symbol}" + }} + |Document text: + | + |```scala + |${Try(path.readText).toOption.getOrElse("Failed to read text")} + |``` + |""".stripMargin, + s"empty definition, found symbol in pc: ${compilerDefn.querySymbol}", + path = Some(path.toURI), + id = Some( + if (compilerDefn.querySymbol != "") compilerDefn.querySymbol + else + s"${path.toURI}:${params.getPosition().getLine()}:${params.getPosition().getCharacter()}" + ), + error = error, + ) + } +} diff --git a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala index c4d660bcc59..ac6865885f2 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala @@ -32,144 +32,153 @@ class FallbackDefinitionProvider( path: AbsolutePath, pos: Position, isScala3: Boolean, - ): Option[DefinitionResult] = { - val range = new LspRange(pos, pos) - - val defResult = for { - tokens <- trees.tokenized(path) - ident <- tokens.collectFirst { - case id: Token.Ident if id.pos.encloses(range) => id - } - tree <- trees.get(path) - } yield { - lazy val nameTree = trees.findLastEnclosingAt(path, pos) - - // for sure is not a class/trait/enum if we access it via select - lazy val isInSelectPosition = - nameTree.flatMap(_.parent).exists(isInSelect(_, range)) - - lazy val isInTypePosition = nameTree.exists(_.is[Type.Name]) - - def guessObjectOrClass(parts: List[String]) = { - val symbolPrefix = mtags.Symbol - .guessSymbolFromParts(parts, isScala3) - .value - if (isInSelectPosition) List(symbolPrefix + ".") - else if (isInTypePosition) List(symbolPrefix + "#") - else List(".", "#", "().").map(ending => symbolPrefix + ending) - } - - // Get all select parts to build symbol from it later - val proposedNameParts = - nameTree - .flatMap(_.parent) - .map { - case tree: Term.Select if nameTree.contains(tree.name) => - nameFromSelect(tree, Nil) - case _ => List(ident.value) - } - .getOrElse(List(ident.value)) - - val currentPackageStatements = trees - .packageStatementsAtPosition(path, pos) match { - case None => List("_empty_") - case Some(value) => - // generate packages from all the package statements - value.foldLeft(Seq.empty[String]) { case (pre, suffix) => - if (pre.isEmpty) List(suffix) else pre :+ (pre.last + "." + suffix) - } - } - - val proposedCurrentPackageSymbols = - currentPackageStatements.flatMap(pkg => - guessObjectOrClass( - (pkg.split("\\.").toList ++ proposedNameParts) + reportBuilder: DefinitionProviderReportBuilder, + ): Option[DefinitionResult] = + try { + val range = new LspRange(pos, pos) + + val defResult = for { + tokens <- trees.tokenized(path) + ident <- tokens.collectFirst { + case id: Token.Ident if id.pos.encloses(range) => id + } + tree <- trees.get(path) + } yield { + lazy val nameTree = trees.findLastEnclosingAt(path, pos) + + // for sure is not a class/trait/enum if we access it via select + lazy val isInSelectPosition = + nameTree.flatMap(_.parent).exists(isInSelect(_, range)) + + lazy val isInTypePosition = nameTree.exists(_.is[Type.Name]) + + def guessObjectOrClass(parts: List[String]) = { + val symbolPrefix = mtags.Symbol + .guessSymbolFromParts(parts, isScala3) + .value + if (isInSelectPosition) List(symbolPrefix + ".") + else if (isInTypePosition) List(symbolPrefix + "#") + else List(".", "#", "().").map(ending => symbolPrefix + ending) + } + + // Get all select parts to build symbol from it later + val proposedNameParts = + nameTree + .flatMap(_.parent) + .map { + case tree: Term.Select if nameTree.contains(tree.name) => + nameFromSelect(tree, Nil) + case _ => List(ident.value) + } + .getOrElse(List(ident.value)) + + val currentPackageStatements = trees + .packageStatementsAtPosition(path, pos) match { + case None => List("_empty_") + case Some(value) => + // generate packages from all the package statements + value.foldLeft(Seq.empty[String]) { case (pre, suffix) => + if (pre.isEmpty) List(suffix) + else pre :+ (pre.last + "." + suffix) + } + } + + val proposedCurrentPackageSymbols = + currentPackageStatements.flatMap(pkg => + guessObjectOrClass( + (pkg.split("\\.").toList ++ proposedNameParts) + ) ) - ) - - // First name in select is the one that must be imported or in scope - val probablyImported = proposedNameParts.headOption.getOrElse(ident.value) - - // Search for imports that match the current symbol - val proposedImportedSymbols = - tree.collect { - case imp @ Import(importers) - // imports should be in the same scope as the current position - if imp.parent.exists(_.pos.encloses(range)) => - importers.collect { case Importer(ref: Term, p) => - val packageSyntax = ref.toString.split("\\.").toList - p.collect { - case Importee.Name(name) if name.value == probablyImported => - guessObjectOrClass(packageSyntax ++ proposedNameParts) - - case Importee.Rename(name, renamed) - if renamed.value == probablyImported => - guessObjectOrClass( - packageSyntax ++ (name.value +: proposedNameParts.drop(1)) - ) - case _: Importee.Wildcard => - guessObjectOrClass(packageSyntax ++ proposedNameParts) + // First name in select is the one that must be imported or in scope + val probablyImported = + proposedNameParts.headOption.getOrElse(ident.value) + + // Search for imports that match the current symbol + val proposedImportedSymbols = + tree.collect { + case imp @ Import(importers) + // imports should be in the same scope as the current position + if imp.parent.exists(_.pos.encloses(range)) => + importers.collect { case Importer(ref: Term, p) => + val packageSyntax = ref.toString.split("\\.").toList + p.collect { + case Importee.Name(name) if name.value == probablyImported => + guessObjectOrClass(packageSyntax ++ proposedNameParts) + + case Importee.Rename(name, renamed) + if renamed.value == probablyImported => + guessObjectOrClass( + packageSyntax ++ (name.value +: proposedNameParts.drop(1)) + ) + case _: Importee.Wildcard => + guessObjectOrClass(packageSyntax ++ proposedNameParts) + + }.flatten }.flatten - }.flatten - }.flatten - - val standardScalaImports = guessObjectOrClass( - List("scala", "Predef") ++ proposedNameParts - ) - val fullyScopedName = - guessObjectOrClass(proposedNameParts) - - def findInIndex(proposedSymbol: String) = { - index - .definition(mtags.Symbol(proposedSymbol)) - // Make sure we don't return unrelated definitions - .filter { _.definitionSymbol.value == proposedSymbol } - } - val nonLocalGuesses = - (proposedImportedSymbols ++ fullyScopedName ++ standardScalaImports).distinct - .flatMap { proposedSymbol => - findInIndex(proposedSymbol) - } - - def toDefinition(guesses: List[mtags.SymbolDefinition]) = { - - DefinitionResult( - guesses - .flatMap(guess => - guess.range.map(range => - new Location(guess.path.toURI.toString(), range.toLsp) - ) - ) - .asJava, - ident.value, - None, - None, - ident.value, - ) - } - val result = if (nonLocalGuesses.nonEmpty) { - Some(toDefinition(nonLocalGuesses)) - } else { - // otherwise might be symbol in a local package, starting from enclosing - proposedCurrentPackageSymbols.reverse - .map(proposedSymbol => findInIndex(proposedSymbol)) - .collectFirst { case Some(dfn) => - toDefinition(List(dfn)) - } - } + }.flatten - result.foreach { _ => - scribe.warn( - s"Could not find '${ident.value}' using presentation compiler nor semanticdb. " + - s"Trying to guess the definition using available information from local class context. " + val standardScalaImports = guessObjectOrClass( + List("scala", "Predef") ++ proposedNameParts ) + val fullyScopedName = + guessObjectOrClass(proposedNameParts) + + def findInIndex(proposedSymbol: String) = { + index + .definition(mtags.Symbol(proposedSymbol)) + // Make sure we don't return unrelated definitions + .filter { _.definitionSymbol.value == proposedSymbol } + } + val nonLocalCandidates = + (proposedImportedSymbols ++ fullyScopedName ++ standardScalaImports).distinct + + reportBuilder.withNonLocalGuesses(nonLocalCandidates) + + val nonLocalGuesses = nonLocalCandidates.flatMap(findInIndex) + + def toDefinition(guesses: List[mtags.SymbolDefinition]) = { + + DefinitionResult( + guesses + .flatMap(guess => + guess.range.map(range => + new Location(guess.path.toURI.toString(), range.toLsp) + ) + ) + .asJava, + ident.value, + None, + None, + ident.value, + ) + } + val result = if (nonLocalGuesses.nonEmpty) { + Some(toDefinition(nonLocalGuesses)) + } else { + // otherwise might be symbol in a local package, starting from enclosing + proposedCurrentPackageSymbols.reverse + .map(proposedSymbol => findInIndex(proposedSymbol)) + .collectFirst { case Some(dfn) => + toDefinition(List(dfn)) + } + } + + result.foreach { _ => + scribe.warn( + s"Could not find '${ident.value}' using presentation compiler nor semanticdb. " + + s"Trying to guess the definition using available information from local class context. " + ) + } + result } - result - } - defResult.flatten - } + defResult.flatten + } catch { + case e: Exception => + reportBuilder.withError(e) + None + } private def isInSelect(tree: Tree, range: LspRange): Boolean = tree match { case Type.Select(qual, _) if qual.pos.encloses(range) => true From b185bc3dcab13ee06b8daf4bacd639d3ab517c52 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Mon, 16 Dec 2024 17:47:10 +0100 Subject: [PATCH 3/7] fix: rename tests --- .../internal/metals/DefinitionProvider.scala | 18 ++++++++---------- .../internal/pc/PcDefinitionProvider.scala | 9 ++++++--- .../test/scala/tests/DefinitionLspSuite.scala | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index 5950a686ea2..0316541fe73 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -89,15 +89,14 @@ final class DefinitionProvider( fromCompiler <- if (path.isScalaFilename) compilers().definition(params, token) else Future.successful(DefinitionResult.empty) + semanticdb = semanticdbs().textDocument(path).documentIncludingStale } yield { - if (!fromCompiler.isEmpty) fromCompiler + if (!fromCompiler.isEmpty) fromCompiler.copy(semanticdb = semanticdb) else { val reportBuilder = new DefinitionProviderReportBuilder(path, params, fromCompiler) - val fromSemanticDB = semanticdbs() - .textDocument(path) - .documentIncludingStale - .map(definitionFromSnapshot(path, params, _)) + val fromSemanticDB = + semanticdb.map(definitionFromSnapshot(path, params, _)) fromSemanticDB.foreach(reportBuilder.withSemanticDBResult(_)) val result = fromSemanticDB match { case Some(definition) @@ -553,16 +552,15 @@ class DefinitionProviderReportBuilder( Option.when(!fundScaladocDef) { Report( "empty-definition", - s"""|empty definition, found symbol in pc: ${compilerDefn.querySymbol} + s"""|empty definition using pc, found symbol in pc: ${compilerDefn.querySymbol} |${semanticDBDefn match { case None => - s"empty definition using semnticdb (not found) " + s"semanticdb not found" case Some(defn) if defn.isEmpty => - s"empty definition using semnticdb" + s"empty definition using semanticdb" case Some(defn) => s"found definition using semanticdb; symbol ${defn.symbol}" }} - |${} |${fallbackDefn.map { case defn if defn.isEmpty => s"""|empty definition using fallback @@ -578,7 +576,7 @@ class DefinitionProviderReportBuilder( |${Try(path.readText).toOption.getOrElse("Failed to read text")} |``` |""".stripMargin, - s"empty definition, found symbol in pc: ${compilerDefn.querySymbol}", + s"empty definition using pc, found symbol in pc: ${compilerDefn.querySymbol}", path = Some(path.toURI), id = Some( if (compilerDefn.querySymbol != "") compilerDefn.querySymbol diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/PcDefinitionProvider.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/PcDefinitionProvider.scala index 0b49c51a36e..6b92bbc6212 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/PcDefinitionProvider.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/PcDefinitionProvider.scala @@ -87,13 +87,16 @@ class PcDefinitionProvider(val compiler: MetalsGlobal, params: OffsetParams) { symbol.pos.source.eq(unit.source) ) { val focused = symbol.pos.focus + val actualName = symbol.decodedName.stripSuffix("_=").trim val namePos = - if (symbol.name.startsWith("x$") && symbol.isSynthetic) focused.toLsp - else focused.withEnd(focused.start + symbol.name.length()).toLsp + if (symbol.name.startsWith("x$") && symbol.isSynthetic) focused + else focused.withEnd(focused.start + actualName.length()) + val adjusted = namePos.adjust(unit.source.content)._1 + DefinitionResultImpl( semanticdbSymbol(symbol), ju.Collections.singletonList( - new Location(params.uri().toString(), namePos) + new Location(params.uri().toString(), adjusted.toLsp) ) ) } else { diff --git a/tests/unit/src/test/scala/tests/DefinitionLspSuite.scala b/tests/unit/src/test/scala/tests/DefinitionLspSuite.scala index 074ae5f2f9a..b4e30c1f7f3 100644 --- a/tests/unit/src/test/scala/tests/DefinitionLspSuite.scala +++ b/tests/unit/src/test/scala/tests/DefinitionLspSuite.scala @@ -233,7 +233,7 @@ class DefinitionLspSuite |package a |object B/*L1*/ { | def main/*L2*/() = { - | println/*Predef.scala*/(A/*;A.scala:2;A.scala:3*/("John")) + | println/*Predef.scala*/(A/*A.scala:2*/("John")) | A/*A.scala:3*/.fun/*A.scala:5*/() | } |} From 5a2a4dd3bb1a6d2047cdb665617aaa7e6f6a3d39 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 18 Dec 2024 14:04:02 +0100 Subject: [PATCH 4/7] don't use pc for definition for Ammonite --- .../scala/meta/internal/metals/DefinitionProvider.scala | 5 ++++- .../main/scala/scala/meta/internal/metals/TargetData.scala | 2 +- .../scala/scala/meta/internal/metals/ammonite/Ammonite.scala | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index 0316541fe73..bc93cc91520 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -80,6 +80,9 @@ final class DefinitionProvider( val scaladocDefinitionProvider = new ScaladocDefinitionProvider(buffers, trees, destinationProvider) + private def isAmmonnite(path: AbsolutePath): Boolean = + path.isAmmoniteScript && buildTargets.inverseSources(path).flatMap(buildTargets.targetData).exists(_.isAmmonite) + def definition( path: AbsolutePath, params: TextDocumentPositionParams, @@ -87,7 +90,7 @@ final class DefinitionProvider( ): Future[DefinitionResult] = for { fromCompiler <- - if (path.isScalaFilename) compilers().definition(params, token) + if (path.isScalaFilename && !isAmmonnite(path)) compilers().definition(params, token) else Future.successful(DefinitionResult.empty) semanticdb = semanticdbs().textDocument(path).documentIncludingStale } yield { diff --git a/metals/src/main/scala/scala/meta/internal/metals/TargetData.scala b/metals/src/main/scala/scala/meta/internal/metals/TargetData.scala index bb868f60409..eaa4aace70b 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/TargetData.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/TargetData.scala @@ -32,7 +32,7 @@ import ch.epfl.scala.bsp4j.SourceItemKind.FILE import ch.epfl.scala.bsp4j.WorkspaceBuildTargetsResult import org.eclipse.{lsp4j => l} -final class TargetData { +final class TargetData(val isAmmonite: Boolean = false) { val sourceItemsToBuildTarget : MMap[AbsolutePath, ConcurrentLinkedQueue[BuildTargetIdentifier]] = diff --git a/metals/src/main/scala/scala/meta/internal/metals/ammonite/Ammonite.scala b/metals/src/main/scala/scala/meta/internal/metals/ammonite/Ammonite.scala index fe53e2d40d4..a3506a26080 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ammonite/Ammonite.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ammonite/Ammonite.scala @@ -55,7 +55,7 @@ final class Ammonite( )(implicit ec: ExecutionContextExecutorService) extends Cancelable { - val buildTargetsData = new TargetData + val buildTargetsData = new TargetData(isAmmonite = true) def buildServer: Option[BuildServerConnection] = buildServer0 From 3b37f023176fbf58e90d0eb29ade0e716ff6115d Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 18 Dec 2024 15:06:36 +0100 Subject: [PATCH 5/7] fix: use tokens to find correct range for rename --- .../internal/metals/DefinitionProvider.scala | 9 +++++---- .../meta/internal/rename/RenameProvider.scala | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index bc93cc91520..e8c01162996 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -92,14 +92,15 @@ final class DefinitionProvider( fromCompiler <- if (path.isScalaFilename && !isAmmonnite(path)) compilers().definition(params, token) else Future.successful(DefinitionResult.empty) - semanticdb = semanticdbs().textDocument(path).documentIncludingStale } yield { - if (!fromCompiler.isEmpty) fromCompiler.copy(semanticdb = semanticdb) - else { + if (!fromCompiler.isEmpty) { + val pathToDef = fromCompiler.locations.asScala.head.getUri.toAbsolutePath + fromCompiler.copy(semanticdb = semanticdbs().textDocument(pathToDef).documentIncludingStale) + } else { val reportBuilder = new DefinitionProviderReportBuilder(path, params, fromCompiler) val fromSemanticDB = - semanticdb.map(definitionFromSnapshot(path, params, _)) + semanticdbs().textDocument(path).documentIncludingStale.map(definitionFromSnapshot(path, params, _)) fromSemanticDB.foreach(reportBuilder.withSemanticDBResult(_)) val result = fromSemanticDB match { case Some(definition) diff --git a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala index da898edd00a..8302eac5445 100644 --- a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala @@ -7,6 +7,7 @@ import scala.concurrent.Future import scala.meta.Importee import scala.meta.Tree +import scala.meta.inputs.Input import scala.meta.internal.async.ConcurrentQueue import scala.meta.internal.implementation.ImplementationProvider import scala.meta.internal.metals.AdjustRange @@ -16,6 +17,7 @@ import scala.meta.internal.metals.Compilations import scala.meta.internal.metals.Compilers import scala.meta.internal.metals.DefinitionProvider import scala.meta.internal.metals.MetalsEnrichments._ +import scala.meta.internal.metals.PositionSyntax.XtensionPositionsScalafix import scala.meta.internal.metals.ReferenceProvider import scala.meta.internal.metals.ReportContext import scala.meta.internal.metals.TextEdits @@ -32,6 +34,7 @@ import scala.meta.internal.semanticdb.XtensionSemanticdbSymbolInformation import scala.meta.internal.{semanticdb => s} import scala.meta.io.AbsolutePath import scala.meta.pc.CancelToken +import scala.meta.tokens.Token.Ident import org.eclipse.lsp4j.Location import org.eclipse.lsp4j.MessageParams @@ -234,6 +237,7 @@ final class RenameProvider( if (parentSymbols.isEmpty) definition.locations.asScala .filter(_.getUri().isScalaOrJavaFilename) + .map(findDefinitionRage) else parentSymbols } companionRefs = companionReferences( @@ -331,6 +335,18 @@ final class RenameProvider( } } + private def findDefinitionRage(location: Location): Location = { + val adjustedPosition = for { + source <- location.getUri().toAbsolutePathSafe + tree <- trees.get(source) + pos <- location.getRange().getStart().toMeta(Input.VirtualFile(source.toString(), tree.text)) + token <- tree.tokens.collectFirst { + case token: Ident if token.pos.contains(pos) => token + } + } yield token.pos.toLsp + adjustedPosition.map(new Location(location.getUri(), _)).getOrElse(location) + } + def runSave(): Future[Unit] = { val all = synchronized { ConcurrentQueue.pollAll(awaitingSave) From c40bbdaa81db002dc8ffc13f39c5c763f23ea805 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 18 Dec 2024 15:10:51 +0100 Subject: [PATCH 6/7] format --- .../internal/metals/DefinitionProvider.scala | 20 ++++++++++++++----- .../meta/internal/rename/RenameProvider.scala | 7 +++++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index e8c01162996..4d28927c544 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -81,7 +81,10 @@ final class DefinitionProvider( new ScaladocDefinitionProvider(buffers, trees, destinationProvider) private def isAmmonnite(path: AbsolutePath): Boolean = - path.isAmmoniteScript && buildTargets.inverseSources(path).flatMap(buildTargets.targetData).exists(_.isAmmonite) + path.isAmmoniteScript && buildTargets + .inverseSources(path) + .flatMap(buildTargets.targetData) + .exists(_.isAmmonite) def definition( path: AbsolutePath, @@ -90,17 +93,24 @@ final class DefinitionProvider( ): Future[DefinitionResult] = for { fromCompiler <- - if (path.isScalaFilename && !isAmmonnite(path)) compilers().definition(params, token) + if (path.isScalaFilename && !isAmmonnite(path)) + compilers().definition(params, token) else Future.successful(DefinitionResult.empty) } yield { if (!fromCompiler.isEmpty) { - val pathToDef = fromCompiler.locations.asScala.head.getUri.toAbsolutePath - fromCompiler.copy(semanticdb = semanticdbs().textDocument(pathToDef).documentIncludingStale) + val pathToDef = + fromCompiler.locations.asScala.head.getUri.toAbsolutePath + fromCompiler.copy(semanticdb = + semanticdbs().textDocument(pathToDef).documentIncludingStale + ) } else { val reportBuilder = new DefinitionProviderReportBuilder(path, params, fromCompiler) val fromSemanticDB = - semanticdbs().textDocument(path).documentIncludingStale.map(definitionFromSnapshot(path, params, _)) + semanticdbs() + .textDocument(path) + .documentIncludingStale + .map(definitionFromSnapshot(path, params, _)) fromSemanticDB.foreach(reportBuilder.withSemanticDBResult(_)) val result = fromSemanticDB match { case Some(definition) diff --git a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala index 8302eac5445..ac85c165d4c 100644 --- a/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/rename/RenameProvider.scala @@ -336,10 +336,13 @@ final class RenameProvider( } private def findDefinitionRage(location: Location): Location = { - val adjustedPosition = for { + val adjustedPosition = for { source <- location.getUri().toAbsolutePathSafe tree <- trees.get(source) - pos <- location.getRange().getStart().toMeta(Input.VirtualFile(source.toString(), tree.text)) + pos <- location + .getRange() + .getStart() + .toMeta(Input.VirtualFile(source.toString(), tree.text)) token <- tree.tokens.collectFirst { case token: Ident if token.pos.contains(pos) => token } From 73fde6a438c0a7868ca6f12800813f35e62aabf6 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Wed, 18 Dec 2024 17:07:37 +0100 Subject: [PATCH 7/7] small fixes --- .../internal/metals/DefinitionProvider.scala | 19 ++++++++++++------- .../metals/FallbackDefinitionProvider.scala | 4 +++- .../internal/metals/MetalsLspService.scala | 1 - 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala index 4d28927c544..399696c03c8 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/DefinitionProvider.scala @@ -8,7 +8,6 @@ import scala.util.Try import scala.meta.inputs.Input import scala.meta.inputs.Position.Range -import scala.meta.internal.metals.LoggerReportContext.unsanitized import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.mtags.GlobalSymbolIndex import scala.meta.internal.mtags.Mtags @@ -61,7 +60,6 @@ final class DefinitionProvider( scalaVersionSelector: ScalaVersionSelector, saveDefFileToDisk: Boolean, sourceMapper: SourceMapper, - warnings: () => Warnings, )(implicit ec: ExecutionContext, rc: ReportContext) { private val fallback = new FallbackDefinitionProvider(trees, index) @@ -132,7 +130,7 @@ final class DefinitionProvider( ) .getOrElse(fromCompiler) } - reportBuilder.build().foreach(unsanitized.create(_)) + reportBuilder.build().foreach(rc.unsanitized.create(_)) result } } @@ -592,12 +590,19 @@ class DefinitionProviderReportBuilder( |""".stripMargin, s"empty definition using pc, found symbol in pc: ${compilerDefn.querySymbol}", path = Some(path.toURI), - id = Some( - if (compilerDefn.querySymbol != "") compilerDefn.querySymbol - else - s"${path.toURI}:${params.getPosition().getLine()}:${params.getPosition().getCharacter()}" + id = querySymbol.orElse( + Some(s"${path.toURI}:${params.getPosition().getLine()}") ), error = error, ) } + + private def querySymbol: Option[String] = + compilerDefn.querySymbol match { + case "" => + semanticDBDefn + .map(_.querySymbol) + .orElse(fallbackDefn.map(_.querySymbol)) + case q => Some(q) + } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala index ac6865885f2..ed08dabee00 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/FallbackDefinitionProvider.scala @@ -1,5 +1,7 @@ package scala.meta.internal.metals +import scala.util.control.NonFatal + import scala.meta.Term import scala.meta.Type import scala.meta._ @@ -175,7 +177,7 @@ class FallbackDefinitionProvider( defResult.flatten } catch { - case e: Exception => + case NonFatal(e) => reportBuilder.withError(e) None } diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index 2c2502fdabe..62c0bf137ba 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -304,7 +304,6 @@ abstract class MetalsLspService( scalaVersionSelector, saveDefFileToDisk = !clientConfig.isVirtualDocumentSupported(), sourceMapper, - () => warnings, ) val stacktraceAnalyzer: StacktraceAnalyzer = new StacktraceAnalyzer(