Add quickfixes to some warnings and errors #10484
Conversation
|
Would be cool https://github.com/scala/scala/blob/v2.13.11/src/library/scala/Predef.scala#L351-L352 |
lrytz
force-pushed
the
moreQuickFixes
branch
2 times, most recently
from
ce5f13e
to
1b73191
Compare
|
Rewrite makes unintentionally dangerous things like lossy conversion too easy to unintentionally ignore. Who is the intended user? Someone upgrading a project with a hundred annoying warnings? The feature I liked on my dotty unused imports branch was rewriting the import, which is always benign. |
This is mainly intended for IDEs. The path via
I think fixes provided by the compiler should always maintain semantics. Let's say we add a new useful warning in a minor release, like for
Metals and IntelliJ have organize imports - what would be the difference? |
Perhaps it's more useful than it first seems. One always has the option of letting the compiler apply the update everywhere, but then checking each one by hand/eye (e.g. with |
SethTisue
added
the
prio:hi
|
The other day, I considered that it would be nice to externalize both messages and code actions. If only we had an |
| val msg = "type application is not allowed for infix operators" | ||
| migrationWarning(offset, msg, "2.13.11", | ||
| runReporting.codeAction("use selection", pos, fix, msg)) | ||
| } |
There was a problem hiding this comment.
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)))))]
| 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)))) | ||
| } |
There was a problem hiding this comment.
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 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)) |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| if (!exempt()) | ||
| if (!exempt()) { | ||
| val msg = "method without a parameter list overrides a method with a single empty one" | ||
| val namePos = member.pos.focus.withEnd(member.pos.point + member.decodedName.length) |
There was a problem hiding this comment.
I'll give one of these nameposses a try locally.
| context.warning(tree.pos, s"integral division is implicitly converted (widened) to floating point. Add an explicit `.to${ptSym.name}`.", WarningCategory.LintIntDivToFloat) | ||
| val msg = s"integral division is implicitly converted (widened) to floating point. Add an explicit `.to${ptSym.name}`." | ||
| context.warning(tree.pos, msg, WarningCategory.LintIntDivToFloat, | ||
| runReporting.codeAction("add conversion", tree.pos, s"(${currentUnit.sourceAt(tree.pos)}).to${ptSym.name}", msg)) |
There was a problem hiding this comment.
This is the one where we make it (too) easy to make a bad choice. If they really want it, they can do it the old-fashioned way. (Tell the intern to edit all the conversions.)
| } | ||
|
|
||
| @Test | ||
| def constructorProcedureSyntax(): Unit = { |
There was a problem hiding this comment.
Just to note that linting that the unit-valued method has parens for a param list is turned off for tests.
That makes sense as a policy because test methods are never called explicitly; though it would be even finer if only test-annotated methods were ignored.
| @@ -5,5 +5,5 @@ import org.junit.runners.JUnit4 | |||
|
|
|||
| @RunWith(classOf[JUnit4]) | |||
| class CodeActionTest extends AbstractCodeActionTest { | |||
| override def compilerArgs: String = "-Ystop-after:typer -Yvalidate-pos:typer -Yrangepos -deprecation" | |||
| override def compilerArgs: String = "-Ystop-after:refchecks -deprecation -Xlint" | |||
There was a problem hiding this comment.
Xlint enables deprecation in case that helps instead of annoying.
| @Test | ||
| def infixTypeArg(): Unit = { | ||
| assertCodeSuggestion("class C { List apply[Int] 1 }", "class C { List.apply[Int](1) }") | ||
| assertCodeSuggestion("class C { List apply 1 map[Int] identity }", "class C { (List apply 1).map[Int](identity) }") |
There was a problem hiding this comment.
my previous example was List(42) map[Int] (_ + 1) type.
|
Travis spec build failed. Maybe transient, myabe a key expired? |
|
@lrytz needs rebase |
|
@SethTisue @som-snytt I'm unsure about the new |
|
I don't like that the I wondered whether it would be OK as trailing text. If we had a word for Maybe we'll need to upgrade to dotty-style message format. Or rust-style, just follow the message with the correction, annotated as a quickfix. |
lrytz
force-pushed
the
moreQuickFixes
branch
2 times, most recently
from
6bb165d
to
2e996a3
Compare
|
Pushing it into the label ( Adding it at the end sounds reasonable. Once you know what it means it would be nice to have something short like There are some multi-line messages, I guess that's OK I pushed that. Also added support for |
allow skipping it with `-quickfix:silent` make more error quickfixes available to `-quickfix` rewrite by using `runReporting.error` instead of `reporter.error`.
|
@SethTisue LGTY? |
|
still lgtm or leb (even better). Trailing It seems that whenever we introduce noise, we must supply a mechanism for noise suppression. (As a general observation.) |
|
Well, I'm tempted to just hit "merge". But how about |
|
I'm willing to merge and allow for follow-up:
but I don't want to hit the button while Seth is in mid-conjecture. |
I agree shorter is better. If you native speakers think it's fine then it's certainly fine with me. |
|
I think it would sound fine in German but not in English. Further bikeshedding: emit the filter: or whatever the syntax is. (See, I don't even remember what to do. I guess in an IDE, you just click a widget.) |
|
It seems "quickfixable" is googleable. |
that could be neat, but it could be a separate PR |
SethTisue
removed
the
prio:hi
SethTisue
changed the title
|
community build regression I verified that when compiling the same code with 2.13.11, a "method with a single empty parameter list overrides method without any parameter list" warning is issued (that's what RefChecks.scala:467 is trying to do, is to issue that warning) the warning generation code should be robust in the face of apparently the without any information about where it's coming from in the user source the problem is reproducible outside of dbuild by checking out the - libraryDependencies += "com.lihaoyi" %% "acyclic" % "0.3.8" cross CrossVersion.full,
+ libraryDependencies += "com.lihaoyi" % "acyclic_2.13.11" % "0.3.8",and then doing |
That was a rash promise. Well, I can never get dbuild to run and I don't know how to get mill to build Ammonite with my published compiler; I tried both, but I don't have the reserves for a research project just to get something to attempt to compile. |
|
Okay, so the underlying bug was there even in 2.13.11, and I've reported it at scala/bug#12851 And then this PR turned that bug into a crasher. I think addressing 12851 can wait for 2.13.13. For 2.13.12, I will submit a minimal PR that just makes this not crash the compiler. |
(I've updated my comment above to include non-dbuild repro instructions, but it's probably moot now since the isolation work is done.) |
|
Thanks, I will try that later! more rash promises. |
### What changes were proposed in this pull request? This pr aims to upgrade Scala from 2.13.11 to 2.13.12. Additionally, this pr adds ``-Wconf:msg=legacy-binding:s`` to suppress similar compiler warnings as below: ``` [ERROR] /Users/yangjie01/SourceCode/git/spark-mine-13/core/src/main/scala/org/apache/spark/deploy/client/StandaloneAppClient.scala:171: reference to stop is ambiguous; it is both defined in the enclosing class StandaloneAppClient and inherited in the enclosing class ClientEndpoint as method stop (defined in trait RpcEndpoint, inherited through parent trait ThreadSafeRpcEndpoint) In Scala 2, symbols inherited from a superclass shadow symbols defined in an outer scope. Such references are ambiguous in Scala 3. To continue using the inherited symbol, write `this.stop`. Or use `-Wconf:msg=legacy-binding:s` to silence this warning. [quickfixable] Applicable -Wconf / nowarn filters for this fatal warning: msg=<part of the message>, cat=other, site=org.apache.spark.deploy.client.StandaloneAppClient.ClientEndpoint.receive ``` ### Why are the changes needed? The new version bring some regression fixes: - scala/scala#10422 - scala/scala#10424 And a new feature: Quickfixes ``` For some errors and warnings, the compiler now suggests an edit that could fix the issue. Tooling such as IDEs can then offer the edits, or the compiler itself will make the change if run again with -quickfix. ``` - scala/scala#10406 - scala/scala#10482 - scala/scala#10484 The release notes as follows: - https://github.com/scala/scala/releases/tag/v2.13.12 ### Does this PR introduce _any_ user-facing change? Yes, Scala version changed. ### How was this patch tested? Pass Github ### Was this patch authored or co-authored using generative AI tooling? NO Closes #43185 from LuciferYang/SPARK-45331. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Looked through Parser, Scanner, Namer, Typer and RefChecks.
Some ad hocery in
a.toDoublevs(a + a).toDouble)Hopefully acceptable for the use case.