Roadmap for actionable diagnostics

Scala Center Updates
1

Hey everyone. Recently at the Scala Tooling Summit there was a large discussion around the topic of more structured diagnostics. You can also read the previous discussion that started this effort
here. Since there seems to be pretty widespread support in this effort I’d like to take a moment an outline what has been done up to this point, and outline a roadmap for this effort moving forward, specifically focusing on actionable diagnostics.

Where we’re at

There was originally an issue in Dotty tracking part of this work introducing the DiagnosticCode and DiagnosticRelatedInformation. As you can see in the issue, the DiagnosticCode has been added all throughout the chain, and the DiagnosticRelatedInformation has been introduced throughout the tool chain, but not yet added to the compiler. The work in that area will continue and be tracked on the existing issue, while this thread will outline the work set to be done regarding actionable diagnostics and utilizing the data field.

What is an actionable diagnostic

An actionable diagnostic is a diagnostic that provides a “fix” for something that the user is experiencing. A great example of this already existing in Scala is when a user is using scala-cli and they have an outdated dependency. In your editor you’ll get an info diagnostic with a “fix” attached to it that they user can trigger to update their dependency. While this fix originates from scala-cli, the idea here would be for the compiler to offer fixes for things it knows how to fix. These would then be forwarded through the compiler → Build tool → BSP → editor chain. You can see an example of this working with the compiler and IntelliJ here. This was a POC done during the tooling summit to show the idea in action.

What’s next

At this point I’d like to outline the steps necessary to start emitting these types of fixes from the compiler. While the necessary things exist from BSP onwards already, we need to prepare the various build servers, sbt bridge, and compiler to start emitting and forwarding them.

Agreeing on a structure.

While scala-cli was already using their own structure of an LSP [TextEdit (Specification), it was being used at the root level. In order to future proof this and allow for more potential fields under the data key we decided to nest this under an edits key. That key would contain an array of LSP WorkspaceEdits instead of TextEdits to further allow for more types of refactorings in the future such as new files being created, moved, or deleted. You can see this new structure in scala-cli and the changes necessary in Metals to handle this new structure here. While this structure works well for BSP onwards, it is quite a complex representations with lots of possibilities. The POC that was done at the summit used a TextEdit representation at the Problem level and re-used the Patch structure from the compiler. I think using the Patch that already exists in the compiler to be wise, but the question then becomes whether or not TextEdit really contains enough possibilities, or if the representation inside of Problem should allow for more information.

Outlining the steps

  • agree on the structure that belongs in Problem.
  • make the necessary additions to Problem.
  • once Problem is merged and released, update zinc to use it
  • once zinc is released update Dotty to use the latest sbt/zinc
  • start utilizing the new data field in Problem and having the compiler forward “fixes”
  • ensure that tools like Metals and IntelliJ are fully able to utilize these.

The Scala Center team is dedicated to providing regular and transparent
community updates about project plans & progress. In this forum we created a new
topic category to allow quicker orientation going forward: “Scala Center
Updates”. Even though all feedback is welcome, we keep the right to make
executive decisions about projects we lead. Overview of all our activities can
always be found at https://scala.epfl.ch/records.html
.

18 Likes
2

This is a splendid initiative, big thanks to everybody involved!

An important tool that I haven’t seen mentioned is Scalafix. What role in this linting/fixing scheme does (or will) Scalafix play?

1 Like
3

We haven’t discussed Scalafix, but it’s not really related to the effort here because it’s a separate API. However, I think it would great to integrate Metals/Intellij with Scalafix for linting, so that use defined rules could be applied.

On the other hand, the compiler will be able to provide actionable diagnostics for any of the linting options it provides, so some of the Scalafix rules might become redundant in the quick fix aspect.

1 Like
4

Small update on the structure for this. Originally I went down the path of using an array of WorkspaceEdits. You can see some of those changes in scala-cli and in Metals. To keep it simple it seemed at first that it’s work fine to just utilize the changes field in the WorkspaceEdit, but there is a neat improvement to LSP 3.16, ChangeAnnotations which allow you to group certain edits together. This would allow for the use of AnnotatedTextEdits. While this sounds good, there is a couple issues here:

  • Apart from VS Code, it doesn’t seem like other clients actually support them. While this may change since it’s newer, it is currently a limitation.
  • More importantly, the ChangeAnnotation is associated with a single edit, not the entire WorkspaceEdit which sort of defeats the purpose of using them as a way to label specific edits. The original idea would be that a single Diagnostic could hold multiple changes. However in order for that to you work you need to be able to differential them on the client side.

Example of what you don’t want where the labels are exactly the same

2023-05-01 14.05.13

What you want (currently what IntelliJ has)

image

The reason the above exists the way it does is that we just take part of the diagnostic message and use that as the title of the code action.

Because the annotations are included at the individual edit level and not at the workspace edit level, that leaves us with a decision to make:

  • Do we just create our own format for this that is similiar to a WorkspaceEdit, but only includes the minimal parts that we’d need with an additional field for a title and a description at the upper level? Meaning something like this:
export interface ScalaWorkspaceEdit {
	/**
	 * All clients start off supporting `changes` here so this would be widely supported. 
     * Note that this wouldn't include the `ResourceOperation`s that would be included `documentChanges`.
	 */
	changes?: { [uri: DocumentUri]: TextEdit[]; };

     /** Title to be shown to the user for this workspace edit */
    title: String;

    /** Optional description that could be shown if the client supports it. **/
    description?: String;
}

The title could then be used to set the codeAction title and the changes could simple be forwarded in the codeAction.changes.

Later on if we’d want to support the ResourceOptions, then we could add in the documentChanges key as well.

Any thoughts on this?

1 Like
5

I don’t think so: as far as I can tell, Multiple AnnotatedTextEdit can have the same ChangeAnnotationIdentifier, and changeAnnotations in WorkspaceEdit maps a ChangeAnnotationIdentifier to a ChangeAnnotation. I don’t know what the UI for this looks like in vscode though.

6

Correct, but these would group them per workspace edit, which isn’t really what we want the label for. For example in the context of a code action (which is what would be used in LSP), the action has a single edit, which is a WorkspaceEdit. Within that edit we really don’t care about the grouping, but we care about ensuring that each individual code action has a distinguishable title.

Just to give an idea of what this would look like in a code action, here was something I was testing today:

it’s a bit nonsensical, but again, just an example

[Trace - 02:55:41 pm] Sending response 'textDocument/codeAction - (11)'. Processing request took 54ms
Result: [
  {
    "title": "Apply suggestion: \"pprint is outdated, update to 0.8.1\"",
    "kind": "quickfix",
    "diagnostics": [...],
    "edit": {
      "changes": {},
      "documentChanges": [
        {
          "textDocument": {
            "version": 0
          },
          "edits": [
            {
              "annotationId": "update",
              "range": {
                "start": {
                  "line": 1,
                  "character": 15
                },
                "end": {
                  "line": 1,
                  "character": 40
                }
              },
              "newText": "com.lihaoyi::pprint:0.8.1"
            }
          ]
        }
      ]
    }
  },
  {
    "title": "Apply suggestion: \"pprint is outdated, update to 0.8.1\"",
    "kind": "quickfix",
    "diagnostics": [
      {
        "range": {
          "start": {
            "line": 1,
            "character": 0
          },
          "end": {
            "line": 1,
            "character": 41
          }
        },
        "severity": 4,
        "source": "scala-cli",
        "message": "\"pprint is outdated, update to 0.8.1\"\n     pprint 0.8.0 -\u003e com.lihaoyi::pprint:0.8.1",
        "data": {
          "edits": [
            {
              "changes": {
                "file:///Users/ckipp/Documents/scratch-workspace/actionable-diagnostic/Main.scala": [
                  {
                    "annotationId": "update",
                    "range": {
                      "start": {
                        "line": 1,
                        "character": 15
                      },
                      "end": {
                        "line": 1,
                        "character": 40
                      }
                    },
                    "newText": "com.lihaoyi::pprint:0.8.1"
                  }
                ]
              },
              "changeAnnotations": {
                "update": {
                  "label": "Update the dep"
                }
              }
            },
            {
              "changes": {
                "file:///Users/ckipp/Documents/scratch-workspace/actionable-diagnostic/Main.scala": [
                  {
                    "annotationId": "remove",
                    "range": {
                      "start": {
                        "line": 1,
                        "character": 15
                      },
                      "end": {
                        "line": 1,
                        "character": 40
                      }
                    },
                    "newText": ""
                  }
                ]
              },
              "changeAnnotations": {
                "remove": {
                  "label": "Remove the dep"
                }
              }
            }
          ]
        }
      }
    ],
    "edit": {
      "changes": {},
      "documentChanges": [
        {
          "textDocument": {
            "version": 0
          },
          "edits": [
            {
              "annotationId": "remove",
              "range": {
                "start": {
                  "line": 1,
                  "character": 15
                },
                "end": {
                  "line": 1,
                  "character": 40
                }
              },
              "newText": ""
            }
          ]
        }
      ]
    }
  }
]

So this example would have two independent workspace edits with a single edit each. I can’t really think of a situation where within a single workspace edit we wouldn’t want to apply all of the edits. I also can’t figure out what this looks like because for some reason in the above scenario it shows that there are no code actions available. Why? I’m not really sure. Once I switched over to documentChanges from changes everything got more convoluted and I paused because I don’t think we need this level of granularity with annotations. I’d rather just stick with the edits/resource operations and add in our own title/description at the workspace edit level that can be used when the code action is put together. It’s more minimal, would be supported by all the clients (instead of just one), and would still allow for as rich of a feature set as we need.

1 Like
7

I think we shouldn’t use ChangeAnnotation if that indeed is not supported by any other client than VS Code and it seems it’s much more complicated than needed.

So currently you are thinking about having a WorkspaceEdit enriched with a title for BSP or zinc’s Problem? This would essentially be a simplified CodeAction, right?

8

Exactly. So given the situation up above where we had two seperate actions from one another the data field coming with the Diagnostic would look like this:

{
  "data": {
    "actions": [
      {
        "title": "Apply suggestion: \"pprint is outdated, update to 0.8.1\"",
        "edit": {
          "changes": {
            "file:///Users/ckipp/Documents/scratch-workspace/actionable-diagnostic/Main.scala": [
              {
                "range": {
                  "start": {
                    "line": 1,
                    "character": 15
                  },
                  "end": {
                    "line": 1,
                    "character": 40
                  }
                },
                "newText": "com.lihaoyi::pprint:0.8.1"
              }
            ]
          }
        }
      },
      {
        "title": "Remove pprint as a dependency",
        "edit": {
          "changes": {
            "file:///Users/ckipp/Documents/scratch-workspace/actionable-diagnostic/Main.scala": [
              {
                "range": {
                  "start": {
                    "line": 1,
                    "character": 15
                  },
                  "end": {
                    "line": 1,
                    "character": 40
                  }
                },
                "newText": ""
              }
            ]
          }
        }
      }
    ]
  }
}

The schema for this would then basically be:

export interface ScalaAction {
	title: string;
    description?:  string;
	/**
	 * This would be a WorkspaceEdit according to the LSP spec, however to start we
     * would only be supporting "changes". This should cover all the initial use-cases
     * we thought of while also allowing us then to add in the resource operations later.
	 */
	edit: WorkspaceEdit;
}

Again, this is prretty minimal, but also the core of what we actually need. Later on we’d be able to dig into documentChanges which would allow for resource operations, but again I think this should be fine to start out with and would cover the vast majority of use-cases that we had in mind.

If we agree on this, I can shoot in a pr to Problem in sbt-interfaces to get the ball rolling on this and also update the scala-cli/metals prs to this as well.

9

I think this covers most of the use cases I can think of. And I wouldn’t complicate it more than that. We want to encourage people to write actionable diagnostics inside the compiler and outside if we provide access to it in the macros.

3 Likes
10

Alrighty, I created feat: add in `actions()` to `Problem` by ckipp01 · Pull Request #7242 · sbt/sbt · GitHub if you have a moment to take a look.

1 Like
11

I’ve created related Scala 2.x proposal - Actionable diagnostic support · Issue #844 · scala/scala-dev · GitHub.

2 Likes
12

Here’s my PR Implement CodeAction (actionable diagnostic) by eed3si9n · Pull Request #10406 · scala/scala · GitHub. This implements CodeActions to:

  • Deprecation of procedure syntax
  • Deprecation of empty-paren method auto-application
  • Deprecation of val in for-comprehension
  • Deprecation of paren-less lambda parameter
3 Likes
13

Those who might not be directly concerned about Scala 2.13.x might also still be interested in reviewing the above PR since I am effectively specifying how Action should be interpreted given a position:

  def applyChange(code: String, a: CodeAction): String = {
    val change = a.edit.changes.head
    val head =
      if (change.position.isRange) code.take(change.position.start)
      else code.take(change.position.point)
    // end is inclusive, so you have to drop one more character here
    val tail =
      if (change.position.isRange) code.drop(change.position.end + 1)
      else code.drop(change.position.point)
    head + change.newText + tail
  }

In other words, given a string "abc", we need to come up with an agreed upon TextEdit(???, "???") to turn it into:

  • "abdc"
  • "ac"

for Scala. In Scala 2 at least, the position is end-inclusive, unlike LSP range right?

14

This is awesome @eed3si9n!

Those who might not be directly concerned about Scala 2.13.x might also still be interested in reviewing the above PR since I am effectively specifying how Action should be interpreted given a position:

I’ll take a closer look at this tomorrow to make sure it aligns with the work I’ve been in with Scala 3 as well. You can see my progress on the PR here. The ones I started with were:

  • converting to a function value
  • inserting parens for empty argument
  • removing duplicated modifiers

And I have a handful more that I’ll be adding.

15

If you want a little eye candy, you can see the first few actions in action here.

1 Like
16

Small meta-remark: Please don’t use GIFs. Use WebM.

GIFs aren’t accessible (you can’t pause them for example), they’re extremely large and have often poor quality at the same time.

Besides that, thank you for the cool work! :smiley:

17

Sorry I glossed over this before and totally forgot to circle back around. So my implementation in here under the hood ends up relying on Spans which are actually end-exclusive as you can see in this little example:

//> using scala 3.3.0
//> using lib org.scala-lang::scala3-compiler:3.3.0

import dotty.tools.dotc.util.Spans.Span

@main def test() =

  val span = Span(3, 7)

  println(span.start)
  println("---------")
  println(span.end)
  println("---------")

  val range = span.start until span.end

  for (i <- range)
    println(i)

Which prints out

3
---------
7
---------
3
4
5
6

The reason I went with that is because at least in Scala 3 the Span is also what is used in that same way for the rewrite functionality and I didn’t want to change the semantics between when they were used via the IDE and when they were used when rewriting with the compiler. Plus, this way ends up being a bit easier to simply translate to the LSP range without having to worry about that difference.

In Scala 2 with yours being end inclusive, is there a reason you went that route?

18

Zinc xsbti.position is a wrapper around Scala 2.x position, which is capable of expressing range for -Yrangepos (on by default since 2.13.4?), and I think it’s end-inclusive.

19

So how should we proceed here? I’m guessing neither of us really want to change our implementations :smile: so what would you propose?

Going off the last comment from @smarter of

Also IMO we should absolutely match what the LSP specifies if we don’t want to end up with incorrect implementations in LSP servers.

On my PR I’m inclined to leave it as is since it does match the current LSP implementation and everything works pretty smoothly going from compiler to editor. Btw I just merged in the metals pr so the latest snapshot should also provide a way to play around with this for you with sbt/scala 2 if you’d like.

1 Like
20

Sorry for the late reply. Range positions in Scala 2 are end-exclusive, see here, or the following using current 2.13.x:

scala> :power
scala> class ABC { val cde = 0 }
scala> typeOf[ABC].typeSymbol.pos
val res0: $r.intp.global.Position = RangePosition(<console>, 6, 6, 9)

I just updated your PR with a few corresponding adjustments.

2 Likes