From a1d20780c9fea78d394a8fda0180ec9c4d3c620d Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Tue, 5 May 2026 19:05:54 +0200 Subject: [PATCH] bugfix: Initialize definitionIndexStrategy when initializing Previously, we would do it after initialized at which point the indexing has already happened. Now, we index according to the strategy the user wants. The other option is to handle the change, but if someone uses non default, they will always get double indexing. --- .../metals/mcp/StandaloneMcpService.scala | 3 ++- .../scala/scala/meta/metals/McpMain.scala | 4 +++- .../internal/metals/ClientConfiguration.scala | 23 +++++++++++++++---- .../scala/meta/internal/metals/Configs.scala | 3 +-- .../scala/meta/internal/metals/Indexer.scala | 6 +++-- .../metals/InitializationOptions.scala | 6 +++++ .../internal/metals/MetalsLspService.scala | 2 +- .../internal/metals/UserConfiguration.scala | 21 +---------------- .../internal/metals/WorkspaceLspService.scala | 1 + .../src/main/scala/tests/BaseLspSuite.scala | 1 - .../test/scala/tests/CompletionLspSuite.scala | 9 +++++--- .../src/test/scala/tests/ManualSuite.scala | 1 - .../scala/tests/UserConfigurationSuite.scala | 8 +++++-- 13 files changed, 50 insertions(+), 38 deletions(-) diff --git a/metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scala b/metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scala index 90085b6e34a..bfbcfc33a2f 100644 --- a/metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scala +++ b/metals-mcp/src/main/scala/scala/meta/internal/metals/mcp/StandaloneMcpService.scala @@ -63,7 +63,8 @@ class StandaloneMcpService( private val time: Time = Time.system private val clientConfig: ClientConfiguration = ClientConfiguration( - MetalsServerConfig.default + MetalsServerConfig.default, + NoopFeatureFlagProvider, ) private val mcpClient = new ConfiguredLanguageClient( diff --git a/metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala b/metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala index 4a3b2063118..377d93ddcf9 100644 --- a/metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala +++ b/metals-mcp/src/main/scala/scala/meta/metals/McpMain.scala @@ -8,6 +8,7 @@ import scala.concurrent.ExecutionContext import scala.concurrent.ExecutionContextExecutorService import scala.util.control.NonFatal +import scala.meta.internal.infra.NoopFeatureFlagProvider import scala.meta.internal.metals.BuildInfo import scala.meta.internal.metals.ClientConfiguration import scala.meta.internal.metals.MetalsServerConfig @@ -329,7 +330,8 @@ object McpMain { json.addProperty(k, v) } } - val clientConfiguration = ClientConfiguration(MetalsServerConfig.default) + val clientConfiguration = + ClientConfiguration(MetalsServerConfig.default, NoopFeatureFlagProvider) UserConfiguration.fromJson(json, clientConfiguration) } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scala b/metals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scala index 18dad4ba1f5..378c1f47d56 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/ClientConfiguration.scala @@ -1,5 +1,8 @@ package scala.meta.internal.metals +import scala.meta.infra.FeatureFlagProvider +import scala.meta.internal.infra.NoopFeatureFlagProvider +import scala.meta.internal.metals.Configs.DefinitionIndexStrategy import scala.meta.internal.metals.Configs.GlobSyntaxConfig import scala.meta.internal.metals.config.DoctorFormat import scala.meta.internal.metals.config.StatusBarState @@ -19,6 +22,7 @@ import org.eclipse.lsp4j.InitializeParams final class ClientConfiguration( val initialConfig: MetalsServerConfig, initializeParams: Option[InitializeParams], + featureFlags: FeatureFlagProvider, ) { private val clientCapabilities: Option[ClientCapabilities] = @@ -67,6 +71,14 @@ final class ClientConfiguration( .flatMap(GlobSyntaxConfig.fromString) .getOrElse(initialConfig.globSyntax) + def definitionIndexStrategy(): DefinitionIndexStrategy = + DefinitionIndexStrategy + .fromConfigOrFeatureFlag( + initializationOptions.definitionIndexStrategy, + featureFlags, + ) + .getOrElse(DefinitionIndexStrategy.default) + def renameFileThreshold(): Int = initializationOptions.renameFileThreshold.getOrElse( initialConfig.renameFileThreshold @@ -238,18 +250,21 @@ final class ClientConfiguration( } object ClientConfiguration { - val default: ClientConfiguration = ClientConfiguration(MetalsServerConfig()) + val default: ClientConfiguration = + ClientConfiguration(MetalsServerConfig(), NoopFeatureFlagProvider) def apply( initialConfig: MetalsServerConfig, initializeParams: InitializeParams, + featureFlags: FeatureFlagProvider, ): ClientConfiguration = { - new ClientConfiguration(initialConfig, Some(initializeParams)) + new ClientConfiguration(initialConfig, Some(initializeParams), featureFlags) } def apply( - initialConfig: MetalsServerConfig + initialConfig: MetalsServerConfig, + featureFlags: FeatureFlagProvider, ): ClientConfiguration = { - new ClientConfiguration(initialConfig, None) + new ClientConfiguration(initialConfig, None, featureFlags) } } diff --git a/metals/src/main/scala/scala/meta/internal/metals/Configs.scala b/metals/src/main/scala/scala/meta/internal/metals/Configs.scala index 272608b8094..5fc3869a6f6 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Configs.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Configs.scala @@ -389,8 +389,7 @@ object Configs { // which fails on syntax errors. See plans/protopc.md for details. def default: ProtoOutlineProviderConfig = v1 def fromConfigOrFeatureFlag( - value: Option[String], - featureFlags: FeatureFlagProvider, + value: Option[String] ): Either[String, ProtoOutlineProviderConfig] = { value match { case Some(ok @ ("v1" | "v2")) => 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 cc1f0bf3be2..423341cce57 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/Indexer.scala @@ -248,7 +248,7 @@ case class Indexer(indexProviders: IndexProviders, mbtBuild: () => MbtBuild)( ) progress.message = s"indexing ${buildTool.importedBuild.dependencyModules.getItems().size()} dependencies" - if (indexProviders.userConfig.definitionIndexStrategy.isClasspath) { + if (indexProviders.clientConfig.definitionIndexStrategy().isClasspath) { usedJars ++= indexDependencyModules( buildTool.importedBuild.dependencyModules, progress, @@ -546,7 +546,9 @@ case class Indexer(indexProviders: IndexProviders, mbtBuild: () => MbtBuild)( if (!path.exists) { scribe.warn(s"dependency missing at absolute path: $path") } else if (path.isJar) { - if (!indexProviders.userConfig.definitionIndexStrategy.isClasspath) { + if ( + !indexProviders.clientConfig.definitionIndexStrategy().isClasspath + ) { usedJars += path if (addSourceJarSymbols(path)) cacheHits += 1 else cacheMisses += 1 diff --git a/metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala b/metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala index f7372457a4a..bcc149572b2 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/InitializationOptions.scala @@ -50,6 +50,8 @@ import org.eclipse.{lsp4j => l} * the output, you can enable this to strip them. * @param doctorVisibilityProvider if the clients implements `metals/doctorVisibilityDidChange` * @param bspStatusBarProvider if the client supports `metals/status` with "bsp" status type + * @param definitionIndexStrategy whether definitions from dependencies should index classfiles + * or source files. */ final case class InitializationOptions( compilerOptions: CompilerInitializationOptions, @@ -82,6 +84,7 @@ final case class InitializationOptions( doctorVisibilityProvider: Option[Boolean], bspStatusBarProvider: Option[String], moduleStatusBarProvider: Option[String], + definitionIndexStrategy: Option[String], ) { def doctorFormat: Option[DoctorFormat.DoctorFormat] = doctorProvider.flatMap(DoctorFormat.fromString) @@ -128,6 +131,7 @@ object InitializationOptions { None, None, None, + None, ) def from( @@ -188,6 +192,8 @@ object InitializationOptions { bspStatusBarProvider = jsonObj.getStringOption("bspStatusBarProvider"), moduleStatusBarProvider = jsonObj.getStringOption("moduleStatusBarProvider"), + definitionIndexStrategy = + jsonObj.getStringOption("definitionIndexStrategy"), ) } diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index d967282bbeb..75d166c34a4 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -2397,7 +2397,7 @@ abstract class MetalsLspService( }, toIndexSource = path => sourceMapper.mappedTo(path).getOrElse(path), isClasspathDefinitionIndexEnabled = () => - userConfig.definitionIndexStrategy.isClasspath, + clientConfig.definitionIndexStrategy().isClasspath, mtags = () => mtags, ) } diff --git a/metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala b/metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala index 37721afbeb0..72be52a2de8 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala @@ -14,7 +14,6 @@ import scala.meta.internal.infra.NoopFeatureFlagProvider import scala.meta.internal.metals.Configs.AdditionalPcChecksConfig import scala.meta.internal.metals.Configs.BatchSemanticdbConfig import scala.meta.internal.metals.Configs.CompilerProgressConfig -import scala.meta.internal.metals.Configs.DefinitionIndexStrategy import scala.meta.internal.metals.Configs.DefinitionProviderConfig import scala.meta.internal.metals.Configs.FallbackClasspathConfig import scala.meta.internal.metals.Configs.FallbackSourcepathConfig @@ -99,8 +98,6 @@ case class UserConfiguration( WorkspaceSymbolProviderConfig.default, definitionProviders: DefinitionProviderConfig = DefinitionProviderConfig.default, - definitionIndexStrategy: DefinitionIndexStrategy = - DefinitionIndexStrategy.default, protoOutlineProvider: ProtoOutlineProviderConfig = ProtoOutlineProviderConfig.default, javaSymbolLoader: JavaSymbolLoaderConfig = JavaSymbolLoaderConfig.default, @@ -275,12 +272,6 @@ case class UserConfiguration( definitionProviders.values.asJava, ) ), - Some( - ( - "definitionIndexStrategy", - definitionIndexStrategy.value, - ) - ), Some( ( "protoOutlineProvider", @@ -1358,14 +1349,6 @@ object UserConfiguration { featureFlags, ), ).getOrElse(WorkspaceSymbolProviderConfig.default) - val definitionIndexStrategy = getParsedKey( - "definition-index-strategy", - value => - DefinitionIndexStrategy.fromConfigOrFeatureFlag( - value, - featureFlags, - ), - ).getOrElse(DefinitionIndexStrategy.default) val definitionProviders = getParsedArrayKey( "definition-providers", values => @@ -1378,8 +1361,7 @@ object UserConfiguration { "proto-outline-provider", value => ProtoOutlineProviderConfig.fromConfigOrFeatureFlag( - value, - featureFlags, + value ), ).getOrElse(ProtoOutlineProviderConfig.default) val javaSymbolLoader = getParsedKey( @@ -1521,7 +1503,6 @@ object UserConfiguration { useSourcePath, workspaceSymbolProvider, definitionProviders, - definitionIndexStrategy, protoOutlineProvider, javaSymbolLoader, javaTurbineRecompileDelay, diff --git a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala index b22ccd4376a..25e917624dd 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/WorkspaceLspService.scala @@ -140,6 +140,7 @@ class WorkspaceLspService( ClientConfiguration( serverInputs.initialServerConfig, initializeParams, + featureFlags, ) private val languageClient = { diff --git a/tests/unit/src/main/scala/tests/BaseLspSuite.scala b/tests/unit/src/main/scala/tests/BaseLspSuite.scala index 44e15b2b283..133c16ad62a 100644 --- a/tests/unit/src/main/scala/tests/BaseLspSuite.scala +++ b/tests/unit/src/main/scala/tests/BaseLspSuite.scala @@ -51,7 +51,6 @@ abstract class BaseLspSuite( buildChangedAction = BuildChangedAction.prompt, fallbackScalaVersion = Some(BuildInfo.scalaVersion), presentationCompilerDiagnostics = false, - definitionIndexStrategy = Configs.DefinitionIndexStrategy.classpath, // Legacy settings that are enabled for tests only. We should eventually // update the tests to use the new defaults. diff --git a/tests/unit/src/test/scala/tests/CompletionLspSuite.scala b/tests/unit/src/test/scala/tests/CompletionLspSuite.scala index 662bb50edba..926d82f34e7 100644 --- a/tests/unit/src/test/scala/tests/CompletionLspSuite.scala +++ b/tests/unit/src/test/scala/tests/CompletionLspSuite.scala @@ -3,7 +3,7 @@ package tests import scala.concurrent.Future import scala.meta.internal.metals.Configs.DefinitionIndexStrategy -import scala.meta.internal.metals.UserConfiguration +import scala.meta.internal.metals.InitializationOptions import scala.meta.internal.metals.{BuildInfo => V} import munit.Location @@ -12,8 +12,11 @@ class CompletionLspSuite extends BaseCompletionLspSuite("completion") { override def munitIgnore: Boolean = isWindows - override def userConfig: UserConfiguration = super.userConfig - .copy(definitionIndexStrategy = DefinitionIndexStrategy.sources) + override def initializationOptions: Option[InitializationOptions] = Some( + InitializationOptions.Default.copy( + definitionIndexStrategy = Some(DefinitionIndexStrategy.sources.value) + ) + ) test("basic-213") { basicTest(V.scala213) diff --git a/tests/unit/src/test/scala/tests/ManualSuite.scala b/tests/unit/src/test/scala/tests/ManualSuite.scala index cd6a9aef51d..0faf860eec0 100644 --- a/tests/unit/src/test/scala/tests/ManualSuite.scala +++ b/tests/unit/src/test/scala/tests/ManualSuite.scala @@ -18,7 +18,6 @@ class ManualSuite extends BaseManualSuite { workspaceSymbolProvider = WorkspaceSymbolProviderConfig.mbt, javaSymbolLoader = JavaSymbolLoaderConfig.turbineClasspath, presentationCompilerDiagnostics = true, - definitionIndexStrategy = DefinitionIndexStrategy.classpath, fallbackSourcepath = FallbackSourcepathConfig.allSources, compilerProgress = CompilerProgressConfig.enabled, referenceProvider = ReferenceProviderConfig.mbt, diff --git a/tests/unit/src/test/scala/tests/UserConfigurationSuite.scala b/tests/unit/src/test/scala/tests/UserConfigurationSuite.scala index 7a32c01c173..901f35730ea 100644 --- a/tests/unit/src/test/scala/tests/UserConfigurationSuite.scala +++ b/tests/unit/src/test/scala/tests/UserConfigurationSuite.scala @@ -6,6 +6,7 @@ import java.util.Properties import scala.meta.infra.FeatureFlag import scala.meta.infra.FeatureFlagProvider +import scala.meta.internal.infra.NoopFeatureFlagProvider import scala.meta.internal.metals.AutoImportBuildKind import scala.meta.internal.metals.BloopJvmProperties import scala.meta.internal.metals.ClientConfiguration @@ -494,7 +495,6 @@ class UserConfigurationSuite extends BaseSuite { "mbt", "protobuf" ], - "definitionIndexStrategy": "classpath", "protoOutlineProvider": "v1", "javaSymbolLoader": "turbine-classpath", "javaTurbineRecompileDelay": "100 milliseconds", @@ -533,7 +533,11 @@ class UserConfigurationSuite extends BaseSuite { Map("testExplorerProvider" -> true).asJava.toJsonObject ) - val clientConfig = ClientConfiguration(MetalsServerConfig.default, params) + val clientConfig = ClientConfiguration( + MetalsServerConfig.default, + params, + NoopFeatureFlagProvider, + ) val roundtrip = UserConfiguration .fromJson(