Multi-span error reporting for overriding

Linyxus · Linyxus · commit fa179d0927f3 · 2025-12-10T14:02:15.000+01:00
diff --git a/compiler/src/dotty/tools/dotc/reporting/MessageRendering.scala b/compiler/src/dotty/tools/dotc/reporting/MessageRendering.scala
@@ -12,7 +12,7 @@ import io.AbstractFile
 import printing.Highlighting.{Blue, Red, Yellow}
 import printing.SyntaxHighlighting
 import Diagnostic.*
-import util.{SourcePosition, NoSourcePosition}
+import util.{SourceFile, SourcePosition, NoSourcePosition}
 import util.Chars.{ LF, CR, FF, SU }
 import scala.annotation.switch
 
@@ -253,47 +253,21 @@ trait MessageRendering {
     else
     pos
 
-  /** Render a message using multi-span information from Message.parts. */
-  def messageAndPosFromParts(dia: Diagnostic)(using Context): String =
-    val msg = dia.msg
-    val pos = dia.pos
-    val pos1 = adjust(pos.nonInlined)
-    val msgParts = msg.parts
-
-    if msgParts.isEmpty then
-      return msg.leading.getOrElse("") + (if msg.leading.isDefined then "\n" else "") + msg.message
-
-    // Collect all positions from message parts
-    val validParts = msgParts.filter(_.srcPos.exists)
-
-    if validParts.isEmpty then
-      return msg.leading.getOrElse("") + (if msg.leading.isDefined then "\n" else "") + msg.message
+  /** Render parts for a single source file.
+   *  @param parts the message parts to render (all must be from the same source)
+   *  @param sb the StringBuilder to append to
+   */
+  private def renderPartsForFile(
+    parts: List[Message.MessagePart],
+    sb: StringBuilder
+  )(using Context, Level, Offset): Unit =
+    if parts.isEmpty then return
 
-    // Check all positions are in the same source file
-    val source = validParts.head.srcPos.source
-    if !validParts.forall(_.srcPos.source == source) || !source.file.exists then
-      // TODO: support rendering source positions across multiple files
-      return msg.leading.getOrElse("") + (if msg.leading.isDefined then "\n" else "") + msg.message
+    val source = parts.head.srcPos.source
 
     // Find the line range covering all positions
-    val minLine = validParts.map(_.srcPos.startLine).min
-    val maxLine = validParts.map(_.srcPos.endLine).max
-    val maxLineNumber = maxLine + 1
-
-    given Level = Level(dia.level)
-    given Offset = Offset(maxLineNumber.toString.length + 2)
-
-    val sb = StringBuilder()
-
-    // Title using the primary position
-    val posString = posStr(pos1, msg, diagnosticLevel(dia))
-    if posString.nonEmpty then sb.append(posString).append(EOL)
-
-    // Display leading text if present
-    msg.leading.foreach { leadingText =>
-      sb.append(leadingText)
-      if !leadingText.endsWith(EOL) then sb.append(EOL)
-    }
+    val minLine = parts.map(_.srcPos.startLine).min
+    val maxLine = parts.map(_.srcPos.endLine).max
 
     // Render the unified code snippet
     val startOffset = source.lineToOffset(minLine)
@@ -316,6 +290,7 @@ trait MessageRendering {
     }
 
     val lines = linesFrom(syntax)
+    val maxLineNumber = maxLine + 1
     val lineNumberWidth = maxLineNumber.toString.length
 
     // Render each line with its markers and messages
@@ -329,7 +304,7 @@ trait MessageRendering {
         sb.append(lnum).append(lineContent.stripLineEnd).append(EOL)
 
         // Find all positions that should show markers after this line
-        val partsOnLine = validParts.filter(_.srcPos.startLine == lineNum)
+        val partsOnLine = parts.filter(_.srcPos.startLine == lineNum)
           .sortBy(p => (p.srcPos.startColumn, !p.isPrimary))
 
         if partsOnLine.size == 1 then
@@ -382,17 +357,76 @@ trait MessageRendering {
             connectorLine.append(msgPadding).append(msgText)
 
             sb.append(connectorLine).append(EOL)
+  end renderPartsForFile
+
+  /** Group consecutive parts by their source file. */
+  private def groupPartsByFile(parts: List[Message.MessagePart]): List[(SourceFile, List[Message.MessagePart])] =
+    if parts.isEmpty then Nil
+    else
+      val head = parts.head
+      val source = head.srcPos.source
+      val (sameSrc, rest) = parts.span(_.srcPos.source == source)
+      (source, sameSrc) :: groupPartsByFile(rest)
+
+  /** Render a message using multi-span information from Message.parts. */
+  def messageAndPosFromParts(dia: Diagnostic)(using Context): String =
+    val msg = dia.msg
+    val pos = dia.pos
+    val pos1 = adjust(pos.nonInlined)
+    val msgParts = msg.parts
+
+    if msgParts.isEmpty then
+      return msg.leading.getOrElse("") + (if msg.leading.isDefined then "\n" else "") + msg.message
+
+    // Collect all positions from message parts
+    val validParts = msgParts.filter(p => p.srcPos.exists && p.srcPos.source.file.exists)
+
+    if validParts.isEmpty then
+      return msg.leading.map(_ + "\n").getOrElse("") + msg.message
+
+    // Group parts by consecutive source files
+    val groupedParts = groupPartsByFile(validParts)
+
+    // Calculate the maximum line number across all files for consistent offset
+    val maxLineNumber = validParts.map(_.srcPos.endLine).max + 1
+
+    given Level = Level(dia.level)
+    given Offset = Offset(maxLineNumber.toString.length + 2)
+
+    val sb = StringBuilder()
+
+    // Title using the primary position
+    val posString = posStr(pos1, msg, diagnosticLevel(dia))
+    if posString.nonEmpty then sb.append(posString).append(EOL)
+
+    // Display leading text if present
+    msg.leading.foreach { leadingText =>
+      sb.append(leadingText)
+      if !leadingText.endsWith(EOL) then sb.append(EOL)
+    }
+
+    // Track the current file
+    // When starting, we set it to the file of the diagnostic
+    var currentFile: SourceFile = pos1.source
+
+    // Render each group of parts
+    for (source, parts) <- groupedParts do
+      // Add a file indicator line when switching to a different file
+      if source != currentFile then
+        sb.append("... ").append(renderPath(source.file)).append(EOL)
+        currentFile = source
+      renderPartsForFile(parts, sb)
 
     // Add explanation if needed
     if Diagnostic.shouldExplain(dia) then
-      sb.append(EOL).append(newBox())
+      sb.append(newBox())
       sb.append(EOL).append(offsetBox).append(" Explanation (enabled by `-explain`)")
       sb.append(EOL).append(newBox(soft = true))
       dia.msg.explanation.split(raw"\R").foreach: line =>
         sb.append(EOL).append(offsetBox).append(if line.isEmpty then "" else " ").append(line)
       sb.append(EOL).append(endBox)
     else if dia.msg.canExplain then
-      sb.append(EOL).append(offsetBox)
+      sb.append(offsetBox)
       sb.append(EOL).append(offsetBox).append(" longer explanation available when compiling with `-explain`")
 
     sb.toString
diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -1212,6 +1212,30 @@ extends DeclarationMsg(OverrideErrorID), NoDisambiguation:
   def explain(using Context) =
     if canExplain then err.whyNoMatchStr(memberTp, otherTp) else ""
 
+  /** Whether the overridden symbol has a valid source position */
+  private def otherHasValidPos(using Context): Boolean =
+    val otherPos = other.sourcePos
+    otherPos.exists && otherPos.source.file.exists
+
+  override def leading(using Context): Option[String] =
+    if otherHasValidPos then Some(message) else None
+
+  override def parts(using Context): List[Message.MessagePart] =
+    if otherHasValidPos then
+      List(
+        Message.MessagePart(
+          i"the overridden ${other.showKind} is here",
+          other.sourcePos,
+          isPrimary = false
+        ),
+        Message.MessagePart(
+          i"the overriding ${member.showKind} $core",
+          member.sourcePos,
+          isPrimary = true
+        )
+      )
+    else Nil
+
 class ForwardReferenceExtendsOverDefinition(value: Symbol, definition: Symbol)(using Context)
 extends ReferenceMsg(ForwardReferenceExtendsOverDefinitionID) {
   extension (sym: Symbol) def srcLine = sym.line + 1
diff --git a/tests/neg/override-multi-file-error.check b/tests/neg/override-multi-file-error.check
@@ -0,0 +1,13 @@
+-- [E164] Declaration Error: tests/neg/override-multi-file-error/B.scala:2:15 ------------------------------------------
+error overriding method foo in class A of type => Int;
+  method foo of type => String has incompatible type
+... tests/neg/override-multi-file-error/A.scala
+2 |  def foo: Int = 0
+  |      -
+  |      the overridden method is here
+... tests/neg/override-multi-file-error/B.scala
+2 |  override def foo: String = "0"   // error
+  |               ^
+  |               the overriding method has incompatible type
+  |
+  | longer explanation available when compiling with `-explain`
diff --git a/tests/neg/override-multi-file-error/A.scala b/tests/neg/override-multi-file-error/A.scala
@@ -0,0 +1,3 @@
+class A:
+  def foo: Int = 0
+
diff --git a/tests/neg/override-multi-file-error/B.scala b/tests/neg/override-multi-file-error/B.scala
@@ -0,0 +1,3 @@
+class B extends A:
+  override def foo: String = "0"   // error
+
diff --git a/tests/neg/override-multi-span.check b/tests/neg/override-multi-span.check
@@ -0,0 +1,14 @@
+-- [E164] Declaration Error: tests/neg/override-multi-span.scala:6:15 --------------------------------------------------
+error overriding method foo in class A of type => Int;
+  method foo of type => String has incompatible type
+2 |  def foo: Int = 0
+  |      -
+  |      the overridden method is here
+3 |def main(): Unit =
+4 |  println("Hello")
+5 |class B extends A:
+6 |  override def foo: String = "0"  // error
+  |               ^
+  |               the overriding method has incompatible type
+  |
+  | longer explanation available when compiling with `-explain`
diff --git a/tests/neg/override-multi-span.scala b/tests/neg/override-multi-span.scala
@@ -0,0 +1,6 @@
+class A:
+  def foo: Int = 0
+def main(): Unit =
+  println("Hello")
+class B extends A:
+  override def foo: String = "0"  // error

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+class A:`
	`2`	`+ def foo: Int = 0`
	`3`	`+`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+class B extends A:`
	`2`	`+ override def foo: String = "0" // error`
	`3`	`+`