feat: hook up actions in sbt-bridge to start forwarding actionable diagnostics

[ckipp01]

There are still a few things I need to add/test fully before this is ready for a full review, but I thought I'd push it up just for others to take a look at or discuss if need be. The background for this is in here, but this uses the POC that @smarter did at the tooling summit with the new structure that we agreed on. Some things I still need to test/work on:

• doing a full e2e with Metals to ensure this is working
• add in a new test suite that tests actions for a given piece of code. Right now I do an integration-like test in sbt-test just to ensure that the action is being forwarded, but I'd like to have a more robust test suite introduced so we can ensure that code snippets produce the expected actions and that the resulting code is what we'd expect.

So far this contains actions for:

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

refs: #17337

[ckipp01]

Here are some examples using sbt 1.9.0 and Metals showing the BSP trace that you'd expect:

For this code:

package example

object Hello:
  def x: Int = 3
  val test = x _

The diagnostic that is forwarded to the editor looks like:

[Trace - 10:29:20 am] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala"
  },
  "buildTarget": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/actionable-diagnostic/#root/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 4,
          "character": 13
        },
        "end": {
          "line": 4,
          "character": 16
        }
      },
      "severity": 1,
      "code": "99",
      "source": "sbt",
      "message": "Only function types can be followed by _ but the current expression has type Int",
      "relatedInformation": [],
      "dataKind": "scala",
      "data": {
        "actions": [
          {
            "title": "Rewrite to function value",
            "edit": {
              "changes": [
                {
                  "range": {
                    "start": {
                      "line": 4,
                      "character": 13
                    },
                    "end": {
                      "line": 4,
                      "character": 13
                    }
                  },
                  "newText": "(() \u003d\u003e "
                },
                {
                  "range": {
                    "start": {
                      "line": 4,
                      "character": 14
                    },
                    "end": {
                      "line": 4,
                      "character": 16
                    }
                  },
                  "newText": ")"
                }
              ]
            }
          }
        ]
      }
    }
  ],
  "reset": true
}

For this code:

package example

object Hello:
  def foo(): Unit = ()
  val x = foo

The trace:

[Trace - 10:32:47 am] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala"
  },
  "buildTarget": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/actionable-diagnostic/#root/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 4,
          "character": 10
        },
        "end": {
          "line": 4,
          "character": 13
        }
      },
      "severity": 1,
      "code": "100",
      "source": "sbt",
      "message": "\u001b[33mmethod\u001b[0m \u001b[35mfoo\u001b[0m in \u001b[33mobject\u001b[0m \u001b[35mHello\u001b[0m must be called with () argument",
      "relatedInformation": [],
      "dataKind": "scala",
      "data": {
        "actions": [
          {
            "title": "Insert ()",
            "edit": {
              "changes": [
                {
                  "range": {
                    "start": {
                      "line": 4,
                      "character": 13
                    },
                    "end": {
                      "line": 4,
                      "character": 13
                    }
                  },
                  "newText": "()"
                }
              ]
            }
          }
        ]
      }
    }
  ],
  "reset": true
}

For this code:

package example

final final class Test

The trace:

[Trace - 10:34:15 am] Received notification 'build/publishDiagnostics'
Params: {
  "textDocument": {
    "uri": "file:///Users/ckipp/Documents/scala-workspace/actionable-diagnostic/src/main/scala/example/Hello.scala"
  },
  "buildTarget": {
    "uri": "file:/Users/ckipp/Documents/scala-workspace/actionable-diagnostic/#root/Compile"
  },
  "diagnostics": [
    {
      "range": {
        "start": {
          "line": 2,
          "character": 12
        },
        "end": {
          "line": 2,
          "character": 17
        }
      },
      "severity": 1,
      "code": "15",
      "source": "sbt",
      "message": "Repeated modifier final",
      "relatedInformation": [],
      "dataKind": "scala",
      "data": {
        "actions": [
          {
            "title": "Remove repeated modifier: \"final\"",
            "edit": {
              "changes": [
                {
                  "range": {
                    "start": {
                      "line": 2,
                      "character": 6
                    },
                    "end": {
                      "line": 2,
                      "character": 11
                    }
                  },
                  "newText": ""
                }
              ]
            }
          }
        ]
      }
    }
  ],
  "reset": true
}

[ckipp01]

Also here is some eye candy showing them all work in Metals:

[smarter]

In https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172/13 @eed3si9n discussed the semantics of changes, it'd be great to have tests in both Scala 2 and Scala 3 to ensure we agree here. Also IMO we should absolutely match what the LSP specifies if we don't want to end up with incorrect implementations in LSP servers.

[smarter]

This duplicates the logic with patch(...) calls at the use-site of OnlyFunctionsCanBeFollowedByUnderscore, I think having the actions in the message like this makes a lot of sense but we need to drop the old patch mechanism at the same time otherwise we'll be in a weird situation where -rewrite and IDEs each do their own thing.

[ckipp01]

Good call. So what I did here was that I ended up making the patches inside of the message like I was before, but now instead of putting patches together like this outside of the message, we instead create the message and then just grab the patch out of the message. This still isn't idea and something we talked about this morning was completely refactoring the way that patches are applied by waiting until the reporting happens and during that time, check if the message that is being reported has an action, and if so, apply it, which contains the patch. However, thinking about this a bit more this afternoon @smarter, one limitation of that approach is that many places where we do patches, messages don't exist. In fact, the vast majority of the places that we call patch, there is no Message for that. So in order for that to work we'd really want to create a unique Message for it that would hold that actions/patch.

[smarter]

I guess we could keep patch around for formatting changes (braces <-> indent) since flooding the console with messages for those changes wouldn't be useful.

[ckipp01]

In https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172/13 @eed3si9n discussed the semantics of changes, it'd be great to have tests in both Scala 2 and Scala 3 to ensure we agree here. Also IMO we should absolutely match what the LSP specifies if we don't want to end up with incorrect implementations in LSP servers.

Yup, so I just created a comment in here because currently we actually differ. The Span that is used in the Patch is actually end exclusive, which currently differs from the PR in Scala 2. So we'll need to align there.

[smarter]

Otherwise LGTM!

[ckipp01]

Alright, I made the necessary adjustments so I'm going to go ahead and pull the trigger! Hopefully moving forward this will allow for more and more of these to be integrated.