Skip to content
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

Add quickfixes to some warnings and errors #10484

Merged
merged 3 commits into from
Aug 23, 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
9 changes: 7 additions & 2 deletions src/compiler/scala/tools/nsc/Reporting.scala
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio
if (settings.quickfix.isSetByUser && settings.quickfix.value.isEmpty) {
globalError(s"Missing message filter for `-quickfix`; see `-quickfix:help` or use `-quickfix:any` to apply all available quick fixes.")
Nil
} else if (settings.quickFixSilent) {
Nil
} else {
val parsed = settings.quickfix.value.map(WConf.parseFilter(_, rootDirPrefix))
val msgs = parsed.collect { case Left(msg) => msg }
Expand Down Expand Up @@ -167,7 +169,7 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio

val quickfixed = {
if (!skipRewriteAction(action) && registerTextEdit(warning)) s"[rewritten by -quickfix] ${warning.msg}"
else if (warning.actions.exists(_.edits.nonEmpty)) s"[quick fix available] ${warning.msg}"
else if (warning.actions.exists(_.edits.nonEmpty) && !settings.quickFixSilent) s"${warning.msg} [quickfixable]"
else warning.msg
}

Expand Down Expand Up @@ -352,11 +354,14 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio
def warning(pos: Position, msg: String, category: WarningCategory, site: Symbol, origin: String): Unit =
issueIfNotSuppressed(Message.Origin(pos, msg, category, siteName(site), origin, actions = Nil))

def codeAction(title: String, pos: Position, newText: String, desc: String, expected: Option[(String, CompilationUnit)] = None) =
CodeAction(title, pos, newText, desc, expected.forall(e => e._1 == e._2.sourceAt(pos)))

// Remember CodeActions that match `-quickfix` and report the error through the reporter
def error(pos: Position, msg: String, actions: List[CodeAction]): Unit = {
val quickfixed = {
if (registerErrorTextEdit(pos, msg, actions)) s"[rewritten by -quickfix] $msg"
else if (actions.exists(_.edits.nonEmpty)) s"[quick fix available] $msg"
else if (actions.exists(_.edits.nonEmpty) && !settings.quickFixSilent) s"$msg [quickfixable]"
else msg
}
reporter.error(pos, quickfixed, actions)
Expand Down
99 changes: 58 additions & 41 deletions src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,12 @@ package scala.tools.nsc
package ast.parser

import scala.annotation.tailrec
import scala.collection.mutable, mutable.ListBuffer
import scala.reflect.internal.{ModifierFlags => Flags, Precedence}
import scala.reflect.internal.util.{
CodeAction,
FreshNameCreator,
ListOfNil,
Position,
SourceFile,
TextEdit,
}
import Tokens._
import scala.collection.mutable
import scala.collection.mutable.ListBuffer
import scala.reflect.internal.util.{CodeAction, FreshNameCreator, ListOfNil, Position, SourceFile}
import scala.reflect.internal.{Precedence, ModifierFlags => Flags}
import scala.tools.nsc.Reporting.WarningCategory
import scala.tools.nsc.ast.parser.Tokens._

/** Historical note: JavaParsers started life as a direct copy of Parsers
* but at a time when that Parsers had been replaced by a different one.
Expand Down Expand Up @@ -329,7 +323,7 @@ self =>
def source = parser.source
}
val treeBuilder = new ParserTreeBuilder
import treeBuilder.{global => _, unit => _, source => _, fresh => _, _}
import treeBuilder.{fresh => _, global => _, source => _, unit => _, _}
som-snytt marked this conversation as resolved.
Show resolved Hide resolved

implicit def fresh: FreshNameCreator = unit.fresh

Expand Down Expand Up @@ -633,11 +627,11 @@ self =>
else deprecationWarning(offset, msg, since, actions)

// deprecation or migration under -Xsource:3, with different messages
def hardMigrationWarning(offset: Offset, depr: => String, migr: => String, since: String, actions: List[CodeAction]): Unit =
if (currentRun.isScala3) warning(offset, migr, WarningCategory.Scala3Migration, actions)
else deprecationWarning(offset, depr, since, actions)
def hardMigrationWarning(offset: Offset, depr: => String, migr: => String, since: String, actions: String => List[CodeAction]): Unit =
if (currentRun.isScala3) warning(offset, migr, WarningCategory.Scala3Migration, actions(migr))
else deprecationWarning(offset, depr, since, actions(depr))
def hardMigrationWarning(offset: Offset, depr: => String, migr: => String, since: String): Unit =
hardMigrationWarning(offset, depr, migr, since, Nil)
hardMigrationWarning(offset, depr, migr, since, _ => Nil)

def expectedMsgTemplate(exp: String, fnd: String) = s"$exp expected but $fnd found."
def expectedMsg(token: Token): String =
Expand Down Expand Up @@ -748,12 +742,18 @@ self =>
def isWildcardType = in.token == USCORE || isScala3WildcardType
def isScala3WildcardType = isRawIdent && in.name == raw.QMARK
def checkQMarkDefinition() =
if (isScala3WildcardType)
syntaxError(in.offset, "using `?` as a type name requires backticks.")
if (isScala3WildcardType) {
val msg = "using `?` as a type name requires backticks."
syntaxError(in.offset, msg,
runReporting.codeAction("add backticks", r2p(in.offset, in.offset, in.offset + 1), "`?`", msg, expected = Some(("?", unit))))
}

def checkKeywordDefinition() =
if (isRawIdent && scala3Keywords.contains(in.name))
deprecationWarning(in.offset,
s"Wrap `${in.name}` in backticks to use it as an identifier, it will become a keyword in Scala 3.", "2.13.7")
if (isRawIdent && scala3Keywords.contains(in.name)) {
val msg = s"Wrap `${in.name}` in backticks to use it as an identifier, it will become a keyword in Scala 3."
deprecationWarning(in.offset, msg, "2.13.7",
runReporting.codeAction("add backticks", r2p(in.offset, in.offset, in.offset + in.name.length), s"`${in.name}`", msg, expected = Some((in.name.toString, unit))))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently noticed on another PR that the advice is not supplied in the common case:

scala> val yield = 42
           ^
       error: illegal start of simple pattern []

scala> val enum = 42
           ^
       warning: Wrap `enum` in backticks to use it as an identifier, it will become a keyword in Scala 3.


def isIdent = in.token == IDENTIFIER || in.token == BACKQUOTED_IDENT
def isMacro = in.token == IDENTIFIER && in.name == nme.MACROkw
Expand Down Expand Up @@ -816,8 +816,7 @@ self =>
val wrn = sm"""|$msg
|Use '-Wconf:msg=lambda-parens:s' to silence this warning."""
def actions =
if (tree.pos.isRange)
List(CodeAction("lambda parameter", Some(msg), List(TextEdit(tree.pos, s"(${unit.sourceAt(tree.pos)})"))))
if (tree.pos.isRange) runReporting.codeAction("add parentheses", tree.pos, s"(${unit.sourceAt(tree.pos)})", msg)
else Nil
migrationWarning(tree.pos.point, wrn, "2.13.11", actions)
List(convertToParam(tree))
Expand Down Expand Up @@ -1029,11 +1028,17 @@ self =>

def finishBinaryOp(isExpr: Boolean, opinfo: OpInfo, rhs: Tree): Tree = {
import opinfo._
if (targs.nonEmpty)
migrationWarning(offset, "type application is not allowed for infix operators", "2.13.11")
val operatorPos: Position = Position.range(rhs.pos.source, offset, offset, offset + operator.length)
val pos = lhs.pos.union(rhs.pos).union(operatorPos).withEnd(in.lastOffset).withPoint(offset)

if (targs.nonEmpty) {
val qual = unit.sourceAt(lhs.pos)
val fun = s"${CodeAction.maybeWrapInParens(qual)}.${unit.sourceAt(operatorPos.withEnd(rhs.pos.start))}".trim
val fix = s"$fun${CodeAction.wrapInParens(unit.sourceAt(rhs.pos))}"
val msg = "type application is not allowed for infix operators"
migrationWarning(offset, msg, "2.13.11",
runReporting.codeAction("use selection", pos, fix, msg))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like a lot of ceremony in the compiler, but the result is neat. I opened a ticket to let the REPL show the edit.

In this case, there is an unnecessary set of parens, a minor nitpick. ((This is a common style from the dotless era.))

scala> List(42) map[String] (_.toString)
                ^
       error: type application is not allowed for infix operators
       Scala 3 migration messages are errors under -Xsource:3. Use -Wconf / @nowarn to filter them or add -Xmigration to demote them to warnings.
       Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=scala3-migration [CodeAction(use selection,Some(type application is not allowed for infix operators),List(TextEdit(RangePosition(<console>, 0, 9, 33),List(42).map[String]((_.toString)))))]

atPos(pos)(makeBinop(isExpr, lhs, operator, rhs, operatorPos, targs))
}

Expand Down Expand Up @@ -1090,7 +1095,9 @@ self =>
if (in.token == ARROW)
atPos(start, in.skipToken()) { makeSafeFunctionType(ts, typ()) }
else if (ts.isEmpty) {
syntaxError(start, "Illegal literal type (), use Unit instead")
val msg = "Illegal literal type (), use Unit instead"
syntaxError(start, msg,
runReporting.codeAction("use `Unit`", r2p(start, start, start + 2), "Unit", msg, expected = Some(("()", unit))))
EmptyTree
}
else {
Expand Down Expand Up @@ -1174,7 +1181,9 @@ self =>
if (lookingAhead(in.token == RPAREN)) {
in.nextToken()
in.nextToken()
syntaxError(start, "Illegal literal type (), use Unit instead")
val msg = "Illegal literal type (), use Unit instead"
syntaxError(start, msg,
runReporting.codeAction("use `Unit`", r2p(start, start, start + 2), "Unit", msg, expected = Some(("()", unit))))
EmptyTree
}
else
Expand Down Expand Up @@ -1475,9 +1484,9 @@ self =>
else withPlaceholders(interpolatedString(inPattern), isAny = true) // interpolator params are Any* by definition
}
else if (in.token == SYMBOLLIT) {
def msg(what: String) =
s"""symbol literal is $what; use Symbol("${in.strVal}") instead"""
deprecationWarning(in.offset, msg("deprecated"), "2.13.0")
val msg = s"""symbol literal is deprecated; use Symbol("${in.strVal}") instead"""
deprecationWarning(in.offset, msg, "2.13.0",
runReporting.codeAction("replace symbol literal", r2p(in.offset, in.offset, in.offset + 1 + in.strVal.length), s"""Symbol("${in.strVal}")""", msg, expected = Some((s"'${in.strVal}", unit))))
Apply(scalaDot(nme.Symbol), List(finish(in.strVal)))
}
else finish(in.token match {
Expand Down Expand Up @@ -2095,17 +2104,15 @@ self =>
val hasEq = in.token == EQUALS

if (hasVal) {
def actions = {
val pos = r2p(valOffset, valOffset, valOffset + 4)
if (unit.sourceAt(pos) != "val ") Nil else
List(CodeAction("val in for comprehension", None, List(TextEdit(pos, ""))))
}
def actions(msg: String) = runReporting.codeAction("remove `val` keyword", r2p(valOffset, valOffset, valOffset + 4), "", msg, expected = Some(("val ", unit)))
def msg(what: String, instead: String): String = s"`val` keyword in for comprehension is $what: $instead"
if (hasEq) {
val without = "instead, bind the value without `val`"
hardMigrationWarning(in.offset, msg("deprecated", without), msg("unsupported", without), "2.10.0", actions)
} else {
val m = msg("unsupported", "just remove `val`")
syntaxError(in.offset, m, actions(m))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the messages differ, but it's always been confusing to me. (And I'm no longer a beginner.) Like, is there a hidden message in "bind the value without val"? Some kind of dark magic incantation? This is an example where it is so much easier to just show me the fixed code! than to explain with words.

}
else syntaxError(in.offset, msg("unsupported", "just remove `val`"), actions)
}

if (hasEq && eqOK && !hasCase) in.nextToken()
Expand Down Expand Up @@ -2969,7 +2976,11 @@ self =>
def funDefOrDcl(start: Int, mods: Modifiers): Tree = {
in.nextToken()
if (in.token == THIS) {
def missingEquals() = hardMigrationWarning(in.lastOffset, "procedure syntax is deprecated for constructors: add `=`, as in method definition", "2.13.2")
def missingEquals() = {
val msg = "procedure syntax is deprecated for constructors: add `=`, as in method definition"
hardMigrationWarning(in.lastOffset, msg, "2.13.2",
runReporting.codeAction("replace procedure syntax", o2p(in.lastOffset), " =", msg))
}
atPos(start, in.skipToken()) {
val vparamss = paramClauses(nme.CONSTRUCTOR, classContextBounds map (_.duplicate), ofCaseClass = false)
newLineOptWhenFollowedBy(LBRACE)
Expand Down Expand Up @@ -3006,8 +3017,8 @@ self =>
var restype = fromWithinReturnType(typedOpt())
def msg(what: String, instead: String) =
s"procedure syntax is $what: instead, add `$instead` to explicitly declare `$name`'s return type"
def declActions = List(CodeAction("procedure syntax (decl)", None, List(TextEdit(o2p(in.lastOffset), ": Unit"))))
def defnActions = List(CodeAction("procedure syntax (defn)", None, List(TextEdit(o2p(in.lastOffset), ": Unit ="))))
def declActions(msg: String) = runReporting.codeAction("add result type", o2p(in.lastOffset), ": Unit", msg)
def defnActions(msg: String) = runReporting.codeAction("replace procedure syntax", o2p(in.lastOffset), ": Unit =", msg)
val rhs =
if (isStatSep || in.token == RBRACE) {
if (restype.isEmpty) {
Expand Down Expand Up @@ -3035,7 +3046,11 @@ self =>
if (nme.isEncodedUnary(name) && vparamss.nonEmpty) {
def instead = DefDef(newmods, name.toTermName.decodedName, tparams, vparamss.drop(1), restype, rhs)
def unaryMsg(what: String) = s"unary prefix operator definition with empty parameter list is $what: instead, remove () to declare as `$instead`"
def warnNilary() = hardMigrationWarning(nameOffset, unaryMsg("deprecated"), unaryMsg("unsupported"), "2.13.4")
def action(msg: String) = {
val o = nameOffset + name.decode.length
runReporting.codeAction("remove ()", r2p(o, o, o + 2), "", msg, expected = Some(("()", unit)))
}
def warnNilary() = hardMigrationWarning(nameOffset, unaryMsg("deprecated"), unaryMsg("unsupported"), "2.13.4", action)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the edge of an edge case, but also an example where the code (to supply actions) is finicky.

The following doesn't get an action:
def unary_~( /* TODO pass context? */ ) = 42
or less weird
def unary_~ ( ) = 42

Consider the style if( ! bool ) ??? to demonstrate that some coders have special preferences.

vparamss match {
case List(List()) => warnNilary()
case List(List(), x :: xs) if x.mods.isImplicit => warnNilary()
Expand Down Expand Up @@ -3313,7 +3328,9 @@ self =>
*/
def templateOpt(mods: Modifiers, name: Name, constrMods: Modifiers, vparamss: List[List[ValDef]], tstart: Offset): Template = {
def deprecatedUsage(): Boolean = {
deprecationWarning(in.offset, "Using `<:` for `extends` is deprecated", since = "2.12.5")
val msg = "Using `<:` for `extends` is deprecated"
deprecationWarning(in.offset, msg, since = "2.12.5",
runReporting.codeAction("use `extends`", r2p(in.offset, in.offset, in.offset + 2), "extends", msg, expected = Some(("<:", unit))))
true
}
val (parents, self, body) =
Expand Down
18 changes: 14 additions & 4 deletions src/compiler/scala/tools/nsc/ast/parser/Scanners.scala
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ trait Scanners extends ScannersCommon {
}

abstract class Scanner extends CharArrayReader with TokenData with ScannerData with ScannerCommon with DocScanner {
def unit: CompilationUnit

/** A switch whether operators at the start of lines can be infix operators. */
private var allowLeadingInfixOperators = true

Expand Down Expand Up @@ -817,10 +819,14 @@ trait Scanners extends ScannersCommon {
case _ =>
def fetchOther() = {
if (ch == '\u21D2') {
deprecationWarning("The unicode arrow `⇒` is deprecated, use `=>` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code.", "2.13.0")
val msg = "The unicode arrow `⇒` is deprecated, use `=>` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code."
deprecationWarning(msg, "2.13.0",
runReporting.codeAction("replace unicode arrow", unit.position(offset).withEnd(offset + 1), "=>", msg, expected = Some(("⇒", unit))))
nextChar(); token = ARROW
} else if (ch == '\u2190') {
deprecationWarning("The unicode arrow `←` is deprecated, use `<-` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code.", "2.13.0")
val msg = "The unicode arrow `←` is deprecated, use `<-` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code."
deprecationWarning(msg, "2.13.0",
runReporting.codeAction("replace unicode arrow", unit.position(offset).withEnd(offset + 1), "<-", msg, expected = Some(("←", unit))))
nextChar(); token = LARROW
} else if (isUnicodeIdentifierStart(ch)) {
putChar(ch)
Expand Down Expand Up @@ -1370,7 +1376,9 @@ trait Scanners extends ScannersCommon {
// 1l is an acknowledged bad practice
def lintel(): Unit = {
val msg = "Lowercase el for long is not recommended because it is easy to confuse with numeral 1; use uppercase L instead"
if (ch == 'l') deprecationWarning(numberOffset + cbuf.length, msg, since="2.13.0")
val o = numberOffset + cbuf.length
if (ch == 'l') deprecationWarning(o, msg, since="2.13.0",
runReporting.codeAction("use uppercase L", unit.position(o).withEnd(o + 1), "L", msg, expected = Some(("l", unit))))
}
// after int: 5e7f, 42L, 42.toDouble but not 42b.
def restOfNumber(): Unit = {
Expand Down Expand Up @@ -1573,6 +1581,8 @@ trait Scanners extends ScannersCommon {
* Useful for looking inside source files that are not currently compiled to see what's there
*/
class SourceFileScanner(val source: SourceFile) extends Scanner {
def unit = global.currentUnit

val buf = source.content

// suppress warnings, throw exception on errors
Expand All @@ -1584,7 +1594,7 @@ trait Scanners extends ScannersCommon {

/** A scanner over a given compilation unit
*/
class UnitScanner(val unit: CompilationUnit, patches: List[BracePatch]) extends SourceFileScanner(unit.source) {
class UnitScanner(override val unit: CompilationUnit, patches: List[BracePatch]) extends SourceFileScanner(unit.source) {
def this(unit: CompilationUnit) = this(unit, List())

override def warning(off: Offset, msg: String, category: WarningCategory): Unit =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,11 @@ trait StandardScalaSettings { _: MutableSettings =>
| -quickfix:msg=Auto-application apply quick fixes where the message contains "Auto-application"
|
|Use `-Wconf:any:warning-verbose` to display applicable message filters with each warning.
|
|Use `-quickfix:silent` to omit the `[quickfixable]` tag in compiler messages.
|""".stripMargin),
prepend = true)
def quickFixSilent: Boolean = quickfix.value == List("silent")
val release =
ChoiceSetting("-release", "release", "Compile for a version of the Java API and target class file.", AllTargetVersions, normalizeTarget(javaSpecVersion))
.withPostSetHook { setting =>
Expand Down
10 changes: 7 additions & 3 deletions src/compiler/scala/tools/nsc/typechecker/Adaptations.scala
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,13 @@ trait Adaptations {
if (settings.lintArgDiscard && discardedArgs) context.warning(t.pos, adaptWarningMessage(
s"adapted the argument list to expected Unit type: arguments will be discarded"),
WarningCategory.LintAdaptedArgs)
else if (settings.warnAdaptedArgs && !isInfix) context.warning(t.pos, adaptWarningMessage(
s"adapted the argument list to the expected ${args.size}-tuple: add additional parens instead"),
WarningCategory.LintAdaptedArgs)
else if (settings.warnAdaptedArgs && !isInfix) {
val msg = adaptWarningMessage(
s"adapted the argument list to the expected ${args.size}-tuple: add additional parens instead")
val pos = wrappingPos(args)
context.warning(t.pos, msg, WarningCategory.LintAdaptedArgs,
runReporting.codeAction("add wrapping parentheses", pos, s"(${currentUnit.sourceAt(pos)})", msg))
}
true // keep adaptation
}
if (args.nonEmpty)
Expand Down
Loading