Shrink range of diagnostics to prevent them from hiding other diagnostics

[adpi2]

Scala version: 2.13.8

Motivation

Some diagnostics span the entire body of, for instance, a class or method. In most cases I think we should shrink the range of the diagnostics to the declared identifier of the class or method. My arguments are:

• Spanning the entire body does not bring much information
• The bigger range can hide smaller ones. This compromise the readability of reports in IDEs.
• In Scala 3, some ranges have already been shrunk.

A typical example of a diagnostic that can be huge and hide many other diagnostics is the missing implementation. Here it spans 400 lines of code.

I give below a non-exhaustive list of diagnostics, with reproduction steps, that could potentially be shrunk.

Missing implementation

package example

trait A {
  def x: String
}

class B[+T] extends A {
  def y(t: T): Unit = ()
}

In this example the "Missing implementation" warning hides a "covariant type T occurs in contravariant position".

In Scala 3, the range only contains the identifier of the class.

Unused method

// build.sbt
scalaVersion := "2.13.8"
scalacOptions += "-Wunused:_"

package example

class UnusedMethod {
  private def m: String = {
    List(1) match {
      case Nil => "nil"
    }
  }
}

Here the "Unused method" warning hides the "match may not be exhaustive warning". Again the range should only contain the identifier.

[som-snytt]

scalac does no hiding here.

t12735.scala:6: error: class D needs to be abstract.
Missing implementation for member of class C:
  def f: Int = ???

class D[+A] extends C {
      ^
t12735.scala:7: error: covariant type A occurs in contravariant position in type A of value a
  def g(a: A) = ()
        ^
2 errors

I tried to add a definition which would demonstrate why the span might be useful:

abstract class C {
  def f: Int
}

class D[+A] extends C {
  def g(a: A) = ()
}
  def f: Int = 42

but dotty only reports:

-- Error: t12735.scala:7:8 ---------------------------------------------------------------------------------------------
7 |  def g(a: A) = ()
  |        ^^^^
  |        covariant type A occurs in contravariant position in type A of parameter a
1 error found

[adpi2]

scalac does no hiding here.

So yes, both errors are reported. The problem is that the nested one is visually not visible from the code editor.

In this example the variance error is reported but I cannot see it directly, without reading the full list of problems. It would be nicer if the compiler reports disjoint ranges.

but dotty only reports:

Yes I guess the "Missing implementation" happens in a later phase in Dotty. But my point is the range of the "Missing implementation" error is the identifier in Dotty and not the full definition.

[error] -- Error: /home/piquerez/github/scalameta/metals-fr-330/src/main/scala/example/MissingImpl.scala:7:6 
[error] 7 |class B extends A {
[error]   |      ^
[error]   |class B needs to be abstract, since def x: String in trait A in package example is not defined 
[error] one error found

[som-snytt]

Without giving it much thought, I am not immediately persuaded that error spans must be disjoint.

First, my simple counterexample seems reasonable: "but I define f right there! oh, but I can see that it is outside the definition (because it shows me the span of the definition)."

Perhaps (again, without giving it much thought) vertical spans should be indicated with a vertical line at the left gutter, where nested or overlapping spans are indicated by multiple parallel vertical lines; and horizontal spans by red squiggled underlining.

An editor may arbitrarily choose to highlight the identifier under the point (B) or the signature under the point (class B[+T] extends A), or what have you. In this case, the A is significant, as it introduces the declaration, as much as the B which fails to define it. Also the editor may choose to hide overlapping spans, or present problems in "bottom-up" order.

class B[+T] extends A

It seems to me that this is a presentation problem.

Maybe diagnostics could be stuffed with more structure info, such as "identifier under point", etc, that clients can consume.

[adpi2]

Perhaps (again, without giving it much thought) vertical spans should be indicated with a vertical line at the left gutter, where nested or overlapping spans are indicated by multiple parallel vertical lines; and horizontal spans by red squiggled underlining.

Well that could work, but unfortunately I don't think it would be easy to get such features implemented in our IDEs and code editors. At least, I did not find any related discussion in VSCode backlog. I would be curious to know which other languages can have nested errors, or even large multiline errors.

Back to the Scala ecosystem, it seems that sbt and IntelliJ choose to not expose the full range:

• sbt prints:

  [error] /home/piquerez/github/scalameta/metals-fr-330/src/main/scala/example/MissingImpl.scala:7:7: class B needs to be abstract. Missing implementation for:
  [error]   def x: String // inherited from trait A
  [error] class B[+T] extends A {
  [error]       ^

• IntelliJ shows:

Maybe diagnostics could be stuffed with more structure info, such as "identifier under point", etc, that clients can consume.

If we have more structure info that we can consume in Metals that would be great, but then I wonder why we would have the full range if no-one exposes it to the user.

About my second example, the "Unused method" warning, I really don't think that showing the full body is any useful. Because in this case, it is clearly the identifier that is never used.

[tgodzik]

If we have more structure info that we can consume in Metals that would be great, but then I wonder why we would have the full range if no-one exposes it to the user.

:100:

It's also important difference for LSP/BSP is that we only have range of the diagnostic, we don't get point which sbt and Intellij (unless someone uses BSP with Intellij) most likely uses.

[som-snytt]

Dotty tracks name position, so the PR does the same. The position of the symbol of a tree which is a named member is the name position. The test shows the error position includes just the name.