-
Notifications
You must be signed in to change notification settings - Fork 338
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
8
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
8 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 e616ec8
improvement: reorder for ammonite
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,48 +78,77 @@ 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 => | ||||||
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) | ||||||
val reportBuilder = new DefinitionProviderReportBuilder(path, params) | ||||||
lazy val isScala3 = ScalaVersions.isScala3Version( | ||||||
scalaVersionSelector.scalaVersionForPath(path) | ||||||
) | ||||||
|
||||||
def fromCompiler() = | ||||||
if (path.isScalaFilename) { | ||||||
compilers() | ||||||
.definition(params, token) | ||||||
.map(reportBuilder.withCompilerResult) | ||||||
.map { | ||||||
case res if res.isEmpty => Some(res) | ||||||
case res => | ||||||
val pathToDef = res.locations.asScala.head.getUri.toAbsolutePath | ||||||
Some( | ||||||
res.copy(semanticdb = | ||||||
semanticdbs().textDocument(pathToDef).documentIncludingStale | ||||||
) | ||||||
) | ||||||
} | ||||||
case defn => | ||||||
if (fromSemanticdb.isEmpty) { | ||||||
warnings().noSemanticdb(path) | ||||||
} else Future.successful(None) | ||||||
|
||||||
def fromSemanticDb() = Future.successful { | ||||||
semanticdbs() | ||||||
.textDocument(path) | ||||||
.documentIncludingStale | ||||||
.map(definitionFromSnapshot(path, params, _)) | ||||||
.map(reportBuilder.withSemanticDBResult) | ||||||
} | ||||||
|
||||||
def fromScalaDoc() = Future.successful { | ||||||
scaladocDefinitionProvider | ||||||
.definition(path, params, isScala3) | ||||||
.map(reportBuilder.withFoundScaladocDef) | ||||||
} | ||||||
|
||||||
def fromFallback() = | ||||||
Future.successful( | ||||||
fallback | ||||||
.search(path, params.getPosition(), isScala3, reportBuilder) | ||||||
.map(reportBuilder.withFallbackResult) | ||||||
) | ||||||
|
||||||
val strategies: List[() => Future[Option[DefinitionResult]]] = | ||||||
if (isAmmonnite(path)) | ||||||
List(fromSemanticDb, fromCompiler, fromScalaDoc, fromFallback) | ||||||
else List(fromCompiler, fromSemanticDb, fromScalaDoc, fromFallback) | ||||||
|
||||||
for { | ||||||
result <- strategies.foldLeft(Future.successful(DefinitionResult.empty)) { | ||||||
case (acc, next) => | ||||||
acc.flatMap { | ||||||
case res if res.isEmpty && !res.symbol.endsWith("/") => | ||||||
next().map(_.getOrElse(res)) | ||||||
case res => Future.successful(res) | ||||||
} | ||||||
Future.successful(defn) | ||||||
} | ||||||
|
||||||
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 | ||||||
} yield { | ||||||
reportBuilder.build().foreach(rc.unsanitized.create(_)) | ||||||
result | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -508,3 +537,96 @@ class DestinationProvider( | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
class DefinitionProviderReportBuilder( | ||||||
path: AbsolutePath, | ||||||
params: TextDocumentPositionParams, | ||||||
) { | ||||||
private var compilerDefn: Option[DefinitionResult] = None | ||||||
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 withCompilerResult(result: DefinitionResult): DefinitionResult = { | ||||||
compilerDefn = Some(result) | ||||||
result | ||||||
} | ||||||
|
||||||
def withSemanticDBResult(result: DefinitionResult): DefinitionResult = { | ||||||
semanticDBDefn = Some(result) | ||||||
result | ||||||
} | ||||||
|
||||||
def withFallbackResult(result: DefinitionResult): DefinitionResult = { | ||||||
fallbackDefn = Some(result) | ||||||
result | ||||||
} | ||||||
|
||||||
def withError(e: Throwable): Unit = { | ||||||
error = Some(e) | ||||||
} | ||||||
|
||||||
def withNonLocalGuesses(guesses: List[String]): Unit = { | ||||||
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
This is more a setter than a builder method, no? Could we make them more consistent or rename as above? |
||||||
nonLocalGuesses = guesses | ||||||
} | ||||||
|
||||||
def withFoundScaladocDef(result: DefinitionResult): DefinitionResult = { | ||||||
fundScaladocDef = true | ||||||
result | ||||||
} | ||||||
|
||||||
def build(): Option[Report] = | ||||||
compilerDefn match { | ||||||
case Some(compilerDefn) if !fundScaladocDef => | ||||||
Some( | ||||||
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, | ||||||
) | ||||||
) | ||||||
case _ => None | ||||||
} | ||||||
|
||||||
private def querySymbol: Option[String] = | ||||||
compilerDefn.map(_.querySymbol) match { | ||||||
case Some("") | None => | ||||||
semanticDBDefn | ||||||
.map(_.querySymbol) | ||||||
.orElse(fallbackDefn.map(_.querySymbol)) | ||||||
case res => res | ||||||
} | ||||||
} |
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.