Skip to content

Commit

Permalink
refactor: Do not index extension methods in workspaceSymbolIndex
Browse files Browse the repository at this point in the history
close scalameta#4212

In scalameta#4141 we started indexing the
extension methods (for auto-import missing extension methods) in
`workspaceSymbolIndex` where we should index only top-level symbols.

Later in scalameta#4183 we decided to index
those extension method symbols in a separate index
(see more details around here: scalameta#4183 (review)).

This PR refactor import missing extension method to use
the index only for extension method symbols.

This change slightly improve the performance of symbol search
and reduce the memory footprint for symbol index.
  • Loading branch information
tanishiking committed Aug 26, 2022
1 parent 1f4b6fd commit 5536080
Show file tree
Hide file tree
Showing 11 changed files with 46 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ final class CodeActionProvider(

private val allActions: List[CodeAction] = List(
new ImplementAbstractMembers(compilers),
new ImportMissingSymbol(compilers),
new ImportMissingSymbol(compilers, buildTargets),
new CreateNewSymbol(),
new StringActions(buffers),
extractMemberAction,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,11 +385,15 @@ class Compilers(
def autoImports(
params: TextDocumentPositionParams,
name: String,
findExtensionMethods: Boolean,
token: CancelToken,
): Future[ju.List[AutoImportsResult]] = {
withPCAndAdjustLsp(params) { (pc, pos, adjust) =>
pc.autoImports(name, CompilerOffsetParams.fromPos(pos, token))
.asScala
pc.autoImports(
name,
CompilerOffsetParams.fromPos(pos, token),
findExtensionMethods,
).asScala
.map { list =>
list.map(adjust.adjustImportResult)
list
Expand Down
10 changes: 1 addition & 9 deletions metals/src/main/scala/scala/meta/internal/metals/Indexer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -522,15 +522,7 @@ final case class Indexer(
val methodSymbols = ArrayBuffer.empty[WorkspaceSymbolInformation]
SemanticdbDefinition.foreach(input, dialect) {
case SemanticdbDefinition(info, occ, owner) =>
// TODO: Do not index (extension) METHOD, they will be indexed later
// we index methods for auto-import missing extension methods feature for now
// but those feature should use methodSymbols
// see: https://github.com/scalameta/metals/issues/4212
if (
WorkspaceSymbolProvider.isRelevantKind(
info.kind
) || info.kind == Kind.METHOD
) {
if (WorkspaceSymbolProvider.isRelevantKind(info.kind)) {
occ.range.foreach { range =>
symbols += WorkspaceSymbolInformation(
info.symbol,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import scala.meta.pc.CancelToken

import org.eclipse.{lsp4j => l}

class ImportMissingSymbol(compilers: Compilers) extends CodeAction {
class ImportMissingSymbol(compilers: Compilers, buildTargets: BuildTargets)
extends CodeAction {

override def kind: String = l.CodeActionKind.QuickFix

Expand All @@ -19,6 +20,15 @@ class ImportMissingSymbol(compilers: Compilers) extends CodeAction {
)(implicit ec: ExecutionContext): Future[Seq[l.CodeAction]] = {

val uri = params.getTextDocument().getUri()
val file = uri.toAbsolutePath
lazy val isScala3 =
(for {
buildId <- buildTargets.inverseSources(file)
target <- buildTargets.scalaTarget(buildId)
isScala3 = ScalaVersions.isScala3Version(
target.scalaInfo.getScalaVersion()
)
} yield isScala3).getOrElse(false)

def joinActionEdits(actions: Seq[l.CodeAction]) = {
actions
Expand All @@ -36,13 +46,19 @@ class ImportMissingSymbol(compilers: Compilers) extends CodeAction {
def importMissingSymbol(
diagnostic: l.Diagnostic,
name: String,
findExtensionMethods: Boolean = false,
): Future[Seq[l.CodeAction]] = {
val textDocumentPositionParams = new l.TextDocumentPositionParams(
params.getTextDocument(),
diagnostic.getRange.getEnd(),
)
compilers
.autoImports(textDocumentPositionParams, name, token)
.autoImports(
textDocumentPositionParams,
name,
findExtensionMethods,
token,
)
.map { imports =>
imports.asScala.map { i =>
val uri = params.getTextDocument().getUri()
Expand Down Expand Up @@ -97,9 +113,12 @@ class ImportMissingSymbol(compilers: Compilers) extends CodeAction {
case diag @ ScalacDiagnostic.SymbolNotFound(name)
if params.getRange().overlapsWith(diag.getRange()) =>
importMissingSymbol(diag, name)
// `foo.xxx` where `xxx` is not a member of `foo`
// we search for `xxx` only when the target is Scala3
// considering there might be an extension method.
case d @ ScalacDiagnostic.NotAMember(name)
if params.getRange().overlapsWith(d.getRange()) =>
importMissingSymbol(d, name)
if isScala3 && params.getRange().overlapsWith(d.getRange()) =>
importMissingSymbol(d, name, findExtensionMethods = true)
}
)
.map { actions =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public abstract class PresentationCompiler {
/**
* Return the necessary imports for a symbol at the given position.
*/
public abstract CompletableFuture<List<AutoImportsResult>> autoImports(String name, OffsetParams params);
public abstract CompletableFuture<List<AutoImportsResult>> autoImports(String name, OffsetParams params, Boolean isExtension);

/**
* Return the missing implements and imports for the symbol at the given position.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ case class ScalaPresentationCompiler(

override def autoImports(
name: String,
params: OffsetParams
params: OffsetParams,
isExtension: java.lang.Boolean // ignore, because Scala2 doesn't support extension method
): CompletableFuture[ju.List[AutoImportsResult]] =
compilerAccess.withInterruptableCompiler(
List.empty[AutoImportsResult].asJava,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ final class AutoImportsProvider(
buildTargetIdentifier: String,
):

def autoImports(): List[AutoImportsResult] =
def autoImports(isExtension: Boolean): List[AutoImportsResult] =
val uri = params.uri
val filePath = Paths.get(uri)
driver.run(
Expand Down Expand Up @@ -67,7 +67,9 @@ final class AutoImportsProvider(
sym.name.show == query

val visitor = new CompilerSearchVisitor(name, visit)
search.search(name, buildTargetIdentifier, visitor)
if isExtension then
search.searchMethods(name, buildTargetIdentifier, visitor)
else search.search(name, buildTargetIdentifier, visitor)
val results = symbols.result.filter(isExactMatch(_, name))

if results.nonEmpty then
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ case class ScalaPresentationCompiler(
def autoImports(
name: String,
params: scala.meta.pc.OffsetParams,
isExtension: java.lang.Boolean,
): CompletableFuture[
ju.List[scala.meta.pc.AutoImportsResult]
] =
Expand All @@ -172,7 +173,7 @@ case class ScalaPresentationCompiler(
config,
buildTargetIdentifier,
)
.autoImports()
.autoImports(isExtension)
.asJava
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class ScalaToplevelMtags(
acceptTrivia()
val name = newIdentifier
withOwner(expect.owner) {
term(name.name, name.pos, Kind.OBJECT, 0)
term(name.name, name.pos, Kind.METHOD, 0)
}
loop(indent, isAfterNewline = false, region, None)
}
Expand Down
3 changes: 3 additions & 0 deletions tests/cross/src/main/scala/tests/BaseAutoImportsSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import munit.TestOptions

trait BaseAutoImportsSuite extends BaseCodeActionSuite {

val isExtensionMethods: Boolean = false

def check(
name: String,
original: String,
Expand Down Expand Up @@ -88,6 +90,7 @@ trait BaseAutoImportsSuite extends BaseCodeActionSuite {
offset,
cancelToken,
),
isExtensionMethods,
)
.get()
result.asScala.toList
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ class AutoImportExtensionMethodsSuite extends BaseAutoImportsSuite {

override def ignoreScalaVersion: Some[IgnoreScalaVersion] = Some(IgnoreScala2)

override val isExtensionMethods: Boolean = true

check(
"basic",
"""|object A:
Expand Down

0 comments on commit 5536080

Please sign in to comment.