Conversation
ckipp01
left a comment
There was a problem hiding this comment.
Thanks a lot for creating this @eed3si9n! Although I didn't even think it was necessary. Since data is sort of arbitrary I sort of assumed that when the BSP server got the Problem it would convert everything into the Diagnostic and the data part would just be serialized and forwarded. If it needed anything more or needed to put together it's own action, it would just use the interface from sbt util. I didn't think we'd need to define all these structures in here as well, but instead have build clients just rely on the sbt util interfaces to deserialize. For example we sort of did that with scala-cli with the ability to update dependencies.
I realize that this is probably safer, but it does cause us to have to duplicate everything as well.
adpi2
left a comment
There was a problem hiding this comment.
I realize that this is probably safer, but it does cause us to have to duplicate everything as well.
@ckipp01 Unfortunately you are right.
However, I also think it is important to document the interface in BSP, because it makes it known to all the parties (sbt, Mill, Metals, IJ) through the website and the libraries. It can inspire other languages too.
As a side note, having BSP used to transmit info from a single build tool (Scala CLI) to a single server (Metals) is not ideal. It would be good to have it documented in BSP as well.
**Problem** We want to pass along code actions via Diagnostics. **Solution** This implements `ScalaDiagnostic`, intended to be used as the `data` field on `Diagnostic`. `ScalaDiagnostic` contains `actions` fields, mirroring the corresponding Zinc change, and mostly based on CodeAction in LSP.
ckipp01
left a comment
There was a problem hiding this comment.
Just a nit on naming, but other than that, LGTM!
|
@jastice Can we merge this? It is kind of blocking sbt/sbt#7264 that we would like to merge in the soon-to-be-released sbt 1.9. |
|
It's fine with me. My only concern would be that it might blow up the amount of data produced on full builds, but I think it can be a matter of server configuration whether to turn it on (if it's a problem at all) |
### What changes were proposed in this pull request? The pr aims to upgrade sbt from 1.9.2 to 1.9.3. ### Why are the changes needed? 1.The new version brings some improvment: Actionable diagnostics (aka quickfix) Actionable diagnostics, or quickfix, is an area in Scala tooling that's been getting attention since Chris Kipp presented it in the March 2023 Tooling Summit. Chris has written the [roadmap](https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172/1) and sent sbt/sbt#7242 that kickstarted the effort, but now there's been steady progress in build-server-protocol/build-server-protocol#527, scala/scala3#17337, scala/scala#10406, IntelliJ, Zinc, etc. Metals 1.0.0, for example, is now capable of surfacing code actions as a quickfix. sbt 1.9.3 adds a new interface called AnalysisCallback2 to relay code actions from the compiler(s) to Zinc's Analysis file. Future version of Scala 2.13.x (and hopefully Scala 3) will release with proper code actions, but as a demo I've implemented a code action for procedure syntax usages even on current Scala 2.13.11 with -deprecation flag. 2.Full release notes: https://github.com/sbt/sbt/releases/tag/v1.9.3 3.v1.9.2 VS v1.9.3 sbt/sbt@v1.9.2...v1.9.3 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass GA. Closes #42141 from panbingkun/SPARK-44536. Authored-by: panbingkun <pbk1982@gmail.com> Signed-off-by: yangjie01 <yangjie01@baidu.com>
Ref https://contributors.scala-lang.org/t/roadmap-for-actionable-diagnostics/6172
Problem
We want to pass along code actions via Diagnostics.
Solution
This implements
ScalaDiagnostic, intended to be used asthe
datafield onDiagnostic.ScalaDiagnosticcontainsactionsfields, mirroring the correspondingZinc change (sbt/sbt#7242), and mostly based on CodeAction in LSP.