Add unsafe RawPresentationCompiler implementation #24133

rochala · 2025-10-03T22:25:35Z

RawPresentationCompiler is a direct, unsafe way to access presentation compiler interface provided in
scalameta/metals#7841

It is up to the consumer to guarantee thread safety and sequential request processing.
This solves many issues with the current PresentationCompiler interface but most important are:

We now have control where and how each presentation compiler method is run. Before it was always run on a SingleThreadExecutor created by the PresentationCompiler. The original behavior will stay with the standard ScalaPresentationCompiler interface. (There is another PR that refactors that part),
If there was bug in scheduler all versions were affected,

On top of that this PR implements all things that will work out of the box:

Diagnostic provider - trivial implementation, previously not possible to LIFO task used by Scala3CompilerAccess. Will be super useful e.g for Scastie which will become a consumer of this new raw API.
InteractiveDriver compilation cancellation by checking Thread.interrupted(). Future changes are required to make it work with safe ScalaPresentationCompiler and CancelTokens. Here we can omit the cancel token support for now, because we would check for Thread.interrupted() along the cancelToken.isCancelled anyway, and we can now easily interrupt it from e.g IO.interruptible

Those changes were adapted from my other PR's that required adjustment of Scala3CompilerAccess but I don't think it is ready / I'm not happy with current state of that refactor.

In the future we should think of replacing the implementation of ScalaPresentationCompiler to use RawPresentationCompiler as underlying impl and just call it in safe way.

The implementation was tested on the actual LSP server and it works fine.

…iver will now check for Thread.isInterrupted cancellation

rochala · 2025-10-03T22:26:36Z

presentation-compiler/src/main/dotty/tools/pc/PcInlayHintsProvider.scala

-              LabelPart(": ") :: toLabelParts(tpe, pos),
-              InlayHintKind.Type,
-            )
-            .addDefinition(adjustedPos.start)


It was removed out of sudden in Scala 2 presentation compiler, I believe it is no longer necessary as we're deduplicating inlay hints during add

I think it wasn't actually doing anything anymore

rochala · 2025-10-03T22:27:06Z

presentation-compiler/test/dotty/tools/pc/tests/inlayHints/InlayHintsSuite.scala

         |
         |def test/*: (path : String<<java/lang/String#>>, num : Int<<scala/Int#>>, line : Int<<scala/Int#>>)*/ =
-         |  hello ++/*[Tuple1<<scala/Tuple1#>>["line"], Tuple1<<scala/Tuple1#>>[Int<<scala/Int#>>]]*/ (line = 1)/*(using refl<<scala/`<:<`.refl().>>)*//*[Tuple1<<scala/Tuple1#>>[Int<<scala/Int#>>]]*/
+         |  hello ++/*[Tuple1<<scala/Tuple1#>>["line"], Tuple1<<scala/Tuple1#>>[Int<<scala/Int#>>]]*/ (line = 1)/*[Tuple1<<scala/Tuple1#>>[Int<<scala/Int#>>]]*//*(using refl<<scala/`<:<`.refl().>>)*/


Caused by mtags version bump

KacperFKorban

Looks good to me. (Just "Comment", since I don't feel "in the loop" enough to understand the context of these changes)

KacperFKorban · 2025-10-06T09:43:14Z

presentation-compiler/src/main/dotty/tools/pc/PcInlayHintsProvider.scala

    }
-    
+
    tree match


TLDR: I think that it's fine to patch only the necessary changes for now, but this should probably be refactored (similarly as Scala 2 code) soon-ish.

The refactor in the Scala 2 PC was meant to allow for more than one "case" to be executed here. This isn't a problem yet, since scalameta/metals#7815 hasn't been ported yet. Though one could make an argument that this problem appeared before and was "hacked" my merging implementations for two inlay hint types.
The idea behind the refactor is:

We move the logic from the branches in this match into private methods on appropriate Inlay-Hint-providing-objects.

Afterwards, this method is simplified to a foldLeft through all the cases.

Every Inlay-Hint-providing-objects returns its inlay hints in separation, and we merge them together for every tree.

Afterwards, they can be deduplicated (right now only for InferredType) – this separates generation and deduplication.

I think that a similar refactor should be done here as well, though it maybe shouldn't be part of this PR.

KacperFKorban · 2025-10-06T09:44:36Z

presentation-compiler/src/main/dotty/tools/pc/RawScalaPresentationCompiler.scala

+import org.eclipse.lsp4j as l
+
+
+case class RawScalaPresentationCompiler(


Could you add a comment that explains the purpose of this class?

tgodzik · 2025-10-08T11:04:24Z

project/Build.scala


  lazy val presentationCompilerSettings = {
-    val mtagsVersion = "1.6.2"
+    val mtagsVersion = "1.6.2+105-e665821a-SNAPSHOT"


We cannot use a snapshot version, since now they get removed after 60 days. We need to wait for Metals release, so this will go into 3.8.1 most likely

Do we have an ETA for Metals release?

I think this will be next week because of all the other stuff going on

Looking at the contents, I'd try to get this in 3.8.0, still... let's try to coordinate with @WojciechMazur and perhaps we could get it in time for 3.8.0-RC1?

presentation-compiler/src/main/dotty/tools/pc/CachingDriver.scala

tgodzik · 2025-10-08T11:11:00Z

presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala

+      )
+      lspDiag.setCode(diag.msg.errorId.errorNumber)
+
+      val scalaDiagnostic = new bsp4j.ScalaDiagnostic()


Shouldn't we instead create our own diagnotic in metals and translate them to code actions as well? We wouldn't need to add bsp4j to the PC compiler.

tgodzik · 2025-10-08T11:11:46Z

presentation-compiler/src/main/dotty/tools/pc/RawScalaPresentationCompiler.scala

+  private val forbiddenOptions = Set("-print-lines", "-print-tasty")
+  private val forbiddenDoubleOptions = Set.empty[String]
+
+  val driverSettings =


This seems duplicated with the Scala Presentation Compiler, shouldn't we reuse this class in the old ScalaPResentationCompiler?

I didn't want to touch original presentation compiler for now. In the end I'll completely replace implementation of ScalaPresentationCompiler to use RawScalaPresentationCompiler as underlying impl but lets try it first.

ScalaPresentationCompiler in my vision will become just a wrapper that ensures thread safety and sequential processing of messages.

tgodzik · 2025-10-08T11:13:19Z

presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala

+class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams):
+
+  def diagnostics(): List[lsp4j.Diagnostic] =
+    val diags = driver.run(params.uri().nn, params.text().nn)


We should check if diagnotics are expected via params.shouldReturnDiagnostics()

I added it recently, will be available in the new metals

Can you tell me the scenario in which you don't want to return diags ?

LSP always sends request after keypress in following order:
didChange -> completion -> inlayHints

It will be compiled by completion or inlay hints anyway so it doesn't matter, and getting diagnostics is a free operation in terms of computations.

I was thinking of cases where people would want to switch between the two modes.

Ok, I see it, it would be very easy to control this on lsp level though but yeah. We can add it for simplicity.

tgodzik · 2025-10-08T11:13:40Z

project/Build.scala

        "io.get-coursier" % "interface" % "1.0.18",
        "org.scalameta" % "mtags-interfaces" % mtagsVersion,
        "com.google.guava" % "guava" % "33.2.1-jre",
+        "ch.epfl.scala" % "bsp4j" % "2.1.1",


We shouldn't add it for Code Actions only

Add RawPresentationCompiler impl, diagnostic provider, interactive dr…

2404514

…iver will now check for Thread.isInterrupted cancellation

rochala commented Oct 3, 2025

View reviewed changes

rochala requested review from KacperFKorban and tgodzik October 3, 2025 22:27

Make tests not flaky

8001619

KacperFKorban reviewed Oct 6, 2025

View reviewed changes

Gedochao assigned tgodzik Oct 8, 2025

tgodzik reviewed Oct 8, 2025

View reviewed changes

presentation-compiler/src/main/dotty/tools/pc/CachingDriver.scala Show resolved Hide resolved

tgodzik reviewed Oct 8, 2025

View reviewed changes

tgodzik requested changes Oct 8, 2025

View reviewed changes

Remove bsp4j, add doc, add test for code action

53c8eec

		import org.eclipse.lsp4j as l


		case class RawScalaPresentationCompiler(

Add unsafe RawPresentationCompiler implementation #24133

Are you sure you want to change the base?

Add unsafe RawPresentationCompiler implementation #24133

Uh oh!

Conversation

rochala commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KacperFKorban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rochala Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rochala Oct 8, 2025 •

edited

Loading