Skip to content

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 12 commits into from
Jul 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ final case class SbtCommunityProject(
case Some(ivyHome) => List(s"-Dsbt.ivy.home=$ivyHome")
case _ => Nil
extraSbtArgs ++ sbtProps ++ List(
"-sbt-version", "1.8.2",
"-sbt-version", "1.9.0",
"-Dsbt.supershell=false",
s"-Ddotty.communitybuild.dir=$communitybuildDir",
s"--addPluginSbtFile=$sbtPluginFilePath"
Expand Down
3 changes: 1 addition & 2 deletions compiler/src/dotty/tools/dotc/parsing/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3059,8 +3059,7 @@ object Parsers {
val mod = atSpan(in.skipToken()) { modOfToken(tok, name) }

if mods.isOneOf(mod.flags) then
syntaxError(RepeatedModifier(mod.flags.flagsString), mod.span)

syntaxError(RepeatedModifier(mod.flags.flagsString, source, mod.span), mod.span)
addMod(mods, mod)
}

Expand Down
16 changes: 16 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/CodeAction.scala
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]
)
6 changes: 5 additions & 1 deletion compiler/src/dotty/tools/dotc/reporting/Message.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import printing.Formatting.hl
import config.SourceVersion

import scala.language.unsafeNulls

import scala.annotation.threadUnsafe

/** ## Tips for error message generation
Expand Down Expand Up @@ -415,6 +414,11 @@ abstract class Message(val errorId: ErrorMessageID)(using Context) { self =>
*/
def showAlways = false

/** A list of actions attached to this message to address the issue this
* message represents.
*/
def actions(using Context): List[CodeAction] = List.empty

override def toString = msg
}

Expand Down
46 changes: 43 additions & 3 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ import transform.SymUtils._
import scala.util.matching.Regex
import java.util.regex.Matcher.quoteReplacement
import cc.CaptureSet.IdentityCaptRefMap
import dotty.tools.dotc.rewrites.Rewrites.ActionPatch
import dotty.tools.dotc.util.Spans.Span
import dotty.tools.dotc.util.SourcePosition
import scala.jdk.CollectionConverters.*
import dotty.tools.dotc.util.SourceFile

/** Messages
* ========
Expand Down Expand Up @@ -493,7 +498,7 @@ extends SyntaxMsg(ObjectMayNotHaveSelfTypeID) {
}
}

class RepeatedModifier(modifier: String)(implicit ctx:Context)
class RepeatedModifier(modifier: String, source: SourceFile, span: Span)(implicit ctx:Context)
extends SyntaxMsg(RepeatedModifierID) {
def msg(using Context) = i"""Repeated modifier $modifier"""

Expand All @@ -512,6 +517,17 @@ extends SyntaxMsg(RepeatedModifierID) {
|
|"""
}

override def actions(using Context) =
import scala.language.unsafeNulls
List(
CodeAction(title = s"""Remove repeated modifier: "$modifier"""",
description = None,
patches = List(
ActionPatch(SourcePosition(source, span), "")
)
)
)
}

class InterpolatedStringError()(implicit ctx:Context)
Expand Down Expand Up @@ -1846,15 +1862,28 @@ class FailureToEliminateExistential(tp: Type, tp1: Type, tp2: Type, boundSyms: L
|are only approximated in a best-effort way."""
}

class OnlyFunctionsCanBeFollowedByUnderscore(tp: Type)(using Context)
class OnlyFunctionsCanBeFollowedByUnderscore(tp: Type, tree: untpd.PostfixOp)(using Context)
extends SyntaxMsg(OnlyFunctionsCanBeFollowedByUnderscoreID) {
def msg(using Context) = i"Only function types can be followed by ${hl("_")} but the current expression has type $tp"
def explain(using Context) =
i"""The syntax ${hl("x _")} is no longer supported if ${hl("x")} is not a function.
|To convert to a function value, you need to explicitly write ${hl("() => x")}"""

override def actions(using Context) =
import scala.language.unsafeNulls
val untpd.PostfixOp(qual, Ident(nme.WILDCARD)) = tree: @unchecked
List(
CodeAction(title = "Rewrite to function value",
description = None,
patches = List(
ActionPatch(SourcePosition(tree.source, Span(tree.span.start)), "(() => "),
ActionPatch(SourcePosition(tree.source, Span(qual.span.end, tree.span.end)), ")")
Comment on lines +1879 to +1880
Copy link
Member

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 old patch mechanism at the same time otherwise we'll be in a weird situation where -rewrite and IDEs each do their own thing.

Copy link
Member Author

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 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.

Copy link
Member

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.

)
)
)
}

class MissingEmptyArgumentList(method: String)(using Context)
class MissingEmptyArgumentList(method: String, tree: tpd.Tree)(using Context)
extends SyntaxMsg(MissingEmptyArgumentListID) {
def msg(using Context) = i"$method must be called with ${hl("()")} argument"
def explain(using Context) = {
Expand All @@ -1869,6 +1898,17 @@ class MissingEmptyArgumentList(method: String)(using Context)
|In Dotty, this idiom is an error. The application syntax has to follow exactly the parameter syntax.
|Excluded from this rule are methods that are defined in Java or that override methods defined in Java."""
}

override def actions(using Context) =
import scala.language.unsafeNulls
List(
CodeAction(title = "Insert ()",
description = None,
patches = List(
ActionPatch(SourcePosition(tree.source, tree.span.endPos), "()"),
)
)
)
}

class DuplicateNamedTypeParameter(name: Name)(using Context)
Expand Down
20 changes: 20 additions & 0 deletions compiler/src/dotty/tools/dotc/rewrites/Rewrites.scala
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import core.Contexts._
import collection.mutable
import scala.annotation.tailrec
import dotty.tools.dotc.reporting.Reporter
import dotty.tools.dotc.util.SourcePosition;

import java.io.OutputStreamWriter
import java.nio.charset.StandardCharsets.UTF_8
import dotty.tools.dotc.reporting.CodeAction

/** Handles rewriting of Scala2 files to Dotty */
object Rewrites {
Expand All @@ -19,6 +21,16 @@ object Rewrites {
def delta = replacement.length - (span.end - span.start)
}

/** A special type of Patch that instead of just a span, contains the
* full SourcePosition. This is useful when being used by
* [[dotty.tools.dotc.reporting.CodeAction]] or if the patch doesn't
* belong to the same file that the actual issue it's addressing is in.
*
* @param srcPos The SourcePosition of the patch.
* @param replacement The Replacement that should go in that position.
*/
case class ActionPatch(srcPos: SourcePosition, replacement: String)

private class Patches(source: SourceFile) {
private[Rewrites] val pbuf = new mutable.ListBuffer[Patch]()

Expand Down Expand Up @@ -88,6 +100,14 @@ object Rewrites {
report.echo(s"[patched file ${source.file.path}]")
rewrites.patched(source).writeBack()
}

/** Given a CodeAction take the patches and apply them.
*
* @param action The CodeAction containing the patches
*/
def applyAction(action: CodeAction)(using Context): Unit =
action.patches.foreach: actionPatch =>
patch(actionPatch.srcPos.span, actionPatch.replacement)
}

/** A completely encapsulated class representing rewrite state, used
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/ErrorReporting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ object ErrorReporting {
val meth = err.exprStr(methPart(tree))
val info = if tree.symbol.exists then tree.symbol.info else mt
if isCallableWithSingleEmptyArgumentList(info) then
report.error(MissingEmptyArgumentList(meth), tree.srcPos)
report.error(MissingEmptyArgumentList(meth, tree), tree.srcPos)
else
report.error(MissingArgumentList(meth, tree.symbol), tree.srcPos)

Expand Down
20 changes: 15 additions & 5 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import cc.CheckCaptures
import config.Config

import scala.annotation.constructorOnly
import dotty.tools.dotc.rewrites.Rewrites

object Typer {

Expand Down Expand Up @@ -2941,11 +2942,13 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
case closure(_, _, _) =>
case _ =>
val recovered = typed(qual)(using ctx.fresh.setExploreTyperState())
report.errorOrMigrationWarning(OnlyFunctionsCanBeFollowedByUnderscore(recovered.tpe.widen), tree.srcPos, from = `3.0`)
val msg = OnlyFunctionsCanBeFollowedByUnderscore(recovered.tpe.widen, tree)
report.errorOrMigrationWarning(msg, tree.srcPos, from = `3.0`)
if (migrateTo3) {
// Under -rewrite, patch `x _` to `(() => x)`
patch(Span(tree.span.start), "(() => ")
patch(Span(qual.span.end, tree.span.end), ")")
msg.actions
.headOption
.foreach(Rewrites.applyAction)
return typed(untpd.Function(Nil, qual), pt)
}
}
Expand Down Expand Up @@ -3949,10 +3952,17 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
def adaptNoArgsUnappliedMethod(wtp: MethodType, functionExpected: Boolean, arity: Int): Tree = {
/** Is reference to this symbol `f` automatically expanded to `f()`? */
def isAutoApplied(sym: Symbol): Boolean =
lazy val msg = MissingEmptyArgumentList(sym.show, tree)

sym.isConstructor
|| sym.matchNullaryLoosely
|| Feature.warnOnMigration(MissingEmptyArgumentList(sym.show), tree.srcPos, version = `3.0`)
&& { patch(tree.span.endPos, "()"); true }
|| Feature.warnOnMigration(msg, tree.srcPos, version = `3.0`)
&& {
msg.actions
.headOption
.foreach(Rewrites.applyAction)
true
}

/** If this is a selection prototype of the form `.apply(...): R`, return the nested
* function prototype `(...)R`. Otherwise `pt`.
Expand Down
91 changes: 91 additions & 0 deletions compiler/test/dotty/tools/dotc/reporting/CodeActionTest.scala
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)
2 changes: 1 addition & 1 deletion project/Dependencies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,6 @@ object Dependencies {
"com.vladsch.flexmark" % "flexmark-ext-yaml-front-matter" % flexmarkVersion,
)

val newCompilerInterface = "org.scala-sbt" % "compiler-interface" % "1.8.0"
val newCompilerInterface = "org.scala-sbt" % "compiler-interface" % "1.9.0"
val oldCompilerInterface = "org.scala-sbt" % "compiler-interface" % "1.3.5"
}
2 changes: 1 addition & 1 deletion project/build.properties
Original file line number Diff line number Diff line change
@@ -1 +1 @@
sbt.version=1.8.2
sbt.version=1.9.0
28 changes: 28 additions & 0 deletions sbt-bridge/src/dotty/tools/xsbt/Action.java
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;
}
}
Loading