From f989032591370a62b280bf66fe5ecc09e8d94ebb Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Fri, 12 Dec 2025 16:20:36 +0100 Subject: [PATCH 1/2] improvement: Allow passing -explain to the presentation compiler --- .../dotty/tools/pc/DiagnosticProvider.scala | 7 ++- .../tools/pc/base/BaseDiagnosticsSuite.scala | 39 +++++++++++++ .../pc/tests/DiagnosticProviderSuite.scala | 44 +++----------- .../ExplainDiagnosticProviderSuite.scala | 57 +++++++++++++++++++ 4 files changed, 109 insertions(+), 38 deletions(-) create mode 100644 presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala create mode 100644 presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala diff --git a/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala index 878dcf72d89b..1d84f872a122 100644 --- a/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala +++ b/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala @@ -15,7 +15,6 @@ import scala.jdk.CollectionConverters.* import scala.meta.pc.VirtualFileParams class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams): - def diagnostics(): List[lsp4j.Diagnostic] = if params.shouldReturnDiagnostics then val diags = driver.run(params.uri().nn, params.text().nn) @@ -25,9 +24,13 @@ class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams): private def toLsp(diag: Diagnostic)(using Context): Option[lsp4j.Diagnostic] = Option.when(diag.pos.exists): + val message = + if Diagnostic.shouldExplain(diag) then + diag.msg.message + "\n\n# Explanation (enabled by `-explain`)\n\n" + diag.msg.explanation + else diag.msg.message val lspDiag = lsp4j.Diagnostic( diag.pos.toLsp, - diag.msg.message, + message, toDiagnosticSeverity(diag.level), "presentation compiler", ) diff --git a/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala new file mode 100644 index 000000000000..74ecffe6800c --- /dev/null +++ b/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala @@ -0,0 +1,39 @@ +package dotty.tools.pc.base + +import dotty.tools.pc.RawScalaPresentationCompiler +import dotty.tools.pc.base.TestResources +import dotty.tools.pc.utils.PcAssertions +import dotty.tools.pc.utils.TestExtensions.getOffset +import org.eclipse.lsp4j.Diagnostic +import org.eclipse.lsp4j.DiagnosticSeverity + +import java.net.URI +import scala.meta.internal.jdk.CollectionConverters.* +import scala.meta.internal.metals.EmptyCancelToken +import scala.meta.pc.CancelToken +import scala.meta.pc.VirtualFileParams + +class BaseDiagnosticsSuite extends PcAssertions: + case class TestDiagnostic(startIndex: Int, endIndex: Int, msg: String, severity: DiagnosticSeverity) + + def options : List[String] = Nil + + val pc = RawScalaPresentationCompiler().newInstance("", TestResources.classpath.asJava, options.asJava) + + case class TestVirtualFileParams(uri: URI, text: String) extends VirtualFileParams { + override def shouldReturnDiagnostics: Boolean = true + override def token: CancelToken = EmptyCancelToken + } + + def check( + text: String, + expected: List[TestDiagnostic], + additionalChecks: List[Diagnostic] => Unit = identity + ): Unit = + val diagnostics = pc + .didChange(TestVirtualFileParams(URI.create("file:/Diagnostic.scala"), text)) + .asScala + + val actual = diagnostics.map(d => TestDiagnostic(d.getRange().getStart().getOffset(text), d.getRange().getEnd().getOffset(text), d.getMessage(), d.getSeverity())) + assertEquals(expected, actual, s"Expected [${expected.mkString(", ")}] but got [${actual.mkString(", ")}]") + additionalChecks(diagnostics.toList) diff --git a/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala b/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala index 96b525931f50..8e060a82f17f 100644 --- a/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala @@ -1,43 +1,15 @@ package dotty.tools.pc.tests -import java.net.URI -import scala.meta.internal.jdk.CollectionConverters.* -import scala.meta.internal.metals.EmptyCancelToken -import scala.meta.pc.VirtualFileParams -import scala.meta.pc.CancelToken - -import org.junit.Test -import org.eclipse.lsp4j.DiagnosticSeverity -import dotty.tools.pc.utils.TestExtensions.getOffset -import dotty.tools.pc.base.TestResources -import java.nio.file.Path -import dotty.tools.pc.RawScalaPresentationCompiler -import dotty.tools.pc.utils.PcAssertions -import org.eclipse.lsp4j.Diagnostic +import dotty.tools.pc.base.BaseDiagnosticsSuite import org.eclipse.lsp4j.CodeAction +import org.eclipse.lsp4j.Diagnostic +import org.eclipse.lsp4j.DiagnosticSeverity +import org.junit.Test -class DiagnosticProviderSuite extends PcAssertions { - case class TestDiagnostic(startIndex: Int, endIndex: Int, msg: String, severity: DiagnosticSeverity) - - val pc = RawScalaPresentationCompiler().newInstance("", TestResources.classpath.asJava, Nil.asJava) - - case class TestVirtualFileParams(uri: URI, text: String) extends VirtualFileParams { - override def shouldReturnDiagnostics: Boolean = true - override def token: CancelToken = EmptyCancelToken - } - - def check( - text: String, - expected: List[TestDiagnostic], - additionalChecks: List[Diagnostic] => Unit = identity - ): Unit = - val diagnostics = pc - .didChange(TestVirtualFileParams(URI.create("file:/Diagnostic.scala"), text)) - .asScala +import java.net.URI +import scala.meta.internal.jdk.CollectionConverters.* - val actual = diagnostics.map(d => TestDiagnostic(d.getRange().getStart().getOffset(text), d.getRange().getEnd().getOffset(text), d.getMessage(), d.getSeverity())) - assertEquals(expected, actual, s"Expected [${expected.mkString(", ")}] but got [${actual.mkString(", ")}]") - additionalChecks(diagnostics.toList) +class DiagnosticProviderSuite extends BaseDiagnosticsSuite { @Test def error = check( @@ -45,7 +17,7 @@ class DiagnosticProviderSuite extends PcAssertions { | Int.maaxValue |""".stripMargin, List(TestDiagnostic(12,25, "value maaxValue is not a member of object Int - did you mean Int.MaxValue?", DiagnosticSeverity.Error)) - ) + ) @Test def warning = check( diff --git a/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala b/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala new file mode 100644 index 000000000000..fcc52526591a --- /dev/null +++ b/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala @@ -0,0 +1,57 @@ +package dotty.tools.pc.tests + +import dotty.tools.pc.base.BaseDiagnosticsSuite +import org.eclipse.lsp4j.CodeAction +import org.eclipse.lsp4j.Diagnostic +import org.eclipse.lsp4j.DiagnosticSeverity +import org.junit.Test + +import java.net.URI +import scala.meta.internal.jdk.CollectionConverters.* + +class ExplainDiagnosticProviderSuite extends BaseDiagnosticsSuite { + + override def options : List[String] = List("-explain") + + @Test def error1 = + check( + """|object C: + | def m(x: Int) = 1 + | object T extends K: + | val x = m(1) // error + |class K: + | def m(i: Int) = 2 + |""".stripMargin, + List( + TestDiagnostic( + 64,65, + """|Reference to m is ambiguous. + |It is both defined in object C + |and inherited subsequently in object T + | + |# Explanation (enabled by `-explain`) + | + |The identifier m is ambiguous because a name binding of lower precedence + |in an inner scope cannot shadow a binding with higher precedence in + |an outer scope. + | + |The precedence of the different kinds of name bindings, from highest to lowest, is: + | - Definitions in an enclosing scope + | - Inherited definitions and top-level definitions in packages + | - Names introduced by import of a specific name + | - Names introduced by wildcard import + | - Definitions from packages in other files + |Note: + | - As a rule, definitions take precedence over imports. + | - Definitions in an enclosing scope take precedence over inherited definitions, + | which can result in ambiguities in nested classes. + | - When importing, you can avoid naming conflicts by renaming: + | import scala.{m => mTick} + |""".stripMargin, + DiagnosticSeverity.Error + + ) + ) + ) + +} From 2f1d6d45a9c478dd37e7369b640bf2cd672e9520 Mon Sep 17 00:00:00 2001 From: Tomasz Godzik Date: Mon, 15 Dec 2025 16:56:47 +0100 Subject: [PATCH 2/2] nit: Improve formatting of changes files --- .../dotty/tools/pc/DiagnosticProvider.scala | 24 +++++--- .../tools/pc/base/BaseDiagnosticsSuite.scala | 45 ++++++++++---- .../pc/tests/DiagnosticProviderSuite.scala | 59 ++++++++++++++++--- .../ExplainDiagnosticProviderSuite.scala | 10 ++-- 4 files changed, 104 insertions(+), 34 deletions(-) diff --git a/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala b/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala index 1d84f872a122..a77b28ef6c08 100644 --- a/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala +++ b/presentation-compiler/src/main/dotty/tools/pc/DiagnosticProvider.scala @@ -24,15 +24,16 @@ class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams): private def toLsp(diag: Diagnostic)(using Context): Option[lsp4j.Diagnostic] = Option.when(diag.pos.exists): - val message = - if Diagnostic.shouldExplain(diag) then - diag.msg.message + "\n\n# Explanation (enabled by `-explain`)\n\n" + diag.msg.explanation - else diag.msg.message + val explanation = + if Diagnostic.shouldExplain(diag) + then + "\n\n# Explanation (enabled by `-explain`)\n\n" + diag.msg.explanation + else "" val lspDiag = lsp4j.Diagnostic( diag.pos.toLsp, - message, + diag.msg.message + explanation, toDiagnosticSeverity(diag.level), - "presentation compiler", + "presentation compiler" ) lspDiag.setCode(diag.msg.errorId.errorNumber) @@ -48,7 +49,8 @@ class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams): val lspAction = lsp4j.CodeAction(action.title) lspAction.setKind(lsp4j.CodeActionKind.QuickFix) lspAction.setIsPreferred(true) - val edits = action.patches.groupBy(_.srcPos.source.path) + val edits = action.patches + .groupBy(_.srcPos.source.path) .map((path, actions) => path -> (actions.map(toLspTextEdit).asJava)) .asJava @@ -57,8 +59,12 @@ class DiagnosticProvider(driver: InteractiveDriver, params: VirtualFileParams): lspAction private def toLspTextEdit(actionPatch: ActionPatch): lsp4j.TextEdit = - val startPos = lsp4j.Position(actionPatch.srcPos.startLine, actionPatch.srcPos.startColumn) - val endPos = lsp4j.Position(actionPatch.srcPos.endLine, actionPatch.srcPos.endColumn) + val startPos = lsp4j.Position( + actionPatch.srcPos.startLine, + actionPatch.srcPos.startColumn + ) + val endPos = + lsp4j.Position(actionPatch.srcPos.endLine, actionPatch.srcPos.endColumn) val range = lsp4j.Range(startPos, endPos) lsp4j.TextEdit(range, actionPatch.replacement) diff --git a/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala b/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala index 74ecffe6800c..d6ec62447f70 100644 --- a/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/base/BaseDiagnosticsSuite.scala @@ -14,26 +14,49 @@ import scala.meta.pc.CancelToken import scala.meta.pc.VirtualFileParams class BaseDiagnosticsSuite extends PcAssertions: - case class TestDiagnostic(startIndex: Int, endIndex: Int, msg: String, severity: DiagnosticSeverity) + case class TestDiagnostic( + startIndex: Int, + endIndex: Int, + msg: String, + severity: DiagnosticSeverity + ) - def options : List[String] = Nil - - val pc = RawScalaPresentationCompiler().newInstance("", TestResources.classpath.asJava, options.asJava) + def options: List[String] = Nil - case class TestVirtualFileParams(uri: URI, text: String) extends VirtualFileParams { + val pc = RawScalaPresentationCompiler().newInstance( + "", + TestResources.classpath.asJava, + options.asJava + ) + + case class TestVirtualFileParams(uri: URI, text: String) + extends VirtualFileParams { override def shouldReturnDiagnostics: Boolean = true override def token: CancelToken = EmptyCancelToken } def check( - text: String, - expected: List[TestDiagnostic], - additionalChecks: List[Diagnostic] => Unit = identity + text: String, + expected: List[TestDiagnostic], + additionalChecks: List[Diagnostic] => Unit = identity ): Unit = val diagnostics = pc - .didChange(TestVirtualFileParams(URI.create("file:/Diagnostic.scala"), text)) + .didChange( + TestVirtualFileParams(URI.create("file:/Diagnostic.scala"), text) + ) .asScala - val actual = diagnostics.map(d => TestDiagnostic(d.getRange().getStart().getOffset(text), d.getRange().getEnd().getOffset(text), d.getMessage(), d.getSeverity())) - assertEquals(expected, actual, s"Expected [${expected.mkString(", ")}] but got [${actual.mkString(", ")}]") + val actual = diagnostics.map(d => + TestDiagnostic( + d.getRange().getStart().getOffset(text), + d.getRange().getEnd().getOffset(text), + d.getMessage(), + d.getSeverity() + ) + ) + assertEquals( + expected, + actual, + s"Expected [${expected.mkString(", ")}] but got [${actual.mkString(", ")}]" + ) additionalChecks(diagnostics.toList) diff --git a/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala b/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala index 8e060a82f17f..8a59a0ce335d 100644 --- a/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/tests/DiagnosticProviderSuite.scala @@ -16,15 +16,29 @@ class DiagnosticProviderSuite extends BaseDiagnosticsSuite { """|object M: | Int.maaxValue |""".stripMargin, - List(TestDiagnostic(12,25, "value maaxValue is not a member of object Int - did you mean Int.MaxValue?", DiagnosticSeverity.Error)) - ) + List( + TestDiagnostic( + 12, + 25, + "value maaxValue is not a member of object Int - did you mean Int.MaxValue?", + DiagnosticSeverity.Error + ) + ) + ) @Test def warning = check( """|object M: | 1 + 1 |""".stripMargin, - List(TestDiagnostic(12, 17, "A pure expression does nothing in statement position", DiagnosticSeverity.Warning)) + List( + TestDiagnostic( + 12, + 17, + "A pure expression does nothing in statement position", + DiagnosticSeverity.Warning + ) + ) ) @Test def mixed = @@ -34,8 +48,18 @@ class DiagnosticProviderSuite extends BaseDiagnosticsSuite { | 1 + 1 |""".stripMargin, List( - TestDiagnostic(12,25, "value maaxValue is not a member of object Int - did you mean Int.MaxValue?", DiagnosticSeverity.Error), - TestDiagnostic(28, 33, "A pure expression does nothing in statement position", DiagnosticSeverity.Warning) + TestDiagnostic( + 12, + 25, + "value maaxValue is not a member of object Int - did you mean Int.MaxValue?", + DiagnosticSeverity.Error + ), + TestDiagnostic( + 28, + 33, + "A pure expression does nothing in statement position", + DiagnosticSeverity.Warning + ) ) ) @@ -45,12 +69,29 @@ class DiagnosticProviderSuite extends BaseDiagnosticsSuite { | private private class Test |""".stripMargin, List( - TestDiagnostic(20, 27, "Repeated modifier private", DiagnosticSeverity.Error), + TestDiagnostic( + 20, + 27, + "Repeated modifier private", + DiagnosticSeverity.Error + ) ), diags => - val action = diags.head.getData().asInstanceOf[java.util.List[CodeAction]].asScala.head - assertWithDiff("Remove repeated modifier: \"private\"", action.getTitle(), false) - assertEquals(1, action.getEdit().getChanges().size(), "There should be one change") + val action = diags.head + .getData() + .asInstanceOf[java.util.List[CodeAction]] + .asScala + .head + assertWithDiff( + "Remove repeated modifier: \"private\"", + action.getTitle(), + false + ) + assertEquals( + 1, + action.getEdit().getChanges().size(), + "There should be one change" + ) ) } diff --git a/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala b/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala index fcc52526591a..34e044a8e2d8 100644 --- a/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala +++ b/presentation-compiler/test/dotty/tools/pc/tests/ExplainDiagnosticProviderSuite.scala @@ -11,7 +11,7 @@ import scala.meta.internal.jdk.CollectionConverters.* class ExplainDiagnosticProviderSuite extends BaseDiagnosticsSuite { - override def options : List[String] = List("-explain") + override def options: List[String] = List("-explain") @Test def error1 = check( @@ -24,8 +24,9 @@ class ExplainDiagnosticProviderSuite extends BaseDiagnosticsSuite { |""".stripMargin, List( TestDiagnostic( - 64,65, - """|Reference to m is ambiguous. + 64, + 65, + """|Reference to m is ambiguous. |It is both defined in object C |and inherited subsequently in object T | @@ -48,8 +49,7 @@ class ExplainDiagnosticProviderSuite extends BaseDiagnosticsSuite { | - When importing, you can avoid naming conflicts by renaming: | import scala.{m => mTick} |""".stripMargin, - DiagnosticSeverity.Error - + DiagnosticSeverity.Error ) ) )