-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: hook up actions in sbt-bridge to start forwarding actionable diagnostics #17561
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
8808e63
feat: update bridge to account for actions
ckipp01 56d4885
dep: bump sbt to 1.9.0-RC3 for tests
ckipp01 3856746
docs: add in a few docs in the newly added classes
ckipp01 df434da
test: add in a CodeActionTest suite
ckipp01 d046e2f
feat: add in an action for missing empty argument list
ckipp01 1618a0b
feat: add in an action to remove duplicated modifiers
ckipp01 75f2db9
fix: add unsafeNull imports back in
ckipp01 a991149
dep: bump sbt to 1.9.0
ckipp01 f1a16f4
refactor: use scala types
ckipp01 6af13b3
refactor: make sure the patch logic isn't duplicated
ckipp01 6c9b967
refactor(test): simplify the code action testing
ckipp01 079a1bf
refactor: add in a `Rewrites.applyAction` helper
ckipp01 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package dotty.tools.dotc.reporting | ||
|
||
import dotty.tools.dotc.rewrites.Rewrites.ActionPatch | ||
|
||
/** A representation of a code action / fix that can be used by tooling to | ||
* apply a fix to their code. | ||
* | ||
* @param title The title of the fix, often showed to a user in their editor. | ||
* @param description An optional description of the fix. | ||
* @param patches The patches that this fix contains. | ||
*/ | ||
case class CodeAction( | ||
title: String, | ||
description: Option[String], | ||
patches: List[ActionPatch] | ||
) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
91 changes: 91 additions & 0 deletions
91
compiler/test/dotty/tools/dotc/reporting/CodeActionTest.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
package dotty.tools.dotc.reporting | ||
|
||
import dotty.tools.DottyTest | ||
import dotty.tools.dotc.rewrites.Rewrites | ||
import dotty.tools.dotc.rewrites.Rewrites.ActionPatch | ||
import dotty.tools.dotc.util.SourceFile | ||
|
||
import scala.annotation.tailrec | ||
import scala.jdk.CollectionConverters.* | ||
import scala.runtime.Scala3RunTime.assertFailed | ||
|
||
import org.junit.Assert._ | ||
import org.junit.Test | ||
|
||
/** This is a test suite that is meant to test the actions attached to the | ||
* diagnostic for a given code snippet. | ||
*/ | ||
class CodeActionTest extends DottyTest: | ||
|
||
@Test def convertToFunctionValue = | ||
checkCodeAction( | ||
"""|object Test: | ||
| def x: Int = 3 | ||
| val test = x _ | ||
|""".stripMargin, | ||
"Rewrite to function value", | ||
"""|object Test: | ||
| def x: Int = 3 | ||
| val test = (() => x) | ||
|""".stripMargin | ||
) | ||
|
||
@Test def insertBracesForEmptyArgument = | ||
checkCodeAction( | ||
"""|object Test: | ||
| def foo(): Unit = () | ||
| val x = foo | ||
|""".stripMargin, | ||
"Insert ()", | ||
"""|object Test: | ||
| def foo(): Unit = () | ||
| val x = foo() | ||
|""".stripMargin | ||
|
||
) | ||
|
||
@Test def removeRepeatModifier = | ||
checkCodeAction( | ||
"""|final final class Test | ||
|""".stripMargin, | ||
"""Remove repeated modifier: "final"""", | ||
// TODO look into trying to remove the extra space that is left behind | ||
"""|final class Test | ||
|""".stripMargin | ||
|
||
) | ||
|
||
// Make sure we're not using the default reporter, which is the ConsoleReporter, | ||
// meaning they will get reported in the test run and that's it. | ||
private def newContext = | ||
val rep = new StoreReporter(null) with UniqueMessagePositions with HideNonSensicalMessages | ||
initialCtx.setReporter(rep).withoutColors | ||
|
||
private def checkCodeAction(code: String, title: String, expected: String) = | ||
ctx = newContext | ||
val source = SourceFile.virtual("test", code).content | ||
val runCtx = checkCompile("typer", code) { (_, _) => () } | ||
val diagnostics = runCtx.reporter.removeBufferedMessages | ||
assertEquals(1, diagnostics.size) | ||
|
||
val diagnostic = diagnostics.head | ||
val actions = diagnostic.msg.actions.toList | ||
assertEquals(1, actions.size) | ||
|
||
// TODO account for more than 1 action | ||
val action = actions.head | ||
assertEquals(action.title, title) | ||
val patches = action.patches.toList | ||
if patches.nonEmpty then | ||
patches.reduceLeft: (p1, p2) => | ||
assert(p1.srcPos.span.end <= p2.srcPos.span.start, s"overlapping patches $p1 and $p2") | ||
p2 | ||
else assertFailed("Expected a patch attatched to this action, but it was empty") | ||
|
||
val result = patches.reverse.foldLeft(code): (newCode, patch) => | ||
import scala.language.unsafeNulls | ||
val start = newCode.substring(0, patch.srcPos.start) | ||
val ending = newCode.substring(patch.srcPos.end, newCode.length) | ||
start + patch.replacement + ending | ||
|
||
assertEquals(expected, result) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
sbt.version=1.8.2 | ||
sbt.version=1.9.0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
package dotty.tools.xsbt; | ||
|
||
import java.util.Optional; | ||
|
||
final public class Action implements xsbti.Action { | ||
private final String _title; | ||
private final Optional<String> _description; | ||
private final WorkspaceEdit _edit; | ||
|
||
public Action(String title, Optional<String> description, WorkspaceEdit edit) { | ||
super(); | ||
this._title = title; | ||
this._description = description; | ||
this._edit = edit; | ||
} | ||
|
||
public String title() { | ||
return _title; | ||
} | ||
|
||
public Optional<String> description() { | ||
return _description; | ||
} | ||
|
||
public WorkspaceEdit edit() { | ||
return _edit; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 oldpatch
mechanism at the same time otherwise we'll be in a weird situation where -rewrite and IDEs each do their own thing.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.
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
patch
es 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 callpatch
, there is noMessage
for that. So in order for that to work we'd really want to create a uniqueMessage
for it that would hold that actions/patch.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 could keep
patch
around for formatting changes (braces <-> indent) since flooding the console with messages for those changes wouldn't be useful.