From 1d5c920a26cdbc0f0dfe54fe0cdb05a8e027dbe8 Mon Sep 17 00:00:00 2001 From: kasiaMarek Date: Mon, 16 Dec 2024 13:16:49 +0100 Subject: [PATCH] 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