scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.72k stars 1.04k forks source link

-Wconf should apply leftmost matching rule #19885

Open lrytz opened 4 months ago

lrytz commented 4 months ago

The -Wconf:help text also says the leftmost matching rule should apply.

Scala 2

➜ sandbox s -Wconf:any:e,cat=deprecation:s
Welcome to Scala 2.13.13 (OpenJDK 64-Bit Server VM, Java 21.0.1).

scala> 1 → 2
         ^
       error: method → in class ArrowAssoc is deprecated (since 2.13.0): Use `->` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code.

Scala 3

➜ sandbox s3 -Wconf:any:e,cat=deprecation:s
Welcome to Scala 3.4.0 (21.0.1, Java OpenJDK 64-Bit Server VM).

scala> 1 → 2
val res0: (Int, Int) = (1,2)

➜ sandbox s3 -Wconf:any:s,cat=deprecation:e
Welcome to Scala 3.4.0 (21.0.1, Java OpenJDK 64-Bit Server VM).

scala> 1 → 2
-- Error: ----------------------------------------------------------------------
1 |1 → 2
  |^^^
  |method → in class ArrowAssoc is deprecated since 2.13.0: Use `->` instead. If you still wish to display it as one character, consider using a font with programming ligatures such as Fira Code.
povder commented 4 months ago

What do you think a way forward should be? Should the help message be fixed or should the behaviour change to match the help message?

I feel changing the behaviour would be breaking too much. While it's not ideal the behaviour doesn't match Scala 2 IMO it shouldn't be changed at this point.

lrytz commented 4 months ago

A related potential issue is when passing multiple -Wconf flags to the compiler, but probably it's OK to rely on the build tool to not reorder settings, like project.settings("-a", "-b").settings("-c", "-d").

In a multi-module build one might want to have a base -Wconf setting and refinements in sub-projects:

val commonSettings = Seq(scalacOptions += "-Wconf:any:e")
val a = project
  .settings(commonSettings)
  .settings(scalacOptions += "-Wconf:cat=deprecation:s")

Fortunately the above works in both Scala 2 and 3

The discrepancy is the precedence within a single -Wconf, as shown in the bug report; it can trip up people that migrate or cross-build.

So I'm not sure; I think the reason I implemented "leftmost" in Scala 2 is because left-to-right seems more natural. But then -Wconf:x -Wconf:y is equivalent to -Wconf:y,x, which is also not obvious. In Scala 3 we have rightmost, -Wconf:x -Wconf:y is -Wconf:x,y. Maybe it's better to keep that and adjust the documentation.

som-snytt commented 4 months ago

If two "matchers" match a warning (one rule makes it an error, the other silences), then the natural "disambiguation" is that the first matching matcher applies.

It's a bit awkward to say "the last matching matcher applies".

Note that there could be any number of strategies, such as "the matcher resulting in error is preferred" or "resulting in silence".

Since there is a "default" configuration, the language about "prepend" (which is an internal detail) suggests "precedence". But maybe lrytz's comment is right and it's just a matter of words. I could not find discussion on old Scala 2.

I don't recall without experiment that -Wconf:x -Wconf:y is different from -Wconf:x,y. I would call that a bug.

Footnote: I don't think "changing behavior" is a hindrance to fixing "bad behavior", especially since the feature is still evolving or improving in scala 3.

lrytz commented 4 months ago

It's a bit awkward to say "the last matching matcher applies".

I guess that's why I did leftmost in Scala 2. But one could argue that the last thing added should count, to allow overriding existing settings / defaults, that doesn't seem too awkward.

I don't recall without experiment that -Wconf:x -Wconf:y is different from -Wconf:x,y. I would call that a bug.

It's a necessary consequence of leftmost precedence in Scala 2. -Wconf:help says

The default configuration is:
  -Wconf:cat=deprecation:ws,cat=feature:ws,cat=optimizer:ws

So when calling scalac -Wconf:cat=deprecation:e, that needs to be added to the left of the default.

som-snytt commented 4 months ago

Figures.

$ git checkout upstream/2.13.x -b test/wconf
fatal: A branch named 'test/wconf' already exists.

I submitted a PR to tweak -Wconf:x,y on scala 2, but did not (yet) amend any verbiage. I think it's just an implementation detail, and we mean "last matching rule" wins.

SethTisue commented 3 months ago

We seem to have an emerging consensus on the Scala 2 PR that Scala 2 should align with Scala 3 on this, even though it's a breaking change.