From 5536080519a0787dae8f99523fcb0af46852947c Mon Sep 17 00:00:00 2001 From: Rikito Taniguchi Date: Fri, 26 Aug 2022 17:55:50 +0900 Subject: [PATCH] refactor: Do not index extension methods in workspaceSymbolIndex close https://github.com/scalameta/metals/issues/4212 In https://github.com/scalameta/metals/pull/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 https://github.com/scalameta/metals/pull/4183 we decided to index those extension method symbols in a separate index (see more details around here: https://github.com/scalameta/metals/pull/4183#pullrequestreview-1050733868). 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. --- .../internal/metals/CodeActionProvider.scala | 2 +- .../meta/internal/metals/Compilers.scala | 8 ++++-- .../scala/meta/internal/metals/Indexer.scala | 10 +------ .../codeactions/ImportMissingSymbol.scala | 27 ++++++++++++++++--- .../scala/meta/pc/PresentationCompiler.java | 2 +- .../pc/ScalaPresentationCompiler.scala | 3 ++- .../internal/pc/AutoImportsProvider.scala | 6 +++-- .../pc/ScalaPresentationCompiler.scala | 3 ++- .../internal/mtags/ScalaToplevelMtags.scala | 2 +- .../scala/tests/BaseAutoImportsSuite.scala | 3 +++ .../pc/AutoImportExtensionMethodsSuite.scala | 2 ++ 11 files changed, 46 insertions(+), 22 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/CodeActionProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/CodeActionProvider.scala index 1d8c23a48df..5fc0829ccfd 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/CodeActionProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/CodeActionProvider.scala @@ -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, diff --git a/metals/src/main/scala/scala/meta/internal/metals/Compilers.scala b/metals/src/main/scala/scala/meta/internal/metals/Compilers.scala index c7c8dbd92a1..a9799400879 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Compilers.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Compilers.scala @@ -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 diff --git a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala index d3705227024..4186c601e4f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala @@ -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, diff --git a/metals/src/main/scala/scala/meta/internal/metals/codeactions/ImportMissingSymbol.scala b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ImportMissingSymbol.scala index bc3e499cd26..55e1fef307c 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/codeactions/ImportMissingSymbol.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/codeactions/ImportMissingSymbol.scala @@ -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 @@ -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 @@ -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() @@ -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 => diff --git a/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java b/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java index d748c4d7121..3526b9c89ca 100644 --- a/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java +++ b/mtags-interfaces/src/main/java/scala/meta/pc/PresentationCompiler.java @@ -83,7 +83,7 @@ public abstract class PresentationCompiler { /** * Return the necessary imports for a symbol at the given position. */ - public abstract CompletableFuture> autoImports(String name, OffsetParams params); + public abstract CompletableFuture> autoImports(String name, OffsetParams params, Boolean isExtension); /** * Return the missing implements and imports for the symbol at the given position. diff --git a/mtags/src/main/scala-2/scala/meta/internal/pc/ScalaPresentationCompiler.scala b/mtags/src/main/scala-2/scala/meta/internal/pc/ScalaPresentationCompiler.scala index 90dcb49c8c3..a9b871e614a 100644 --- a/mtags/src/main/scala-2/scala/meta/internal/pc/ScalaPresentationCompiler.scala +++ b/mtags/src/main/scala-2/scala/meta/internal/pc/ScalaPresentationCompiler.scala @@ -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, diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/AutoImportsProvider.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/AutoImportsProvider.scala index 661431a3fb3..da01a8eb911 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/AutoImportsProvider.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/AutoImportsProvider.scala @@ -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( @@ -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 diff --git a/mtags/src/main/scala-3/scala/meta/internal/pc/ScalaPresentationCompiler.scala b/mtags/src/main/scala-3/scala/meta/internal/pc/ScalaPresentationCompiler.scala index e7125af8166..aaeab4e5696 100644 --- a/mtags/src/main/scala-3/scala/meta/internal/pc/ScalaPresentationCompiler.scala +++ b/mtags/src/main/scala-3/scala/meta/internal/pc/ScalaPresentationCompiler.scala @@ -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] ] = @@ -172,7 +173,7 @@ case class ScalaPresentationCompiler( config, buildTargetIdentifier, ) - .autoImports() + .autoImports(isExtension) .asJava } diff --git a/mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala b/mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala index 349ba9efc7f..b93e3e6e62e 100644 --- a/mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala +++ b/mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala @@ -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) } diff --git a/tests/cross/src/main/scala/tests/BaseAutoImportsSuite.scala b/tests/cross/src/main/scala/tests/BaseAutoImportsSuite.scala index 358a3294255..23e484df581 100644 --- a/tests/cross/src/main/scala/tests/BaseAutoImportsSuite.scala +++ b/tests/cross/src/main/scala/tests/BaseAutoImportsSuite.scala @@ -12,6 +12,8 @@ import munit.TestOptions trait BaseAutoImportsSuite extends BaseCodeActionSuite { + val isExtensionMethods: Boolean = false + def check( name: String, original: String, @@ -88,6 +90,7 @@ trait BaseAutoImportsSuite extends BaseCodeActionSuite { offset, cancelToken, ), + isExtensionMethods, ) .get() result.asScala.toList diff --git a/tests/cross/src/test/scala/tests/pc/AutoImportExtensionMethodsSuite.scala b/tests/cross/src/test/scala/tests/pc/AutoImportExtensionMethodsSuite.scala index 37a61a2cda1..a285dda68df 100644 --- a/tests/cross/src/test/scala/tests/pc/AutoImportExtensionMethodsSuite.scala +++ b/tests/cross/src/test/scala/tests/pc/AutoImportExtensionMethodsSuite.scala @@ -6,6 +6,8 @@ class AutoImportExtensionMethodsSuite extends BaseAutoImportsSuite { override def ignoreScalaVersion: Some[IgnoreScalaVersion] = Some(IgnoreScala2) + override val isExtensionMethods: Boolean = true + check( "basic", """|object A: