Fixes CodeAction (quickfix) support #1226
Conversation
| * @param diagnosticRelatedInformation The possible releated information for the diagnostic being reported. | ||
| * @param actions Actions (aka quick fixes) that are able to either fix or address the issue that is causing this problem. | ||
| */ | ||
| void problem2(String what, |
There was a problem hiding this comment.
Is this something that the Scala 3 bridge also needs to call? I thought we had all the pieces in place already /cc @ckipp01
Also couldn't this interface take an xsbti.Problem so we don't need a new one every time there's a new field?
There was a problem hiding this comment.
Nope, I don't believe this is necessary for Scala 3. These changes were specific to the Scala 2 implementation. You can actually test the actions already with the nightly version of the compiler and the latest metals release and see that everything is working as expected in Scala 3.
There was a problem hiding this comment.
But the Scala 3 bridge does call problem: https://github.com/lampepfl/dotty/blob/ce1ce99b6b2e67d4d2ca05f6732e3eb0ee692f1f/sbt-bridge/src/dotty/tools/xsbt/CompilerBridgeDriver.java#L139
Doesn't that need to become problem2 too?
There was a problem hiding this comment.
Huh, I guess it would seem so. I guess I don't fully understand how this is all working or when this run is called that you have linked since at least via BSP with sbt current the actions are forwarded as expected.
There was a problem hiding this comment.
You can actually test the actions already with the nightly version of the compiler and the latest metals release and see that everything is working as expected in Scala 3.
I confirmed this
There was a problem hiding this comment.
sbt uses xsbi.Problem in 2 different ways:
- via
xsbi.Reporterwhich you get a live callback per compiler warnings - via
CompileAnalysis,ReadSourceInfos,SourceInfo, which contains list ofxsbti.Problem
This bug fix pertains the second pathway. I think the initial implementation of LSP or BSP use to resend all errors and warnings for all files at the end of compile as a workaround to make sure compiler warnings do not disappear, then in 2022 a few folks realized we're bombarding Metals with unnecessary problems, and we got fixes:
- [BSP]Do not fire
build/publishDiagnosticsif there are (and were) no problems sbt#6847 by @tanishiking - fix: publish BSP diagnostics when it's necessary sbt#6929 by @kpodsiad
I usually ask @adpi2 to figure out the BSP handling, so I only occasionally jump in so I am not sure if this double dipping is unavoidable, but as a principle, it seems like a good idea that the problems we collect in Analysis contains new problem informations (like CodeAction and DiagnosticRelatedInformation).
There was a problem hiding this comment.
it seems like a good idea that the problems we collect in Analysis contains new problem informations (like CodeAction and DiagnosticRelatedInformation).
Agreed. In the BSP reporter, we still use the CompileAnalysis to report the warnings from the files that the incremental compiler skipped. It's good to have the code actions in there.
| * Scala 3 compiler publishes their own compiler bridge against Zinc de jour. | ||
| * By having this interface, they can test if `callback` supports AnalysisCallback2. |
There was a problem hiding this comment.
How do you envision this working? If the dotty bridge starts doing isInstanceOf[AnalysisCallback2] we'll get a class not found exception if we're running against an older sbt/zinc, so we'd need to catch the exception I guess?
There was a problem hiding this comment.
Yea, I guess something like that. I figured it's better than just adding a new method.
There was a problem hiding this comment.
I guess we can work with that, but it'd be great if we could coordinate on this sort of things before sbt releases in the future so we can take the time to come up with the best way forward, for example if we had gone with a problem that takes an xsbti.Problem we could have ensured we don't need another interface like this next time we add a field to xsbti.Problem.
There was a problem hiding this comment.
Yea, I wanted to unblock the development on this, but I guess in the hindsight I should've waited for reviews.
Fixes #1225
This adds
AnalysisCallback2interface that's capable of forwarding CodeAction to the Analysis file.