-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improvement: use pc for go to def when stale semanticdb #7028
Open
kasiaMarek
wants to merge
7
commits into
scalameta:main
Choose a base branch
from
kasiaMarek:i7027
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
35a7b01
improvement: use pc for go to def when stale semanticdb
kasiaMarek 1d5c920
improvement: reorder go to definition handlers (start with pc, then s…
kasiaMarek b185bc3
fix: rename tests
kasiaMarek 5a2a4dd
don't use pc for definition for Ammonite
kasiaMarek 3b37f02
fix: use tokens to find correct range for rename
kasiaMarek c40bbda
format
kasiaMarek 73fde6a
small fixes
kasiaMarek File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,6 +4,7 @@ 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 | ||||||
|
@@ -59,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) | ||||||
|
@@ -78,50 +78,62 @@ 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, | ||||||
token: CancelToken, | ||||||
): Future[DefinitionResult] = { | ||||||
val fromSemanticdb = | ||||||
semanticdbs().textDocument(path).documentIncludingStale | ||||||
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 => | ||||||
): Future[DefinitionResult] = | ||||||
for { | ||||||
fromCompiler <- | ||||||
if (path.isScalaFilename && !isAmmonnite(path)) | ||||||
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) | ||||||
else Future.successful(DefinitionResult.empty) | ||||||
} yield { | ||||||
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 = | ||||||
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(rc.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 +520,89 @@ 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
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 using pc, found symbol in pc: ${compilerDefn.querySymbol} | ||||||
|${semanticDBDefn match { | ||||||
case None => | ||||||
s"semanticdb not found" | ||||||
case Some(defn) if defn.isEmpty => | ||||||
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 | ||||||
|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 using pc, found symbol in pc: ${compilerDefn.querySymbol}", | ||||||
path = Some(path.toURI), | ||||||
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) | ||||||
} | ||||||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just keep the current order for Ammonite? With this change we would not use compiler for Ammonite, right?
I wonder if we could just adjust tests to take that into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also fix it and map generated Ammonite files to source files like we do for Scala CLI, if we want to still have Ammonite support.
Reordering is only half of a fix since for local symbols using pc will lead to generated sources. But I can also do that, if don't know where we are with Ammonite support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I raised the topic on discord again and will start pushing it. In the meantime I don't think we want to work on any fixes, which we will be removing in the future.
So if it's too much work let's just leave it as is then.