Fix stale Java diagnostics after MBT import - #8689
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe change coordinates MBT workspace import, Turbine recompilation, and diagnostic refreshes. It adds generation checks for Turbine and presentation compiler work, isolates Java compiler name tables, and adds concurrency and manual-import tests. ChangesMBT refresh flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionProvider
participant MetalsLspService
participant TurbineCompiler
participant Compilers
Client->>ConnectionProvider: Import MBT workspace
ConnectionProvider->>ConnectionProvider: Reload eligible session
ConnectionProvider->>MetalsLspService: Invoke refreshMbtStateAfterIndex
MetalsLspService->>TurbineCompiler: Recompile Turbine classpath
MetalsLspService->>Compilers: Cancel and refresh diagnostics
Compilers-->>Client: Publish current diagnostics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala (1)
702-712: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the
isTurbineClasspathcheck to ensure cache invalidation runs.By restricting
onMbtBuildWrittento only run when Turbine is enabled, you prevent the Java compiler cache invalidation and diagnostics refresh from running during a workspace reload for users without Turbine. SincerefreshDiagnosticsAfterMbtBuildUpdatealready performs its own internal check for Turbine before recompiling, you can safely remove the outer check here.Note: You may also consider renaming this method (e.g., to
refreshMbtDiagnostics) since it now encapsulates broader cache clearing and refresh logic rather than just the Turbine classpath.🐛 Proposed fix
private def refreshMbtTurbineClasspath( session: BspSession ): Future[Unit] = - if ( - MbtBuildServer.isMbtServer(session.main.name) && - userConfig.javaSymbolLoader.isTurbineClasspath - ) { + if (MbtBuildServer.isMbtServer(session.main.name)) { onMbtBuildWritten(mbtBuild()) } else { Future.unit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala` around lines 702 - 712, Update refreshMbtTurbineClasspath to invoke onMbtBuildWritten(mbtBuild()) whenever the session is an MBT server, removing the userConfig.javaSymbolLoader.isTurbineClasspath condition. Leave the non-MBT path returning Future.unit; renaming the method is optional and not required for this fix.
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala (1)
628-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
shutdown()calls on evicted cache entries.The Guava
presentationCompilerCachebackingjcacheis already configured with aRemovalListenerthat automatically invokesshutdown()on any removed or evictedMtagsPresentationCompiler. Explicitly callingshutdown()on entries removed viajcache.remove(...)is redundant and can be omitted.
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala#L628-L634: Remove thepc.shutdown()loop logic inclearJavaCompilerCacheand just rely onjcache.remove(key).metals/src/main/scala/scala/meta/internal/metals/Compilers.scala#L680-L683: Remove the.foreach { compiler => compiler.shutdown() }call block insiderestartPresentationCompilers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala` around lines 628 - 634, Remove explicit compiler shutdown calls because presentationCompilerCache’s RemovalListener already shuts down evicted entries. In clearJavaCompilerCache, retain only the jcache.remove(key) operations at metals/src/main/scala/scala/meta/internal/metals/Compilers.scala:628-634; in restartPresentationCompilers, remove the compiler.shutdown() foreach block at metals/src/main/scala/scala/meta/internal/metals/Compilers.scala:680-683.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 1164-1178: Remove the second compilers.clearJavaCompilerCache()
and subsequent refreshAllDiagnostics() calls from
refreshDiagnosticsAfterMbtBuildUpdate. Preserve the initial cache clear and
diagnostics refresh sequence after the MBT update and Turbine recompilation.
---
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala`:
- Around line 702-712: Update refreshMbtTurbineClasspath to invoke
onMbtBuildWritten(mbtBuild()) whenever the session is an MBT server, removing
the userConfig.javaSymbolLoader.isTurbineClasspath condition. Leave the non-MBT
path returning Future.unit; renaming the method is optional and not required for
this fix.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 628-634: Remove explicit compiler shutdown calls because
presentationCompilerCache’s RemovalListener already shuts down evicted entries.
In clearJavaCompilerCache, retain only the jcache.remove(key) operations at
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala:628-634; in
restartPresentationCompilers, remove the compiler.shutdown() foreach block at
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala:680-683.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb1a5fbd-be72-4c9d-8b45-920d8d732456
📒 Files selected for processing (5)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/MbtImport.scala
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala (1)
1236-1241: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure Turbine recompilation and Java compiler cache clearing on
mbt.jsonchanges.When
mbt.jsonis modified directly, the current flow updates the build state and refreshes diagnostics, but misses recompiling the Turbine classpath and clearing the Java compiler cache. This bypasses the fixes added for MBT imports, which can lead to stale Java diagnostics.Reusing
refreshDiagnosticsAfterMbtBuildUpdateafter the reconnection ensures that all compiler caches and classpaths are consistently and fully invalidated.🐛 Proposed fix
case Some(mbtJsonPath) => - Future { - updateMbtBuild(MbtBuild.fromFile(mbtJsonPath.toNIO)) - }.flatMap(_ => reconnectAfterMbtJsonChange()) - .flatMap(_ => refreshAllDiagnostics()) + Future { MbtBuild.fromFile(mbtJsonPath.toNIO) } + .flatMap { build => + Future { updateMbtBuild(build) } + .flatMap(_ => reconnectAfterMbtJsonChange()) + .flatMap(_ => refreshDiagnosticsAfterMbtBuildUpdate(build)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala` around lines 1236 - 1241, Update the mbt.json change flow after reconnectAfterMbtJsonChange() to call refreshDiagnosticsAfterMbtBuildUpdate instead of refreshAllDiagnostics(), preserving the existing updateMbtBuild step so Turbine recompilation and Java compiler cache clearing use the established refresh path.
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala (1)
1180-1188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Future.traverseovermapandFuture.sequence.Using
Future.traverseis a more idiomatic and concise way to evaluate a collection of futures in Scala.♻️ Proposed fix
- val futures = - buffers.open.filter(isIncludedPath).map { path => - for { - reportedDiagnostics <- compilers.didFocus(path) - _ = diagnostics - .publishDiagnosticsNotAdjusted(path, reportedDiagnostics) - } yield () - } - Future.sequence(futures).map(_ => ()) + Future + .traverse(buffers.open.filter(isIncludedPath)) { path => + for { + reportedDiagnostics <- compilers.didFocus(path) + _ = diagnostics + .publishDiagnosticsNotAdjusted(path, reportedDiagnostics) + } yield () + } + .map(_ => ())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala` around lines 1180 - 1188, Update the future aggregation in the diagnostics flow to use Future.traverse directly on buffers.open.filter(isIncludedPath), preserving the existing per-path compilers.didFocus and publishDiagnosticsNotAdjusted operations and the Unit result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 1236-1241: Update the mbt.json change flow after
reconnectAfterMbtJsonChange() to call refreshDiagnosticsAfterMbtBuildUpdate
instead of refreshAllDiagnostics(), preserving the existing updateMbtBuild step
so Turbine recompilation and Java compiler cache clearing use the established
refresh path.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 1180-1188: Update the future aggregation in the diagnostics flow
to use Future.traverse directly on buffers.open.filter(isIncludedPath),
preserving the existing per-path compilers.didFocus and
publishDiagnosticsNotAdjusted operations and the Unit result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 232410cb-ba1f-4489-be3e-457f636407b6
📒 Files selected for processing (1)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
01f6f47 to
2d71199
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala (1)
616-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRely on the Guava Cache removal listener for cleanup.
Guava's
presentationCompilerCachealready configures a synchronous removal listener that automatically calls.shutdown()on theMtagsPresentationCompilerwhenever an entry is removed. Explicitly callingshutdown()here is redundant.Consider simplifying the loop to just remove the keys.
♻️ Proposed refactor
- for { - key <- javaKeys - pc <- Option(jcache.remove(key)) - } { - pc.shutdown() - } + javaKeys.foreach(jcache.remove)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala` around lines 616 - 634, Update clearJavaCompilerCache to remove each matching key from jcache without capturing the removed compiler or calling pc.shutdown(); rely on the cache’s synchronous removal listener to perform compiler cleanup. Preserve the existing Java-key filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala`:
- Around line 244-274: Prevent duplicate MBT refreshes and ensure diagnostics
refresh regardless of Turbine configuration. In
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala:244-274,
remove onMbtBuildUpdatedAfterImport and its chaining from runMbtReimport and
reimportMbtAndReload, relying on reloadCurrentSession/importBuildAndIndex;
update forceMbtReimport as needed to use the unconditional import directly. In
710-717, optionally rename refreshMbtTurbineClasspath to refreshMbtState. In
725-735, remove the isTurbineClasspath guard so onMbtBuildUpdated always runs
after BSP indexing.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 616-634: Update clearJavaCompilerCache to remove each matching key
from jcache without capturing the removed compiler or calling pc.shutdown();
rely on the cache’s synchronous removal listener to perform compiler cleanup.
Preserve the existing Java-key filtering behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2825638-1ffd-4779-a9f3-a3a3160ed586
📒 Files selected for processing (4)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
2d71199 to
755db23
Compare
26864e9 to
632836e
Compare
97dc22e to
9ef51e3
Compare
9ef51e3 to
6675839
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala`:
- Around line 698-703: Update the production branch in importBuildAndIndex’s
refresh handling so it returns the recovered refresh future rather than
discarding refresh.recover’s result and immediately returning
Future.unit.withInterrupt. Preserve the existing warning recovery and interrupt
behavior while ensuring completion awaits the diagnostics refresh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d52796e7-7bfe-4e43-85f6-1d8de299ee50
📒 Files selected for processing (3)
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/ProjectMetalsLspService.scala
There was a problem hiding this comment.
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala (1)
288-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated reload-then-import sequence.
This mirrors
reloadCurrentSession(Line 251-265):session.workspaceReload()followed byconnect(new ImportBuildAndIndex(session), progress). Consider extracting a shared private helper (e.g.,reloadAndImport(session, progress): Future[BuildChange]) to avoid the two call sites diverging.♻️ Proposed refactor
+ private def reloadAndImportSession( + session: BspSession, + progress: TaskProgress, + ): Future[BuildChange] = + for { + _ <- session.workspaceReload() + buildChange <- connect(new ImportBuildAndIndex(session), progress) + } yield buildChange + def reloadCurrentSession(): Future[Unit] = bspSession match { case Some(session) if session.canReloadWorkspace => workDoneProgress.trackProgressFuture( "Sync", - progress => - for { - _ <- session.workspaceReload() - _ <- connect(new ImportBuildAndIndex(session), progress) - } yield (), + progress => reloadAndImportSession(session, progress).ignoreValue, metricName = Some("reload_build_server"), ) case _ => fullConnect() }bspSession match { case Some(session) if MbtBuildServer.isMbtServer(session.main.name) && session.canReloadWorkspace => - for { - _ <- session.workspaceReload() - buildChange <- connect(new ImportBuildAndIndex(session), progress) - } yield buildChange + reloadAndImportSession(session, progress) case Some(session) => connect(new ImportBuildAndIndex(session), progress)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala` around lines 288 - 294, Extract the duplicated session.workspaceReload() followed by connect(new ImportBuildAndIndex(session), progress) flow from reloadCurrentSession and the MbtBuildServer branch into a shared private helper such as reloadAndImport(session, progress). Replace both call sites with the helper while preserving their existing Future[BuildChange] behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala`:
- Around line 288-294: Extract the duplicated session.workspaceReload() followed
by connect(new ImportBuildAndIndex(session), progress) flow from
reloadCurrentSession and the MbtBuildServer branch into a shared private helper
such as reloadAndImport(session, progress). Replace both call sites with the
helper while preserving their existing Future[BuildChange] behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 402e0cce-1828-49df-bdf2-7a7d5eb8baab
📒 Files selected for processing (4)
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalatests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala (1)
987-998: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
userConfigoverride.This is nearly identical to
MbtBuildServerLspSuite.userConfig(lines 46-58), differing only inautomaticImportBuildand the absence oftestUserInterface. Extracting a shared base trait/class with the common config would prevent the two configs from silently drifting apart.♻️ Proposed refactor sketch
+trait MbtBuildServerBaseUserConfig { self: BaseCompletionLspSuite => + protected def baseUserConfig: UserConfiguration = + self.userConfig.copy( + fallbackScalaVersion = Some(BuildInfo.scalaVersion), + presentationCompilerDiagnostics = true, + buildOnChange = false, + buildOnFocus = false, + workspaceSymbolProvider = WorkspaceSymbolProviderConfig.mbt, + referenceProvider = ReferenceProviderConfig.mbt, + fallbackSourcepath = FallbackSourcepathConfig("all-sources"), + preferredBuildServer = Some(MbtBuildServer.name), + ) +} + class MbtBuildServerManualImportLspSuite - extends BaseCompletionLspSuite("mbt-build-server-manual-import") { + extends BaseCompletionLspSuite("mbt-build-server-manual-import") + with MbtBuildServerBaseUserConfig { override def userConfig: UserConfiguration = - super.userConfig.copy( - fallbackScalaVersion = Some(BuildInfo.scalaVersion), - presentationCompilerDiagnostics = true, - buildOnChange = false, - buildOnFocus = false, - workspaceSymbolProvider = WorkspaceSymbolProviderConfig.mbt, - referenceProvider = ReferenceProviderConfig.mbt, - fallbackSourcepath = FallbackSourcepathConfig("all-sources"), - preferredBuildServer = Some(MbtBuildServer.name), - automaticImportBuild = AutoImportBuildKind.Off, - ) + baseUserConfig.copy(automaticImportBuild = AutoImportBuildKind.Off)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala` around lines 987 - 998, Remove the duplicated userConfig definition in MbtBuildServerLspSuite by extracting the shared configuration into a reusable base trait or class, then override only the suite-specific automaticImportBuild and testUserInterface differences. Ensure both suites continue using the common fallback, provider, build, and preferred server settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala`:
- Around line 987-998: Remove the duplicated userConfig definition in
MbtBuildServerLspSuite by extracting the shared configuration into a reusable
base trait or class, then override only the suite-specific automaticImportBuild
and testUserInterface differences. Ensure both suites continue using the common
fallback, provider, build, and preferred server settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dd4dbd6-80b8-47cd-9df2-c6e526feef5f
📒 Files selected for processing (4)
metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalatests/unit/src/test/scala/tests/mbt/MbtBuildServerLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (3)
- metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
- metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala
- metals/src/main/scala/scala/meta/internal/metals/ConnectionProvider.scala
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala (1)
244-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark source-path entries after a forced compile.
compileNow()passes the default emptymarkCompiledlist, socleanup()cannot remove any source-path entries. They continue to be served fromSOURCE_PATHafter the forced Turbine rebuild.Proposed fix
def compileNow(): Future[TurbineCompileResult] = { + val toCompile = sourcepathSources() val generation = compileGeneration.incrementAndGet() doCompile.cancelAll() Future { - doCompileNow(expectedGeneration = generation) + doCompileNow( + expectedGeneration = generation, + markCompiled = toCompile, + ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala` around lines 244 - 249, Update compileNow() to pass the source-path entries requiring cleanup to doCompileNow via its markCompiled parameter, rather than the default empty list. Ensure the forced Turbine compilation marks those entries so cleanup() removes them from SOURCE_PATH after rebuilding.metals/src/main/scala/scala/meta/internal/metals/Compilers.scala (1)
406-431: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease completed in-flight tokens.
inFlightDidChangeis never cleaned up, so every distinct edited path remains retained for the server lifetime. Remove only the matching token when its request completes.Proposed fix
timerProvider .withTimer( "computed diagnostics", reportStatus = false, onlyIf = false, ) { pc.didChange(params).asScala } + .andThen { case _ => + inFlightDidChange.remove(file, token) + } .map { case (timer, reportedDiagnostics) =>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala` around lines 406 - 431, Update the request completion handling in fileDidChange so the corresponding entry is removed from inFlightDidChange when its token completes. Remove it only if the stored token still matches the completed request’s token, preserving newer in-flight requests for the same path.metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala (1)
1232-1240: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestart Java compilers for watched proto deletions too.
pathsexcludes delete events, so deleting a.protoonly reachesonDelete; this branch never restarts Java presentation compilers. Cached generated symbols can therefore survive a watched proto deletion.Proposed fix
if (paths.exists(_.isProtoFilename)) { paths.filter(_.isProtoFilename).foreach(mbt2.didSave) - compilers.restartJavaCompilers() } + if ( + paths.exists(_.isProtoFilename) || + otherDeleteEvents.exists(_.getUri().toAbsolutePath.isProtoFilename) + ) + compilers.restartJavaCompilers()Based on the PR objective to invalidate stale diagnostics after MBT/proto changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala` around lines 1232 - 1240, Extend the proto-change handling in the visible Metals LSP service flow to also restart Java presentation compilers when a watched proto deletion is processed through onDelete, since deleted paths are absent from paths. Reuse the existing proto filename detection and compilers.restartJavaCompilers() behavior, while preserving the current didSave handling for existing proto paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 406-431: Update the request completion handling in fileDidChange
so the corresponding entry is removed from inFlightDidChange when its token
completes. Remove it only if the stored token still matches the completed
request’s token, preserving newer in-flight requests for the same path.
In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala`:
- Around line 244-249: Update compileNow() to pass the source-path entries
requiring cleanup to doCompileNow via its markCompiled parameter, rather than
the default empty list. Ensure the forced Turbine compilation marks those
entries so cleanup() removes them from SOURCE_PATH after rebuilding.
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 1232-1240: Extend the proto-change handling in the visible Metals
LSP service flow to also restart Java presentation compilers when a watched
proto deletion is processed through onDelete, since deleted paths are absent
from paths. Reuse the existing proto filename detection and
compilers.restartJavaCompilers() behavior, while preserving the current didSave
handling for existing proto paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54220ca3-646b-4a2a-a2d0-b70aa263b850
📒 Files selected for processing (5)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalatests/unit/src/test/scala/scala/meta/internal/metals/CompilersRaceLspSuite.scalatests/unit/src/test/scala/tests/mbt/MbtBuildServerManualImportLspSuite.scala
155bba8 to
ce50ab3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala (2)
1153-1164: 🎯 Functional Correctness | 🟠 MajorVerify concurrent-recompile safety of
recompileTurbineClasspath().A prior reviewer (tgodzik) flagged this exact pattern on an earlier commit: "The problem here is that we can't run the recompile if the turbine compilation is already running. We should cancel it and run again." The current
refreshMbtStateAfterIndexstill callsmbt2.recompileTurbineClasspath()unconditionally with no visible cancellation of an in-flight compile. This PR's stack description claimsTurbineCompiler.scala(not in this review batch) now does generation-aware suppression of stale results, which may address the correctness half of this concern, but doesn't necessarily avoid wasted duplicate compilation work whenrefreshMbtStateAfterIndexruns while a manual/previous recompile is still in progress.Please confirm whether the generation tracking in
TurbineCompiler.scalamakes concurrentrecompileTurbineClasspath()calls safe (correct + non-wasteful), or whether this call site still needs to cancel an in-flight compile first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala` around lines 1153 - 1164, Verify the concurrency contract of mbt2.recompileTurbineClasspath() in refreshMbtStateAfterIndex: ensure generation tracking prevents stale results and avoids duplicate work when a compile is already running. If it does not, cancel the in-flight Turbine compilation before starting the refresh, using the existing cancellation mechanism, while preserving the subsequent compiler cancellation, diagnostics reset, and refresh flow.
1284-1309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
onDelete's proto-triggered Java compiler restart is fire-and-forget and never refreshes diagnostics.Unlike
didSave(Line 1128-1139), which restarts Java compilers on proto save and then unconditionally awaitsrefreshAllDiagnostics(),onDeleterestarts Java compilers via.map { _ => if (path.isProtoFilename) compilers.restartJavaCompilers() }without chaining/awaiting the restart and without any subsequent diagnostics refresh. Open Java files depending on a deleted proto will keep showing stale (clean) diagnostics until the user manually refocuses them — which is exactly the class of bug this PR is meant to fix. The companion test inProtoPCJavaSuite.scalaworks around this by sleeping 500ms and manually refocusing (see linked comment there).🐛 Proposed fix
.ignoreValue - .map { _ => - if (path.isProtoFilename) compilers.restartJavaCompilers() - } + .flatMap { _ => + if (path.isProtoFilename) { + compilers.restartJavaCompilers() + refreshAllDiagnostics() + } else Future.successful(()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala` around lines 1284 - 1309, Update onDelete to chain and await the proto-triggered compilers.restartJavaCompilers() operation rather than invoking it fire-and-forget, then invoke and await refreshAllDiagnostics() after the restart, matching the didSave flow while preserving the existing behavior for non-proto deletions.
🧹 Nitpick comments (2)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scala (1)
358-403: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: consider real cancellation instead of compute-then-discard for stale
didFocusretries.
fileDidChangecancels superseded in-flight compiler work via a realCompletableCancelToken, whereasdidFocusalways lets the (possibly now-irrelevant) computation finish before detecting staleness and retrying. WiringdidFocusinto the same in-flight-cancellation mechanism would avoid wasted presentation-compiler work during rapid focus/cancel churn, though this is a performance nicety rather than a correctness gap given the retry-once-then-Nilfallback already prevents publishing stale results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala` around lines 358 - 403, Optionally update didFocus to use the same in-flight cancellation mechanism as fileDidChange, replacing EmptyCancelToken with a real CompletableCancelToken tied to superseded focus computations. Cancel the prior didFocus work when a newer generation starts, while preserving the existing generation check, single retry, and Nil fallback behavior.metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala (1)
160-253: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider dedicated concurrency test coverage for
TurbineCompiler's generation tracking.This cohort adds
CompilersRaceLspSuite.scalato directly exercise the new generation-based staleness handling inCompilers.scala, but no analogous focused test targets the newcompileGeneration/compileLocklogic inTurbineCompiler. Given the subtlety of the double-checked-locking pattern here, a small unit test simulating overlappingcompileNow()/scheduleCompile()calls (with a controllableSleeper) would guard against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala` around lines 160 - 253, Add focused concurrency test coverage for TurbineCompiler’s compileGeneration and compileLock behavior, using a controllable Sleeper to overlap compileNow() and scheduleCompile() calls. Verify stale generations do not publish results while the current generation does, and preserve the existing compilation behavior for non-overlapping calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/src/test/scala/tests/p/ProtoPCJavaSuite.scala`:
- Around line 1451-1492: Update the delete-handling flow in onDelete so the
Java-compiler restart and subsequent diagnostics refresh are awaited before its
returned future completes. Then simplify proto-delete-invalidates-java by
removing server.waitFor(500) and the manual server.didFocus(java), asserting the
expected diagnostic directly after didChangeWatchedFiles completes.
---
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 1153-1164: Verify the concurrency contract of
mbt2.recompileTurbineClasspath() in refreshMbtStateAfterIndex: ensure generation
tracking prevents stale results and avoids duplicate work when a compile is
already running. If it does not, cancel the in-flight Turbine compilation before
starting the refresh, using the existing cancellation mechanism, while
preserving the subsequent compiler cancellation, diagnostics reset, and refresh
flow.
- Around line 1284-1309: Update onDelete to chain and await the proto-triggered
compilers.restartJavaCompilers() operation rather than invoking it
fire-and-forget, then invoke and await refreshAllDiagnostics() after the
restart, matching the didSave flow while preserving the existing behavior for
non-proto deletions.
---
Nitpick comments:
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 358-403: Optionally update didFocus to use the same in-flight
cancellation mechanism as fileDidChange, replacing EmptyCancelToken with a real
CompletableCancelToken tied to superseded focus computations. Cancel the prior
didFocus work when a newer generation starts, while preserving the existing
generation check, single retry, and Nil fallback behavior.
In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala`:
- Around line 160-253: Add focused concurrency test coverage for
TurbineCompiler’s compileGeneration and compileLock behavior, using a
controllable Sleeper to overlap compileNow() and scheduleCompile() calls. Verify
stale generations do not publish results while the current generation does, and
preserve the existing compilation behavior for non-overlapping calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df63659b-ba45-4160-bb1a-c49897576315
📒 Files selected for processing (8)
metals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalamtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scalatests/unit/src/test/scala/scala/meta/internal/jpc/JavaPruneCompilerConcurrencySuite.scalatests/unit/src/test/scala/scala/meta/internal/metals/CompilersRaceLspSuite.scalatests/unit/src/test/scala/tests/mbt/MbtBuildServerManualImportLspSuite.scalatests/unit/src/test/scala/tests/p/ProtoPCJavaSuite.scala
f5e9043 to
5ec1c86
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Fixes #8686
Summary by CodeRabbit
Bug Fixes
Tests