Skip to content

Add unsafe RawPresentationCompiler implementation #24133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+525 −32
Open

Add unsafe RawPresentationCompiler implementation #24133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2404514
Add RawPresentationCompiler impl, diagnostic provider, interactive dr…
rochala Oct 3, 2025
8001619
Make tests not flaky
rochala Oct 3, 2025
53c8eec
Remove bsp4j, add doc, add test for code action
rochala Oct 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/core/Contexts.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ object Contexts {
val local = incCallback
local != null && local.enabled || forceRun

/** The Zinc compile progress callback implementation if we are run from Zinc, null otherwise */
/** The Zinc compile progress callback implementation if we are run from Zinc or used by presentation compiler, null otherwise */
def progressCallback: ProgressCallback | Null = store(progressCallbackLoc)

/** Run `op` if there exists a Zinc progress callback */
Expand Down
9 changes: 6 additions & 3 deletions compiler/src/dotty/tools/dotc/interactive/InteractiveDriver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import java.util.zip.*
import scala.collection.*
import scala.io.Codec

import dotty.tools.dotc.sbt.interfaces.ProgressCallback
import dotty.tools.io.AbstractFile

import ast.{Trees, tpd}
Expand All @@ -30,6 +31,9 @@ class InteractiveDriver(val settings: List[String]) extends Driver {

override def sourcesRequired: Boolean = false

private var myProgressCallback: ProgressCallback = new ProgressCallback:
override def isCancelled(): Boolean = Thread.interrupted()

private val myInitCtx: Context = {
val rootCtx = initCtx.fresh.addMode(Mode.ReadPositions).addMode(Mode.Interactive)
rootCtx.setSetting(rootCtx.settings.YretainTrees, true)
Expand Down Expand Up @@ -151,7 +155,7 @@ class InteractiveDriver(val settings: List[String]) extends Driver {
val reporter =
new StoreReporter(null) with UniqueMessagePositions with HideNonSensicalMessages

val run = compiler.newRun(using myInitCtx.fresh.setReporter(reporter))
val run = compiler.newRun(using myInitCtx.fresh.setReporter(reporter).setProgressCallback(myProgressCallback))
myCtx = run.runContext.withRootImports

given Context = myCtx
Expand All @@ -169,8 +173,7 @@ class InteractiveDriver(val settings: List[String]) extends Driver {
myCtx = myCtx.fresh.setPhase(myInitCtx.base.typerPhase)

reporter.removeBufferedMessages
}
catch {
} catch {
case ex: FatalError =>
myCtx = previousCtx
close(uri)
Expand Down
16 changes: 4 additions & 12 deletions presentation-compiler/src/main/dotty/tools/pc/CachingDriver.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import scala.compiletime.uninitialized
*/
class CachingDriver(override val settings: List[String]) extends InteractiveDriver(settings):

@volatile private var lastCompiledURI: URI = uninitialized
private var lastCompiledURI: URI = uninitialized
private var previousDiags = List.empty[Diagnostic]

private def alreadyCompiled(uri: URI, content: Array[Char]): Boolean =
compilationUnits.get(uri) match
Expand All @@ -40,17 +41,8 @@ class CachingDriver(override val settings: List[String]) extends InteractiveDriv
case _ => false

override def run(uri: URI, source: SourceFile): List[Diagnostic] =
val diags =
if alreadyCompiled(uri, source.content) then Nil
else super.run(uri, source)
if !alreadyCompiled(uri, source.content) then previousDiags = super.run(uri, source)
lastCompiledURI = uri
diags

override def run(uri: URI, sourceCode: String): List[Diagnostic] =
val diags =
if alreadyCompiled(uri, sourceCode.toCharArray().nn) then Nil
else super.run(uri, sourceCode)
lastCompiledURI = uri
diags
previousDiags

end CachingDriver
65 changes: 65 additions & 0 deletions presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package dotty.tools.pc

import org.eclipse.lsp4j
import org.eclipse.lsp4j.DiagnosticSeverity
import dotty.tools.dotc.interactive.InteractiveDriver
import dotty.tools.dotc.interfaces.Diagnostic as DiagnosticInterfaces
import dotty.tools.dotc.reporting.Diagnostic
import dotty.tools.pc.utils.InteractiveEnrichments.toLsp

import scala.meta.pc.VirtualFileParams
import ch.epfl.scala.bsp4j
import dotty.tools.dotc.reporting.CodeAction
import dotty.tools.dotc.rewrites.Rewrites.ActionPatch
import scala.jdk.CollectionConverters.*
import dotty.tools.dotc.core.Contexts.Context
import dotty.tools.dotc.reporting.ErrorMessageID
import org.eclipse.lsp4j.DiagnosticTag

class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams):

def diagnostics(): List[lsp4j.Diagnostic] =
val diags = driver.run(params.uri().nn, params.text().nn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@rochala rochala Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

given Context = driver.currentCtx
diags.flatMap(toLsp)

private def toLsp(diag: Diagnostic)(using Context): Option[lsp4j.Diagnostic] =
Option.when(diag.pos.exists):
val lspDiag = lsp4j.Diagnostic(
diag.pos.toLsp,
diag.msg.message,
toDiagnosticSeverity(diag.level),
"presentation compiler",
)
lspDiag.setCode(diag.msg.errorId.errorNumber)

val scalaDiagnostic = new bsp4j.ScalaDiagnostic()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val actions = diag.msg.actions.map(toBspScalaAction).asJava
scalaDiagnostic.setActions(actions)
// lspDiag.setRelatedInformation(???) Currently not emitted by the compiler
lspDiag.setData(scalaDiagnostic)
if diag.msg.errorId == ErrorMessageID.UnusedSymbolID then
lspDiag.setTags(List(DiagnosticTag.Unnecessary).asJava)

lspDiag

private def toBspScalaAction(action: CodeAction): bsp4j.ScalaAction =
val bspAction = bsp4j.ScalaAction(action.title)
action.description.foreach(bspAction.setDescription)
val workspaceEdit = bsp4j.ScalaWorkspaceEdit(action.patches.map(toBspTextEdit).asJava)
bspAction.setEdit(workspaceEdit)
bspAction

private def toBspTextEdit(actionPatch: ActionPatch): bsp4j.ScalaTextEdit =
val startPos = bsp4j.Position(actionPatch.srcPos.startLine, actionPatch.srcPos.startColumn)
val endPos = bsp4j.Position(actionPatch.srcPos.endLine, actionPatch.srcPos.endColumn)
val range = bsp4j.Range(startPos, endPos)
bsp4j.ScalaTextEdit(range, actionPatch.replacement)


private def toDiagnosticSeverity(severity: Int): DiagnosticSeverity =
severity match
case DiagnosticInterfaces.ERROR => DiagnosticSeverity.Error
case DiagnosticInterfaces.WARNING => DiagnosticSeverity.Warning
case DiagnosticInterfaces.INFO => DiagnosticSeverity.Information
case _ => DiagnosticSeverity.Information
29 changes: 17 additions & 12 deletions presentation-compiler/src/main/dotty/tools/pc/PcInlayHintsProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import dotty.tools.dotc.util.Spans.Span
import org.eclipse.lsp4j.InlayHint
import org.eclipse.lsp4j.InlayHintKind
import org.eclipse.{lsp4j as l}
import scala.meta.internal.pc.InlayHintOrigin

class PcInlayHintsProvider(
driver: InteractiveDriver,
Expand Down Expand Up @@ -80,7 +81,7 @@ class PcInlayHintsProvider(
)
case _ => inlayHints
}

tree match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case ImplicitConversion(symbol, range) =>
val adjusted = adjustPos(range)
Expand All @@ -89,11 +90,13 @@ class PcInlayHintsProvider(
adjusted.startPos.toLsp,
labelPart(symbol, symbol.decodedName) :: LabelPart("(") :: Nil,
InlayHintKind.Parameter,
InlayHintOrigin.ImplicitConversion
)
.add(
adjusted.endPos.toLsp,
LabelPart(")") :: Nil,
InlayHintKind.Parameter,
InlayHintOrigin.ImplicitConversion
)
case ImplicitParameters(trees, pos) =>
firstPassHints.add(
Expand All @@ -104,12 +107,14 @@ class PcInlayHintsProvider(
case None => LabelPart(label)
),
InlayHintKind.Parameter,
InlayHintOrigin.ImplicitParameters
)
case ValueOf(label, pos) =>
firstPassHints.add(
adjustPos(pos).toLsp,
LabelPart("(") :: LabelPart(label) :: List(LabelPart(")")),
InlayHintKind.Parameter,
InlayHintOrigin.ImplicitParameters
)
case TypeParameters(tpes, pos, sel)
if !syntheticTupleApply(sel) =>
Expand All @@ -118,19 +123,18 @@ class PcInlayHintsProvider(
adjustPos(pos).endPos.toLsp,
label,
InlayHintKind.Type,
InlayHintOrigin.TypeParameters
)
case InferredType(tpe, pos, defTree)
if !isErrorTpe(tpe) =>
val adjustedPos = adjustPos(pos).endPos
if firstPassHints.containsDef(adjustedPos.start) then firstPassHints
else
firstPassHints
.add(
adjustedPos.toLsp,
LabelPart(": ") :: toLabelParts(tpe, pos),
InlayHintKind.Type,
)
.addDefinition(adjustedPos.start)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it wasn't actually doing anything anymore

firstPassHints
.add(
adjustedPos.toLsp,
LabelPart(": ") :: toLabelParts(tpe, pos),
InlayHintKind.Type,
InlayHintOrigin.InferredType
)
case Parameters(isInfixFun, args) =>
def isNamedParam(pos: SourcePosition): Boolean =
val start = text.indexWhere(!_.isWhitespace, pos.start)
Expand Down Expand Up @@ -167,6 +171,7 @@ class PcInlayHintsProvider(
hintPos.startPos.toLsp,
List(LabelPart(labelStr)),
InlayHintKind.Parameter,
if params.byNameParameters then InlayHintOrigin.ByNameParameters else InlayHintOrigin.NamedParameters
)
else ih
}
Expand Down Expand Up @@ -515,7 +520,7 @@ object XRayModeHint:
case Some(sel: Select) if sel.isForComprehensionMethod => false
case Some(par) if par.sourcePos.exists && par.sourcePos.line == tree.sourcePos.line => true
case _ => false

tree match
/*
anotherTree
Expand All @@ -530,7 +535,7 @@ object XRayModeHint:
.select
*/
case select @ Select(innerTree, _)
if innerTree.sourcePos.exists && !isParentOnSameLine && !isParentApply &&
if innerTree.sourcePos.exists && !isParentOnSameLine && !isParentApply &&
isEndOfLine(tree.sourcePos) =>
Some((select.tpe.widen.deepDealiasAndSimplify, tree.sourcePos))
case _ => None
Expand Down
Loading
Loading