-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes CodeAction (quickfix) support #1226
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
/* | ||
* Zinc - The incremental compiler for Scala. | ||
* Copyright Scala Center, Lightbend, and Mark Harrah | ||
* | ||
* Licensed under Apache License 2.0 | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* See the NOTICE file distributed with this work for | ||
* additional information regarding copyright ownership. | ||
*/ | ||
|
||
package xsbti; | ||
|
||
import java.util.List; | ||
import java.util.Optional; | ||
|
||
/** | ||
* Extension to AnalsysisCallback. | ||
* This is a compatibility layer created for Scala 3.x compilers. | ||
* Even though compiler-interface is NOT intended to be a public API, | ||
* Scala 3 compiler publishes their own compiler bridge against Zinc de jour. | ||
* By having this interface, they can test if `callback` supports AnalysisCallback2. | ||
*/ | ||
public interface AnalysisCallback2 extends AnalysisCallback { | ||
/** | ||
* Register a compilation problem. | ||
* | ||
* This error may have already been logged or not. Unreported problems may | ||
* happen because the reporting option was not enabled via command-line. | ||
* | ||
* @param what The headline of the error. | ||
* @param pos At a given source position. | ||
* @param msg The in-depth description of the error. | ||
* @param severity The severity of the error reported. | ||
* @param reported Flag that confirms whether this error was reported or not. | ||
* @param rendered If present, the string shown to the user when displaying this Problem. | ||
* @param diagnosticCode The unique code attached to the diagnostic being reported. | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this something that the Scala 3 bridge also needs to call? I thought we had all the pieces in place already /cc @ckipp01 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the Scala 3 bridge does call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, I guess it would seem so. I guess I don't fully understand how this is all working or when this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I confirmed this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sbt uses
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
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed. In the BSP reporter, we still use the |
||
Position pos, | ||
String msg, | ||
Severity severity, | ||
boolean reported, | ||
Optional<String> rendered, | ||
Optional<DiagnosticCode> diagnosticCode, | ||
List<DiagnosticRelatedInformation> diagnosticRelatedInformation, | ||
List<Action> actions); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I guess something like that. I figured it's better than just adding a new method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also we have methodhandles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anxsbti.Problem
we could have ensured we don't need another interface like this next time we add a field toxsbti.Problem
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I wanted to unblock the development on this, but I guess in the hindsight I should've waited for reviews.