feature: Add annotation processor support for Java and Scala PCs - #42
sellophane wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
60fd783 to
87e9ab8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala (1)
88-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDetect
-processorpath=<path>before adding-proc:none.
processExtraOptionsaccepts-processorpath=<path>, buthasAnnotationProcessorsonly checks for the separate-argument form. When only the equals form is present,-proc:nonegets added and disables the specified annotation processor. Update both annotation-processor checks to also acceptoption.startsWith("-processorpath=").🤖 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 `@mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala` around lines 88 - 106, Update hasAnnotationProcessors in processExtraOptions to recognize options beginning with "-processorpath=" in addition to the existing separate-argument forms. Apply this check before adding "-proc:none" so an equals-form processor path preserves annotation processing.
🧹 Nitpick comments (2)
metals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scala (1)
33-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass a
DiagnosticListenerinstead ofnull.Both
getStandardFileManagerandgetTaskreceivenullfor the diagnostic listener. javac then writes all diagnostics toSystem.err. A batch compile of a whole build target commonly produces many errors, because the target may not compile in isolation. This pollutes the Metals process output and gives no way to log the failure reason.Supply a collecting listener and log a summary at debug level.
♻️ Proposed refactor
- val fileManager = compiler.getStandardFileManager(null, null, null) + val diagnostics = + new javax.tools.DiagnosticCollector[javax.tools.JavaFileObject]() + val fileManager = + compiler.getStandardFileManager(diagnostics, null, null) try { val units = fileManager.getJavaFileObjectsFromPaths(javaFiles.asJava) val options = buildOptions() val task = compiler.getTask( null, fileManager, - null, + diagnostics, options.asJava, null, units, ) - task.call() + val result: Boolean = task.call() + if (!result) { + scribe.debug( + s"[JavaAnnotationProcessorBatchCompiler] ${diagnostics.getDiagnostics().size()} diagnostics for $outputDir" + ) + } + result } finally {🤖 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/JavaAnnotationProcessorBatchCompiler.scala` around lines 33 - 49, Update the batch compilation flow around getStandardFileManager and getTask to create and pass a collecting DiagnosticListener instead of null. Accumulate compiler diagnostics without writing them to System.err, and after task.call() log a concise failure summary at debug level using the existing Metals logging facilities.metals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scala (1)
371-379: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate wording to reflect the new re-interrupt behavior.
stopThreadand the log message"thread stop (re-interrupt)"still describe forceful termination. The Runnable now only callsthread.interrupt()again. Rename the variable and the log message to avoid implying that the thread is actually being stopped.This method's fallback no longer guarantees termination of a runaway thread. See the consolidated comment on this and
mtags/src/main/scala-2/scala/meta/internal/pc/ScalaCompilerAccess.scalafor the reliability implication.✏️ Proposed wording fix
- // Last resort: re-interrupt the thread (Thread.stop() was removed in Java 20+). - val stopThread = new Runnable { + // Last resort: re-interrupt the thread (Thread.stop() was removed in Java 20+). + val reinterruptThread = new Runnable { def run(): Unit = { if (thread.isAlive()) { - scribe.warn(s"thread stop (re-interrupt): ${thread.getName()}") + scribe.warn(s"thread re-interrupt: ${thread.getName()}") thread.interrupt() } } } new Cancelable { def cancel() = if (thread.isAlive()) { result.complete(None) - threadStopper.schedule(stopThread, 3, TimeUnit.SECONDS) + threadStopper.schedule(reinterruptThread, 3, TimeUnit.SECONDS)🤖 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/worksheets/WorksheetProvider.scala` around lines 371 - 379, Rename the Runnable variable stopThread to reflect that it only re-interrupts the thread, and update the scribe.warn message to remove “thread stop” wording while retaining the re-interrupt context. Keep the existing thread.isAlive check and thread.interrupt() behavior unchanged.
🤖 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/CompilerConfiguration.scala`:
- Around line 390-413: The annotation processor output directory is attached
under narrower conditions than those used by
Compilers.triggerAnnotationProcessorBatchCompilation. In
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
lines 390-413, replace the isMbtTarget split with a single condition combining
BSP processor options and enabled MBT processors, and remove
hasMbtFallbackProcessors. In lines 480-505, update the relevant compiler
classpath construction to add annotationProcessorOutputDir(buildTargetId)
whenever processorOpts is non-empty, while keeping -proc:none restricted to the
MBT-managed case.
In `@metals/src/main/scala/scala/meta/internal/metals/Compilers.scala`:
- Around line 1595-1602: The scaladoc for the annotation processor batch
compilation method promises it runs at most once per session, but the cancel()
method clears the annotationProcessorBatchStarted flag, causing it to rerun
after each sync or import. Either update the scaladoc comment to accurately
reflect that the batch compilation can run multiple times per session (once per
sync/import cycle), or modify the cancel() method to preserve the
annotationProcessorBatchStarted flag across invocations to honor the
one-per-session guarantee described in the documentation.
- Around line 1659-1670: The failure path in the batch compilation block removes
targetId from annotationProcessorBatchStarted unconditionally, allowing
immediate retry on the next lookup with no limits or backoff, which blocks
threads in the shared executor ec indefinitely for targets that fail
compilation. Track failures in annotationProcessorBatchStarted or a separate
tracking map to enforce bounded retry attempts with backoff between attempts,
preventing the same compilation from running repeatedly. Additionally, migrate
the entire Future block away from the shared ec to a dedicated executor that can
respect shutdown timeouts and cancellation.
- Around line 1606-1648: Move the annotationProcessorBatchStarted.add(targetId)
atomic winner check to the start of triggerAnnotationProcessorBatchCompilation,
before source, classpath, and processor setup. If the target has no Java sources
or effective processor options, remove targetId from the set before returning so
a later lookup can retry; retain the existing guard’s winner-only behavior for
concurrent callers.
In
`@metals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scala`:
- Around line 21-32: The compile method reuses outputDir across sessions without
clearing it first, causing stale generated class files from previous
compilations to remain when source files are deleted or renamed. Before calling
Files.createDirectories(outputDir) in the compile method, remove any existing
contents from outputDir to ensure only the current compilation's generated
artifacts are present. Apply the same cleanup to the second compilation path
referenced in lines 58-68 to maintain consistency across all compilation flows.
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelMbtImporter.scala`:
- Around line 258-300: Update queryJavaPlugins and applyPluginInfo so each
java_plugin contributes its complete dependency closure, not only direct
dependency labels, to processorPath metadata. Resolve transitive plugin
dependencies through the existing Bazel query/import path and attach processor
information to every corresponding module, including helper JARs. Add a
regression covering a processor that depends on a direct helper JAR and verifies
both modules are available to annotation processing.
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/MbtWorkspaceSymbolProvider.scala`:
- Around line 163-166: Update the projectStubsDir argument in
MbtWorkspaceSymbolProvider to use CompilerConfiguration.mbtOutDir instead of
resolving workspace/.metals/mbt-out, so MBT stubs are written to the same
directory Java batch annotation processing adds to the classpath.
In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala`:
- Around line 215-231: Update writeStubs to ensure projectStubsDir contains only
class files from the current compilation, removing stale outputs when classes
are deleted or renamed. Prefer writing the current lowered output into a fresh
temporary directory and atomically replacing or swapping the existing stub
directory; otherwise track prior binary names and delete files no longer
present. Preserve onDone invocation and existing failure handling.
- Around line 313-345: Update listSourcepath in TurbineCompiler so workspace and
combined SOURCE_PATH entries are deduplicated using the same canonical
original-URI/file-identity representation as WorkspaceSourcepath, rather than
raw JavaFileObject.getName() values. Preserve the existing ordering and merging
behavior while ensuring equivalent SourceJavaFileObject and VirtualTextDocument
entries are emitted only once.
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala`:
- Around line 1214-1236: Update processorOptions to match the javac option
grammar accepted by MbtAnnotationProcessingOptions.parseJavacOptions: recognize
split and inline -processor-path/--processor-path and -processor forms,
including “=” and “:” separators, and preserve -proc:none. Keep forwarding -A
options and existing two-argument handling intact while ensuring all supported
processor options reach JavaAnnotationProcessorBatchCompiler or compiler
configuration.
In `@metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala`:
- Around line 351-354: Update the onAnnotationProcessorStubsReady callback to
restart only the loaded presentation compilers or otherwise deduplicate targets
before invoking compilers.restartPresentationCompilers, avoiding a full
mbtBuild.mbtTargets traversal and repeated inverse-dependency walks on each
turbine recompilation.
In `@mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala`:
- Around line 323-347: Update the sourcepath listing loop in the package-name
handling around extractPackageName so fileManager.list is called for every
parameter, including when packageName is empty. Remove the packageName.nonEmpty
guard while preserving the existing filtering, deduplication, and NonFatal
warning behavior.
---
Outside diff comments:
In `@mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala`:
- Around line 88-106: Update hasAnnotationProcessors in processExtraOptions to
recognize options beginning with "-processorpath=" in addition to the existing
separate-argument forms. Apply this check before adding "-proc:none" so an
equals-form processor path preserves annotation processing.
---
Nitpick comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scala`:
- Around line 33-49: Update the batch compilation flow around
getStandardFileManager and getTask to create and pass a collecting
DiagnosticListener instead of null. Accumulate compiler diagnostics without
writing them to System.err, and after task.call() log a concise failure summary
at debug level using the existing Metals logging facilities.
In
`@metals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scala`:
- Around line 371-379: Rename the Runnable variable stopThread to reflect that
it only re-interrupts the thread, and update the scribe.warn message to remove
“thread stop” wording while retaining the re-interrupt context. Keep the
existing thread.isAlive check and thread.interrupt() behavior unchanged.
🪄 Autofix
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: 80baa717-d191-43ff-9e50-7d700ea2c32f
📒 Files selected for processing (20)
java-header-compiler/src/main/java/scala/meta/internal/headercompilers/MetalsJavaHeaderCompilerPlugin.javametals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scalametals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/mbt/MbtAnnotationProcessingOptions.scalametals/src/main/scala/scala/meta/internal/metals/mbt/MbtWorkspaceSymbolProvider.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineClasspathFileManager.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelMbtBuildSupport.scalametals/src/main/scala/scala/meta/internal/metals/mbt/importer/BazelMbtImporter.scalametals/src/main/scala/scala/meta/internal/worksheets/WorksheetProvider.scalamtags-java/src/main/scala/scala/meta/internal/jpc/JavaCompletionProvider.scalamtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scalamtags-java/src/main/scala/scala/meta/internal/jpc/JavaSourceCompile.scalamtags/src/main/scala-2/scala/meta/internal/pc/ScalaCompilerAccess.scalatests/javapc/src/main/scala/tests/pc/BaseJavaPCSuite.scalatests/javapc/src/test/scala/pc/CompletionAnnotationProcessorSuite.scalatests/unit/src/test/scala/tests/mbt/BazelMavenJsonImporterSuite.scala
| val annotationProcessorClasspath: Seq[Path] = { | ||
| val isMbtTarget = mbtBuild().mbtTargets.exists(_.id == scalaTarget.id) | ||
| if (isMbtTarget) { | ||
| val hasMbtProcessors = mbtBuild().mbtTargets | ||
| .find(_.id == scalaTarget.id) | ||
| .exists(t => MbtAnnotationProcessingOptions.fromTarget(t).isEnabled) | ||
| if (hasMbtProcessors) | ||
| Seq(annotationProcessorOutputDir(scalaTarget.id)) | ||
| else Nil | ||
| } else { | ||
| val hasBspProcessors = buildTargets | ||
| .javaTarget(scalaTarget.id) | ||
| .exists(_.javac.processorOptions.nonEmpty) | ||
| val hasMbtFallbackProcessors = | ||
| !hasBspProcessors && mbtBuild().mbtTargets | ||
| .find(_.id == scalaTarget.id) | ||
| .exists(t => | ||
| MbtAnnotationProcessingOptions.fromTarget(t).isEnabled | ||
| ) | ||
| if (hasBspProcessors || hasMbtFallbackProcessors) | ||
| Seq(annotationProcessorOutputDir(scalaTarget.id)) | ||
| else Nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate annotationProcessorOutputDir on the same condition that triggers the batch compilation. Compilers.triggerAnnotationProcessorBatchCompilation writes to annotationProcessorOutputDir(targetId) when the target has either BSP javac.processorOptions or enabled MBT processors. Both compiler constructors attach that directory under narrower conditions, so batch-compiled output can exist without reaching the compiler classpath.
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala#L390-L413: replace theisMbtTargetsplit with a singlehasBspProcessors || hasMbtProcessorscondition, and drop the unreachablehasMbtFallbackProcessorsvalue.metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala#L480-L505: addannotationProcessorOutputDir(buildTargetId)to the classpath whenprocessorOptsis non-empty as well, and keep-proc:nonelimited to the MBT-managed case.
📍 Affects 1 file
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala#L390-L413(this comment)metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala#L480-L505
🤖 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/CompilerConfiguration.scala`
around lines 390 - 413, The annotation processor output directory is attached
under narrower conditions than those used by
Compilers.triggerAnnotationProcessorBatchCompilation. In
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
lines 390-413, replace the isMbtTarget split with a single condition combining
BSP processor options and enabled MBT processors, and remove
hasMbtFallbackProcessors. In lines 480-505, update the relevant compiler
classpath construction to add annotationProcessorOutputDir(buildTargetId)
whenever processorOpts is non-empty, while keeping -proc:none restricted to the
MBT-managed case.
| /** | ||
| * If this build target has Java annotation processors, batch-compile all Java | ||
| * sources with those processors and write class files to the dedicated ap-cp | ||
| * directory. Once done, restart the Scala presentation compiler for the target | ||
| * so it picks up the AP-generated symbols. | ||
| * | ||
| * Guaranteed to run at most once per build target per Metals session. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The scaladoc contradicts cancel().
The doc states the compilation is "Guaranteed to run at most once per build target per Metals session". cancel() clears annotationProcessorBatchStarted at Line 299. The comment at Line 286 states that cancel runs after every sync and import while the Compilers instance is reused. The batch compilation therefore reruns after each sync or import.
Correct the doc, or keep the set across cancel() if a single run per session is the intent.
🤖 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 1595 - 1602, The scaladoc for the annotation processor batch compilation
method promises it runs at most once per session, but the cancel() method clears
the annotationProcessorBatchStarted flag, causing it to rerun after each sync or
import. Either update the scaladoc comment to accurately reflect that the batch
compilation can run multiple times per session (once per sync/import cycle), or
modify the cancel() method to preserve the annotationProcessorBatchStarted flag
across invocations to honor the one-per-session guarantee described in the
documentation.
| val javaTargetOpt = buildTargets.javaTarget(targetId) | ||
| val processorOpts = | ||
| javaTargetOpt.map(_.javac.processorOptions).getOrElse(Nil) | ||
|
|
||
| val javaFiles = buildTargets | ||
| .buildTargetSources(targetId) | ||
| .filter(_.isJavaFilename) | ||
| .map(_.toNIO) | ||
| .toSeq | ||
| if (javaFiles.isEmpty) return | ||
|
|
||
| val mbtOutDir = compilerConfiguration.mbtOutDir | ||
| val isMbtTarget = mbtBuild().mbtTargets.exists(_.id == targetId) | ||
| val classpath = javaTargetOpt | ||
| .flatMap(_.classpath) | ||
| .getOrElse(Nil) | ||
| .map(_.toAbsolutePath.toNIO) | ||
| .filter(java.nio.file.Files.exists(_)) ++ | ||
| (if (isMbtTarget && java.nio.file.Files.isDirectory(mbtOutDir)) | ||
| Seq(mbtOutDir) | ||
| else Nil) | ||
|
|
||
| val effectiveProcessorOpts = | ||
| if (processorOpts.nonEmpty) processorOpts | ||
| else | ||
| mbtBuild().mbtTargets | ||
| .find(_.id == targetId) | ||
| .map(MbtAnnotationProcessingOptions.fromTarget) | ||
| .filter(_.isEnabled) | ||
| .map { opts => | ||
| List( | ||
| "-processorpath", | ||
| (opts.processorPath.map(_.toString) ++ classpath.map(_.toString)) | ||
| .mkString(java.io.File.pathSeparator), | ||
| "-processor", | ||
| opts.processors.mkString(","), | ||
| ) | ||
| } | ||
| .getOrElse(Nil) | ||
|
|
||
| if (effectiveProcessorOpts.isEmpty) return | ||
|
|
||
| if (!annotationProcessorBatchStarted.add(targetId)) return |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Move the deduplication guard before the expensive setup work.
triggerAnnotationProcessorBatchCompilation runs synchronously on the caller thread. loadJavaCompiler and withKeyAndDefault call it on every presentation compiler lookup, which includes completion, hover, and diagnostics requests. Before reaching the annotationProcessorBatchStarted.add(targetId) guard at Line 1648, the method walks buildTargetSources(targetId), and calls Files.exists for every classpath entry at Line 1623. This performs filesystem I/O on the request path for every lookup, even after the batch compilation already ran.
Check the guard first, and roll it back if the target turns out to have no sources or no processors.
♻️ Proposed restructure
private def triggerAnnotationProcessorBatchCompilation(
targetId: BuildTargetIdentifier
): Unit = {
+ if (annotationProcessorBatchStarted.contains(targetId)) return
val javaTargetOpt = buildTargets.javaTarget(targetId)Keep the existing add call as the atomic winner check so concurrent callers still run the batch once.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val javaTargetOpt = buildTargets.javaTarget(targetId) | |
| val processorOpts = | |
| javaTargetOpt.map(_.javac.processorOptions).getOrElse(Nil) | |
| val javaFiles = buildTargets | |
| .buildTargetSources(targetId) | |
| .filter(_.isJavaFilename) | |
| .map(_.toNIO) | |
| .toSeq | |
| if (javaFiles.isEmpty) return | |
| val mbtOutDir = compilerConfiguration.mbtOutDir | |
| val isMbtTarget = mbtBuild().mbtTargets.exists(_.id == targetId) | |
| val classpath = javaTargetOpt | |
| .flatMap(_.classpath) | |
| .getOrElse(Nil) | |
| .map(_.toAbsolutePath.toNIO) | |
| .filter(java.nio.file.Files.exists(_)) ++ | |
| (if (isMbtTarget && java.nio.file.Files.isDirectory(mbtOutDir)) | |
| Seq(mbtOutDir) | |
| else Nil) | |
| val effectiveProcessorOpts = | |
| if (processorOpts.nonEmpty) processorOpts | |
| else | |
| mbtBuild().mbtTargets | |
| .find(_.id == targetId) | |
| .map(MbtAnnotationProcessingOptions.fromTarget) | |
| .filter(_.isEnabled) | |
| .map { opts => | |
| List( | |
| "-processorpath", | |
| (opts.processorPath.map(_.toString) ++ classpath.map(_.toString)) | |
| .mkString(java.io.File.pathSeparator), | |
| "-processor", | |
| opts.processors.mkString(","), | |
| ) | |
| } | |
| .getOrElse(Nil) | |
| if (effectiveProcessorOpts.isEmpty) return | |
| if (!annotationProcessorBatchStarted.add(targetId)) return | |
| if (annotationProcessorBatchStarted.contains(targetId)) return | |
| val javaTargetOpt = buildTargets.javaTarget(targetId) | |
| val processorOpts = | |
| javaTargetOpt.map(_.javac.processorOptions).getOrElse(Nil) | |
| val javaFiles = buildTargets | |
| .buildTargetSources(targetId) | |
| .filter(_.isJavaFilename) | |
| .map(_.toNIO) | |
| .toSeq | |
| if (javaFiles.isEmpty) return | |
| val mbtOutDir = compilerConfiguration.mbtOutDir | |
| val isMbtTarget = mbtBuild().mbtTargets.exists(_.id == targetId) | |
| val classpath = javaTargetOpt | |
| .flatMap(_.classpath) | |
| .getOrElse(Nil) | |
| .map(_.toAbsolutePath.toNIO) | |
| .filter(java.nio.file.Files.exists(_)) ++ | |
| (if (isMbtTarget && java.nio.file.Files.isDirectory(mbtOutDir)) | |
| Seq(mbtOutDir) | |
| else Nil) | |
| val effectiveProcessorOpts = | |
| if (processorOpts.nonEmpty) processorOpts | |
| else | |
| mbtBuild().mbtTargets | |
| .find(_.id == targetId) | |
| .map(MbtAnnotationProcessingOptions.fromTarget) | |
| .filter(_.isEnabled) | |
| .map { opts => | |
| List( | |
| "-processorpath", | |
| (opts.processorPath.map(_.toString) ++ classpath.map(_.toString)) | |
| .mkString(java.io.File.pathSeparator), | |
| "-processor", | |
| opts.processors.mkString(","), | |
| ) | |
| } | |
| .getOrElse(Nil) | |
| if (effectiveProcessorOpts.isEmpty) return | |
| if (!annotationProcessorBatchStarted.add(targetId)) return |
🤖 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 1606 - 1648, Move the annotationProcessorBatchStarted.add(targetId) atomic
winner check to the start of triggerAnnotationProcessorBatchCompilation, before
source, classpath, and processor setup. If the target has no Java sources or
effective processor options, remove targetId from the set before returning so a
later lookup can retry; retain the existing guard’s winner-only behavior for
concurrent callers.
| Future { | ||
| val success = batchCompiler.compile(javaFiles) | ||
| if (success) { | ||
| cache | ||
| .get(PresentationCompilerKey.JavaBuildTarget(targetId)) | ||
| .foreach(_.await.restart()) | ||
| restartPresentationCompilers(targetId) | ||
| } else { | ||
| annotationProcessorBatchStarted.remove(targetId) | ||
| } | ||
| }(ec) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The failure path retries without any limit or backoff.
On failure the code removes targetId from annotationProcessorBatchStarted. The next presentation compiler lookup for that target calls this method again and starts another batch compilation. A target that never compiles successfully therefore triggers a full javac run on each lookup. Each run also blocks a thread of the shared ec for the whole compilation, with no timeout and no cancellation on shutdown.
Record the failure and stop retrying, or apply a bounded retry with backoff, and run the compilation on a dedicated executor.
🤖 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 1659 - 1670, The failure path in the batch compilation block removes
targetId from annotationProcessorBatchStarted unconditionally, allowing
immediate retry on the next lookup with no limits or backoff, which blocks
threads in the shared executor ec indefinitely for targets that fail
compilation. Track failures in annotationProcessorBatchStarted or a separate
tracking map to enforce bounded retry attempts with backoff between attempts,
preventing the same compilation from running repeatedly. Additionally, migrate
the entire Future block away from the shared ec to a dedicated executor that can
respect shutdown timeouts and cancellation.
| def compile(javaFiles: Seq[Path]): Boolean = | ||
| if (javaFiles.isEmpty) false | ||
| else | ||
| try { | ||
| Files.createDirectories(outputDir) | ||
| val compiler = ToolProvider.getSystemJavaCompiler() | ||
| if (compiler == null) { | ||
| scribe.warn( | ||
| "[JavaAnnotationProcessorBatchCompiler] no system Java compiler available (running on JRE?)" | ||
| ) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
outputDir is reused without cleanup.
compile calls Files.createDirectories(outputDir) and then lets javac write into the existing directory. The directory is stable per build target across sessions, as CompilerConfiguration.annotationProcessorOutputDir hashes the target URI. Class files from a previous run stay in place when the corresponding source is deleted or renamed, and the presentation compilers receive this directory on their classpath. Stale generated symbols then resolve.
Clear outputDir before the compilation, or write to a new directory and swap it.
Also applies to: 58-68
🤖 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/JavaAnnotationProcessorBatchCompiler.scala`
around lines 21 - 32, The compile method reuses outputDir across sessions
without clearing it first, causing stale generated class files from previous
compilations to remain when source files are deleted or renamed. Before calling
Files.createDirectories(outputDir) in the compile method, remove any existing
contents from outputDir to ensure only the current compilation's generated
artifacts are present. Apply the same cleanup to the second compilation path
referenced in lines 58-68 to maintain consistency across all compilation flows.
| private def writeStubs(dir: Path, onDone: () => Unit = () => ()): Unit = | ||
| try { | ||
| Files.createDirectories(dir) | ||
| for { | ||
| (_, symbols) <- result.symbolsByPackage | ||
| sym <- symbols.asScala | ||
| bytes <- Option(result.lowered.bytes().get(sym.binaryName())) | ||
| } { | ||
| val classFile = dir.resolve(sym.binaryName() + ".class") | ||
| Files.createDirectories(classFile.getParent) | ||
| Files.write(classFile, bytes) | ||
| } | ||
| onDone() | ||
| } catch { | ||
| case NonFatal(e) => | ||
| scribe.warn(s"[TurbineCompiler] failed to write stubs: ${e.getMessage}") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
writeStubs never removes stale class files.
writeStubs only writes the class files present in the current result. It does not delete files from a previous compilation. If a class is deleted or renamed, its .class file stays in the stub directory. TurbineClasspathFileManager lists directory classpath entries by scanning .class files, so javac and the annotation-processor batch compiler continue to resolve the removed class. The directory also grows without bound across sessions.
The write is also unconditional for projectStubsDir on every recompile, so the full lowered output is written to disk each time.
Consider writing into a fresh temporary directory and then swapping it, or tracking and deleting the binary names that are no longer present.
🤖 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 215 - 231, Update writeStubs to ensure projectStubsDir contains
only class files from the current compilation, removing stale outputs when
classes are deleted or renamed. Prefer writing the current lowered output into a
fresh temporary directory and atomically replacing or swapping the existing stub
directory; otherwise track prior binary names and delete files no longer
present. Preserve onDone invocation and existing failure handling.
| workspaceSourcepath: String => java.lang.Iterable[JavaFileObject] = _ => | ||
| ju.Collections.emptyList(), | ||
| ): JavaFileManager = { | ||
| val isGlobalClasspathEntry = this.classpath().toSet | ||
| val filteredProjectClasspath = | ||
| projectClasspathJars.asScala.filter(file => | ||
| !isGlobalClasspathEntry(file) && TurbineCompiler.isJarFile(file) | ||
| ) | ||
| val entries = projectClasspathJars.asScala | ||
| val filteredProjectClasspath = entries.filter(TurbineCompiler.isJarFile) | ||
| val directoryClasspaths = entries.filter(Files.isDirectory(_)).toSeq | ||
| val projectClasspath = | ||
| ClassPathBinder.bindClasspath(filteredProjectClasspath.asJava) | ||
| onNewProjectClasspath(projectClasspath) | ||
| new TurbineClasspathFileManager( | ||
| underlying, | ||
| () => result, | ||
| listSourcepath = listCombinedSourcepath, | ||
| listSourcepath = (packageName: String) => { | ||
| val workspaceFiles = workspaceSourcepath(packageName) | ||
| val workspaceIt = workspaceFiles.iterator() | ||
| if (!workspaceIt.hasNext()) { | ||
| listCombinedSourcepath(packageName) | ||
| } else { | ||
| val seenNames = new ju.HashSet[String]() | ||
| val combined = new ju.ArrayList[JavaFileObject]() | ||
| listCombinedSourcepath(packageName).forEach { obj => | ||
| if (seenNames.add(obj.getName())) combined.add(obj) | ||
| } | ||
| workspaceIt.forEachRemaining { obj => | ||
| if (seenNames.add(obj.getName())) combined.add(obj) | ||
| } | ||
| combined | ||
| } | ||
| }, | ||
| isDeleted, | ||
| projectClasspath, | ||
| directoryClasspaths, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e scala 'SemanticdbCompilationUnit|SourceJavaFileObject|SourcepathJavaFileObject' | xargs -r rg -n -C6 'def getName|getName\(\)|class SemanticdbCompilationUnit|case class SourcepathJavaFileObject'
rg -n --type=scala -C6 'def toSemanticdbCompilationUnit'Repository: tgodzik/metals
Length of output: 1466
🏁 Script executed:
#!/bin/bash
set -u
echo "## files matching relevant names"
git ls-files '*.scala' | rg -i 'semanticdb|sourcepath|turbinecompiler|document|indexed' || true
echo
echo "## searches"
rg -n --type=scala -C5 'SemanticdbCompilationUnit|SourcepathJavaFileObject|toSemanticdbCompilationUnit|JavaFileObject\(uri|class .*FileObject|object.*FileObject|getName\(\)' metals/src/main/scala || trueRepository: tgodzik/metals
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "## candidate files"
git ls-files '*.scala' | rg 'metals/src/main/scala/scala/meta/internal/metals/mbt/(IndexedDocument|SourcepathJavaFileObject|SourceJavaFileObject|TurbineCompiler|TurbineClasspathFileManager|ClassPathBinder|JavacSourcepathFileManager)'
echo
echo "## relevant file outlines"
for f in \
metals/src/main/scala/scala/meta/internal/metals/mbt/IndexedDocument.scala \
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala \
metals/src/main/scala/scala/meta/internal/metals/mbt/JavacSourcepathFileManager.scala \
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineClasspathFileManager.scala \
metals/src/main/scala/scala/meta/internal/metals/mbt/ClassPathBinder.scala \
metals/src/main/scala/scala/meta/internal/metals/mbt/SourcepathJavaFileObject.scala
do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" | sed -n '1,220p' || true
fi
done
echo
echo "## targeted definitions/calls"
rg -n --type=scala -C4 'SemanticdbCompilationUnit|SemanticdbCompilationUnit|SourcepathJavaFileObject|toSemanticdbCompilationUnit|workspaceSourcepath|SOURCE_PATH|listCombinedSourcepath|getName\(\)' \
metals/src/main/scala/scala/meta/internal/metals/mbt \
mtags/src/main/scala/scala/tools/nsc 2>/dev/null || trueRepository: tgodzik/metals
Length of output: 45125
🏁 Script executed:
#!/bin/bash
set -u
echo "## IndexedDocument.scala relevant section"
sed -n '1,130p' metals/src/main/scala/scala/meta/internal/metals/mbt/IndexedDocument.scala | cat -n
echo
echo "## VirtualTextDocument.scala"
sed -n '1,80p' metals/src/main/scala/scala/meta/internal/metals/mbt/VirtualTextDocument.scala | cat -n
echo
echo "## find JavaSource class declarations"
git ls-files '*.scala' | rg 'SourceJavaFileObject|SemanticdbCompilationUnit|VirtualTextDocument' | while read -r f; do
echo "--- $f"
rg -n -C5 'class .*SourceJavaFileObject|def binaryName|override def getName|getName\(\)|class VirtualTextDocument|extends SimpleJavaFileObject|case class' "$f" || true
doneRepository: tgodzik/metals
Length of output: 10789
Ensure sourcepath getName() uses the same canonical form.
WorkspaceSourcepath deduplicates incoming entries by SourceJavaFileObject.getName(), but combined SOURCE_PATH listing deduplicates by JavaFileObject.getName(). Since SourceJavaFileObject returns originaluri-... while VirtualTextDocument returns uri.toString, same-file entries can be treated as distinct after the merge and be passed to javac twice. Normalize both paths to the original URI/value before merging or deduplicate by file identity instead of this method.
🤖 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 313 - 345, Update listSourcepath in TurbineCompiler so workspace
and combined SOURCE_PATH entries are deduplicated using the same canonical
original-URI/file-identity representation as WorkspaceSourcepath, rather than
raw JavaFileObject.getName() values. Preserve the existing ordering and merging
behavior while ensuring equivalent SourceJavaFileObject and VirtualTextDocument
entries are emitted only once.
| def processorOptions: List[String] = { | ||
| val twoArgFlags = Set("-processorpath", "-processor") | ||
| val opts = item.getOptions.asScala.toList | ||
|
|
||
| @scala.annotation.tailrec | ||
| def loop(remaining: List[String], acc: List[String]): List[String] = | ||
| remaining match { | ||
| case Nil => acc.reverse | ||
| case flag :: value :: rest if twoArgFlags.contains(flag) => | ||
| loop(rest, value :: flag :: acc) | ||
| case opt :: rest if opt.startsWith("--add-opens") => | ||
| if (opt.contains("=")) loop(rest, opt :: acc) | ||
| else | ||
| rest match { | ||
| case value :: rest2 => loop(rest2, value :: opt :: acc) | ||
| case Nil => acc.reverse | ||
| } | ||
| case opt :: rest if opt.startsWith("-A") => | ||
| loop(rest, opt :: acc) | ||
| case _ :: rest => loop(rest, acc) | ||
| } | ||
|
|
||
| loop(opts, Nil) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 --glob '*.scala' \
'def processorOptions|def parseJavacOptions|--processor-path|-processorpath=|-processorpath:|-processor=|-proc:none|processorOpts' \
metalsRepository: tgodzik/metals
Length of output: 14878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- MetalsEnrichments processorOptions excerpt ---\n'
sed -n '1208,1242p' metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala | cat -n
printf '\n--- MbtAnnotationProcessingOptions parseJavacOptions excerpt ---\n'
sed -n '66,106p' metals/src/main/scala/scala/meta/internal/metals/mbt/MbtAnnotationProcessingOptions.scala | cat -n
printf '\n--- Call sites and local behavior tests ---\n'
python3 - <<'PY'
from pathlib import Path
import re
metals_path = Path('metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala')
txt = metals_path.read_text()
start = txt.index(' def processorOptions: List[String] = {')
end = txt.index('\n }', start) + 7
block = txt[start:end]
print('uses_two_arg_minus_processorpath=', '-processorpath' in block)
print('uses_two_arg_minus_processor=', '-processor' in block or '-processor"' in block)
print('mentions_dash_proc_none=', '-proc:none' in block)
print('mentions_processor_path_split=', '--processor-path' in block or '-processorpath:' in block or '-processorpath=' in block)
print('contains_ignored_option_pattern=', bool(re.search(r'case\s+opt\s*::\s*rest\s*=>\s*loop\(rest,\s*acc\)', block)))
def loop(opts):
twoArgFlags = {"-processorpath", "-processor"}
def impl(remaining, acc):
while True:
if remaining == []:
return acc[::-1]
if len(remaining) >= 2 and remaining[0] in twoArgFlags:
remaining = remaining[2:]
acc = [remaining[1], remaining[0], remaining[2]] if len(remaining) >= 2 else [remaining[1], remaining[0]]
acc = remaining[2:] + acc if len(remaining) >= 2 else []
return acc[::-1] # fake continuation, not needed
# ... hard enough; use direct pattern for skipped options from next
break
PY
printf '\n--- Search for tests covering processorOptions ---\n'
rg -n --glob '*.scala' 'processorOptions|parseJavacOptions|-proc:none|processor-path|processorpath|processor=' test metals/src/test || trueRepository: tgodzik/metals
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Java compiler javac processor option forms (javadoc excerpt from JDK source doc URL unavailable; print local references only) ---\n'
rg -n --glob '*.scala' 'processOptionFile|Options|javacOptions|processorOptions' metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala metals/src/main/scala/scala/meta/internal/metals/Compilers.scala metals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scala metals/src/main/scala/scala/meta/internal/metals/mbt/MbtAnnotationProcessingOptions.scalaRepository: tgodzik/metals
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- MetalsEnrichments processorOptions excerpt ---'
sed -n '1208,1242p' metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala | cat -n
printf '%s\n' ''
printf '%s\n' '--- MbtAnnotationProcessingOptions parseJavacOptions excerpt ---'
sed -n '66,106p' metals/src/main/scala/scala/meta/internal/metals/mbt/MbtAnnotationProcessingOptions.scala | cat -n
printf '%s\n' ''
printf '%s\n' '--- Static behavior probe ---'
python3 - <<'PY'
from pathlib import Path
import re
metals = Path('metals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scala').read_text()
start = metals.index(' def processorOptions: List[String] = {')
end = metals.index('\n }', start) + 7
block = metals[start:end]
mbt = Path('metals/src/main/scala/scala/meta/internal/metals/mbt/MbtAnnotationProcessingOptions.scala').read_text()
mbt_start = mbt.index(' private def parseJavacOptions(')
mbt_end = mbt.index(' case _ +: tail =>', mbt_start) + len(' case _ +: tail =>\n') + 20
def emulate_processor_options(opts):
twoArgFlags = {"-processorpath", "-processor"}
acc = []
def loop(remaining):
nonlocal acc
if remaining == []:
return acc[::-1]
if len(remaining) >= 2 and remaining[0] in twoArgFlags:
acc.append(remaining[1])
acc.append(remaining[0])
return loop(remaining[2:])
if remaining[0].startswith("--add-opens"):
if "=" in remaining[0]:
acc.append(remaining[0])
return loop(remaining[1:])
elif len(remaining) >= 2:
acc.append(remaining[1])
acc.append(remaining[0])
return loop(remaining[2:])
else:
return loop([])
if remaining[0].startswith("-A"):
acc.append(remaining[0])
return loop(remaining[1:])
acc.append(remaining[0])
return loop(remaining[1:])
return loop(opts)
cases = {
"processorpath-separate": ["-processorpath", "a.b"],
"processorpath-equals": ["-processorpath=a.b"],
"processorpath-colon": ["-processorpath:a/b"],
"processor-path-separated": ["--processor-path", "a/b"],
"processor-path-equals": ["--processor-path=a/b"],
"processor-separated": ["-processor", "ProcessorClass"],
"processor-equals": ["-processor=ProcessorClass"],
"proc-none": ["-proc:none"],
"add-opens-separated": ["--add-opens", "java.base/java.lang"],
"add-opens-equals": ["--add-opens=java.base/java.lang"],
"proc-none-before-processorpath": ["-proc:none", "-processorpath", "x"],
"processorpath-before-processor-equals-processorNone": ["-processorpath", "x", "-processor=ProcessorClass", "-proc:none"],
}
for name, opts in cases.items():
print(f"{name}={emulate_processor_options(opts)}")
print(re.search(r'case\s+opt\s*::~~>*\s*rest\s*=>\s*loop\(rest,\s*acc\)', re.sub(r'_', '_', block)))
print(block.find('case _ :: rest => loop(rest, acc)'))
print()
print('metals_has_two_arg_processorpath=', '-processorpath' in block)
print('metals_has_two_arg_processor=', bool(re.search(r'twoArgFlags\s*=\s*Set\([^)]*-processor[^)]*\)', block)))
print('metals_has_proc_none_case=', '-proc:none' in block)
print('metals_has_processorpath_equals_case=', bool(re.search(r'-processorpath=', block)))
print('metals_has_processorpath_colon_case=', bool(re.search(r'-processorpath:', block)))
print('metals_has_processor_path_cases=', 'processor-path' in block)
print()
print('mbt_proc_none_present=', 'case "-proc:none" +: tail =>' in mbt)
print('mbt_processor_path_cases_present=', all(s in mbt for s in [
'case ("-processorpath" | "--processor-path") +: value +: tail =>',
'case option +: tail if option.startsWith("-processorpath=") =>',
'case option +: tail if option.startsWith("--processor-path=") =>',
'case option +: tail if option.startsWith("-processorpath:") =>',
]))
print('mbt_processor_equals_present=', '-processor=' in mbt)
PY
printf '%s\n' ''
printf '%s\n' '--- JavaProcessorBatchCompiler and call sites excerpts ---'
sed -n '56,68p' metals/src/main/scala/scala/meta/internal/metals/JavaAnnotationProcessorBatchCompiler.scala | cat -n
sed -n '1603,1633p' metals/src/main/scala/scala/meta/internal/metals/Compilers.scala | cat -n
sed -n '483,506p' metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala | cat -nRepository: tgodzik/metals
Length of output: 3691
Preserve the processor-option grammar.
processorOptions drops the split --processor-path, processor -processor=..., processor-path -processorpath=... / -processorpath:..., and -proc:none forms that MbtAnnotationProcessingOptions.parseJavacOptions supports. Since processor options are forwarded to JavaAnnotationProcessorBatchCompiler or compiler configuration, a BSP target using any of these forms can lose its processor path or disable option; use the same accepted javac option grammar in both parsers.
🤖 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/MetalsEnrichments.scala`
around lines 1214 - 1236, Update processorOptions to match the javac option
grammar accepted by MbtAnnotationProcessingOptions.parseJavacOptions: recognize
split and inline -processor-path/--processor-path and -processor forms,
including “=” and “:” separators, and preserve -proc:none. Keep forwarding -A
options and existing two-argument handling intact while ensuring all supported
processor options reach JavaAnnotationProcessorBatchCompiler or compiler
configuration.
| onAnnotationProcessorStubsReady = () => | ||
| mbtBuild.mbtTargets.foreach(t => | ||
| compilers.restartPresentationCompilers(t.id) | ||
| ), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The callback restarts every presentation compiler on each turbine recompile.
TurbineCompiler.doCompileNow invokes onAnnotationProcessorStubsReady after each recompilation when annotation processing is enabled. restartPresentationCompilers already walks allInverseDependencies for the given target. Applying it to every entry of mbtBuild.mbtTargets therefore restarts all loaded presentation compilers, and repeats the inverse-dependency traversal once per target. Turbine recompiles on each debounce window, so this can restart compilers repeatedly during editing and discard warm compiler state.
Restrict the restart to the loaded compilers, or deduplicate the target set before restarting.
🤖 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 351 - 354, Update the onAnnotationProcessorStubsReady callback to
restart only the loaded presentation compilers or otherwise deduplicate targets
before invoking compilers.restartPresentationCompilers, avoiding a full
mbtBuild.mbtTargets traversal and repeated inverse-dependency walks on each
turbine recompilation.
| params.foreach { p => | ||
| val packageName = extractPackageName(p.text()) | ||
| if (packageName.nonEmpty) { | ||
| try { | ||
| fileManager | ||
| .list( | ||
| StandardLocation.SOURCE_PATH, | ||
| packageName, | ||
| java.util.EnumSet.of(JavaFileObject.Kind.SOURCE), | ||
| false | ||
| ) | ||
| .asScala | ||
| .filterNot(f => primaryUris.contains(f.toUri())) | ||
| .foreach { f => | ||
| val key = f.toUri().toString() | ||
| if (!seen.contains(key)) seen(key) = f | ||
| } | ||
| } catch { | ||
| case scala.util.control.NonFatal(e) => | ||
| logger.warn( | ||
| s"[JavaPruneCompiler] failed to list sourcepath for package '$packageName': ${e.getMessage}" | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 --glob '*.scala' \
'extractPackageName|StandardLocation\.SOURCE_PATH|packageName\.nonEmpty' \
mtags-java tests/javapcRepository: tgodzik/metals
Length of output: 2732
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== JavaPruneCompiler relevant section =="
sed -n '280,435p' mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala | cat -n
echo
echo "== Java file search for extractPackageName package behavior tests/usages =="
rg -n -C 2 --glob '*.java' --glob '*.scala' 'package[[:space:]]+|unnamed|SOURCE_PATH|JavaFileObject\.Kind\.SOURCE|list\(.*SOURCE_PATH|StandardLocation\.SOURCE_PATH' tests mtags-javaRepository: tgodzik/metals
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
java -version 2>&1 | sed -n '1p'
java - <<'JAVA'
import javax.tools.StandardLocation;
import javax.fileobject.*;
class Test { }
JAVA 2>&1 || true
python3 - <<'PY'
from pathlib import Path
p = Path('mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala')
text = p.read_text()
start = text.find('private def extractPackageName')
print(text[start:start+2000] if start != -1 else 'extractPackageName not found')
PYRepository: tgodzik/metals
Length of output: 413
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== extractPackageName implementation =="
python3 - <<'PY'
from pathlib import Path
p = Path('mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala')
text = p.read_text()
start = text.find('private def extractPackageName')
end = text.find('\n }\n', start)
print(text[start:end+4] if start != -1 else 'missing')
PY
echo
echo "== focused Java fileobject usages in repository =="
rg -n -C 3 --glob '*.scala' 'extractPackageName|package[[:space:]]+[^;]*(;|$)|StandardLocation\.SOURCE_PATH|JavacProcessingEnvironment|getPackage\.\\(|ProcessingEnvironment' mtags-java/src/main/scala mtags-java/src/main/java 2>/dev/null || true
echo
echo "== locate JavaPruneCompiler =="
fd -a 'JavaPruneCompiler\.scala$' . | sed 's#^\./##'Repository: tgodzik/metals
Length of output: 2222
🌐 Web query:
Java compiler AnnotationProcessor list SOURCE_PATH package unnamed empty string StandardLocation SOURCE_PATH
💡 Result:
In the context of the Java Annotation Processing API (javax.annotation.processing), using StandardLocation.SOURCE_PATH with the Filer to read files—including those in the unnamed package represented by an empty string—is not guaranteed to work and often fails depending on the build tool or environment [1][2][3]. Key technical points regarding this behavior include: 1. Support for StandardLocation: The Filer interface is only strictly required to support the locations StandardLocation.CLASS_OUTPUT and StandardLocation.SOURCE_OUTPUT [4][5][6]. Support for reading from StandardLocation.SOURCE_PATH is not mandated by the API specification [2][3]. 2. Environment Dependencies: Whether filer.getResource(StandardLocation.SOURCE_PATH, "", "filename") works depends on the internal implementation of the JavaFileManager provided by the compiler (e.g., javac) or the build tool (e.g., Maven, Gradle) [1][7]. In many common build environments, this location is either unsupported or improperly mapped, often resulting in null returns, FileNotFoundException, or unexpected behavior [1][2][8][9]. 3. Unnamed Package: When attempting to access files in the unnamed package, passing an empty string ("") as the package name parameter is the correct syntax according to the API, but it remains subject to the same underlying limitation of the location's support [1][4]. 4. Alternative Approaches: Because relying on StandardLocation.SOURCE_PATH is unreliable, developers frequently use workarounds when they need to locate source files or project directories: - Creating a temporary file using the Filer (in a supported location like SOURCE_OUTPUT) and traversing the directory structure upwards to find the project root [8][9][3]. - If working specifically with elements currently being processed, using the Trees API (com.sun.source.util.Trees) to obtain the compilation unit and its source file object [9]. - Relying on build-system-specific configurations or environment variables to pass the source path into the processor as an option [10][11].
Citations:
- 1: https://stackoverflow.com/questions/39772189/how-to-read-file-from-src-main-resources-with-annotation-processor
- 2: https://stackoverflow.com/questions/14675525/using-standardlocation-class-in-annotation-processor
- 3: https://stackoverflow.com/questions/22494596/eclipse-annotation-processor-get-project-path
- 4: https://docs.oracle.com/en/java/javase/25/docs/api/java.compiler/javax/annotation/processing/Filer.html
- 5: https://docs.oracle.com/en/java/javase/11/docs/api/java.compiler/javax/annotation/processing/Filer.html
- 6: https://javadoc.scijava.org/Java17/java.compiler/javax/annotation/processing/Filer.html
- 7: https://mail.openjdk.org/pipermail/jigsaw-dev/2016-July/008871.html
- 8: https://stackoverflow.com/questions/37225786/android-annotation-processor-access-resources-assets
- 9: https://stackoverflow.com/questions/71675280/how-do-i-get-the-directory-of-the-input-source-files-in-an-annotation-processo
- 10: Annotation processor's Filer can't read the files being compiled google/compile-testing#189
- 11: https://stackoverflow.com/questions/58893468/java-annotation-processor-not-generating-file-in-generated-sources
🌐 Web query:
Java Platform Module System ModulePath SourcePath Filer getLocation JavaFileManager getJavaFileForInput SOURCE_PATH empty package
💡 Result:
In the context of the Java Platform Module System and the Java Compiler API, the distinction between package-oriented and module-oriented locations is critical when using JavaFileManager [1]. The method getJavaFileForInput is designed for package-oriented locations [2][1]. If you attempt to use it with a module-oriented location, it will throw an IllegalArgumentException [3][4][5]. Regarding SOURCE_PATH: 1. SOURCE_PATH is a package-oriented location [6][7]. It is used to search for source files in a traditional package/class hierarchy [1][7]. 2. MODULE_SOURCE_PATH is the module-oriented equivalent used to search for source code organized by modules [7]. When dealing with an empty package (the default package) in a package-oriented location like SOURCE_PATH: - The packageName argument in getJavaFileForInput should be passed as an empty string ("") to represent the default package. - The file manager will then resolve the relative name (the class or file) directly under the root of the source paths provided in the location [6][3]. If you need to access files within a module-oriented location (like MODULE_SOURCE_PATH), you cannot use getJavaFileForInput directly [1]. Instead, you must first obtain a package-oriented location for the specific module of interest using getLocationForModule(location, moduleName) [8][1]. Once you have that package-oriented location, you can then proceed to use getJavaFileForInput or other file-listing methods for that specific module's contents [1].
Citations:
- 1: https://docs.oracle.com/en/java/javase/22/docs/api/java.compiler/javax/tools/JavaFileManager.Location.html
- 2: https://docs.oracle.com/en/java/javase/26/docs/api/java.compiler/javax/tools/JavaFileManager.Location.html
- 3: https://docs.oracle.com/en/java/javase/24/docs/api/java.compiler/javax/tools/JavaFileManager.html
- 4: https://docs.oracle.com/en/java/javase/23/docs/api/java.compiler/javax/tools/JavaFileManager.html
- 5: https://docs.oracle.com/en/java/javase/11/docs/api/java.compiler/javax/tools/JavaFileManager.html
- 6: https://docs.oracle.com/en/java/javase/17/docs/api/java.compiler/javax/tools/JavaFileManager.html
- 7: https://docs.oracle.com/javase/10/docs/api/javax/tools/StandardLocation.html
- 8: https://github.com/openjdk/jdk/blob/6520843f86f638fe4d1e5b3358fab5799daca654/src/java.compiler/share/classes/javax/tools/JavaFileManager.java
Include sources from the default package.
extractPackageName returns "" when a comment/blank-line source file has no package declaration. The if (packageName.nonEmpty) guard skips fileManager.list for those sources, so annotation processors do not receive sibling sourcepath units that also have no package declaration. List SOURCE_PATH with packageName even when it is empty.
Proposed fix
- if (packageName.nonEmpty) {
- try {
+ try {
fileManager
.list(
StandardLocation.SOURCE_PATH,
packageName,
@@
case scala.util.control.NonFatal(e) =>
logger.warn(
s"[JavaPruneCompiler] failed to list sourcepath for package '$packageName': ${e.getMessage}"
),
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| params.foreach { p => | |
| val packageName = extractPackageName(p.text()) | |
| if (packageName.nonEmpty) { | |
| try { | |
| fileManager | |
| .list( | |
| StandardLocation.SOURCE_PATH, | |
| packageName, | |
| java.util.EnumSet.of(JavaFileObject.Kind.SOURCE), | |
| false | |
| ) | |
| .asScala | |
| .filterNot(f => primaryUris.contains(f.toUri())) | |
| .foreach { f => | |
| val key = f.toUri().toString() | |
| if (!seen.contains(key)) seen(key) = f | |
| } | |
| } catch { | |
| case scala.util.control.NonFatal(e) => | |
| logger.warn( | |
| s"[JavaPruneCompiler] failed to list sourcepath for package '$packageName': ${e.getMessage}" | |
| ) | |
| } | |
| } | |
| } | |
| params.foreach { p => | |
| val packageName = extractPackageName(p.text()) | |
| try { | |
| fileManager | |
| .list( | |
| StandardLocation.SOURCE_PATH, | |
| packageName, | |
| java.util.EnumSet.of(JavaFileObject.Kind.SOURCE), | |
| false | |
| ) | |
| .asScala | |
| .filterNot(f => primaryUris.contains(f.toUri())) | |
| .foreach { f => | |
| val key = f.toUri().toString() | |
| if (!seen.contains(key)) seen(key) = f | |
| } | |
| } catch { | |
| case scala.util.control.NonFatal(e) => | |
| logger.warn( | |
| s"[JavaPruneCompiler] failed to list sourcepath for package '$packageName': ${e.getMessage}" | |
| ) | |
| } | |
| } |
🤖 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 `@mtags-java/src/main/scala/scala/meta/internal/jpc/JavaPruneCompiler.scala`
around lines 323 - 347, Update the sourcepath listing loop in the package-name
handling around extractPackageName so fileManager.list is called for every
parameter, including when packageName is empty. Remove the packageName.nonEmpty
guard while preserving the existing filtering, deduplication, and NonFatal
warning behavior.
tgodzik
left a comment
tgodzik
left a comment
There was a problem hiding this comment.
Added a review, let me know if you have time to answer some of the questions. I will anyway be going over the PR and making sure I fully understand it before I open a PR to metals.
| } else { | ||
| val hasBspProcessors = buildTargets | ||
| .javaTarget(scalaTarget.id) | ||
| .exists(_.javac.processorOptions.nonEmpty) |
There was a problem hiding this comment.
We should be able to join the two branches. MBT serves the javac options via BSP.
| val hasBspProcessors = buildTargets | ||
| .javaTarget(scalaTarget.id) | ||
| .exists(_.javac.processorOptions.nonEmpty) | ||
| val hasMbtFallbackProcessors = |
There was a problem hiding this comment.
I would not use fallback here, either it's BSP or MBT.
| val processorOpts = explicitJavaTarget | ||
| .map(_.javac.processorOptions) | ||
| .getOrElse(Nil) | ||
| val hasMbtProcessors = mbtBuild().mbtTargets |
There was a problem hiding this comment.
Might be duplicated:
The logic for determining if annotation processors are enabled is duplicated in several places. Consider extracting a helper method like:
|
|
||
| private val worksheetsDigests = new TrieMap[AbsolutePath, String]() | ||
|
|
||
| private val annotationProcessorBatchStarted = |
There was a problem hiding this comment.
We can probably use BatchedFunction here.
| * | ||
| * Guaranteed to run at most once per build target per Metals session. | ||
| */ | ||
| private def triggerAnnotationProcessorBatchCompilation( |
There was a problem hiding this comment.
Do we need to run it only with MBT? Should this be solved by turbine itself? (note to self: double check)
| !isGlobalClasspathEntry(file) && TurbineCompiler.isJarFile(file) | ||
| ) | ||
| val entries = projectClasspathJars.asScala | ||
| val filteredProjectClasspath = entries.filter(TurbineCompiler.isJarFile) |
There was a problem hiding this comment.
it's no longer using !isGlobalClasspathEntry(file)
| val fromK = keys(t) | ||
| val toK = keys(d) | ||
| if (fromK != toK) { | ||
| if (fromK != toK && namespacePkgs.forall(_.contains(toK))) { |
| fileManager | ||
| .list( | ||
| StandardLocation.SOURCE_PATH, | ||
| packageName, |
There was a problem hiding this comment.
Why do we need to query by package name? What sources are needed additionally for annotation processors?
| if (nextIsWhitespace) | ||
| params.text().substring(0, params.offset()) + | ||
| ";" + | ||
| (if (isMemberSelectCompletion) |
There was a problem hiding this comment.
I think we shouldn't need the changes for this PR, no? Should this be a separate fix?
| filename.endsWith(".bazelproject") | ||
| filename.endsWith(".bazelproject") || | ||
| (filename.endsWith(".json") && filename.contains("maven") && filename | ||
| .contains("install")) // required for repinning |
There was a problem hiding this comment.
Can't the file be named anything? Doesn't need to have all those 3 parts.
2 participants
Adds support for Java annotation processors (e.g. Lombok) in the Java and Scala presentation compilers for MBT/Bazel projects
Summary by CodeRabbit