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

-Wconf should apply leftmost matching rule #19885

Open
lrytz opened this issue Mar 5, 2024 · 6 comments
Open

-Wconf should apply leftmost matching rule #19885

lrytz opened this issue Mar 5, 2024 · 6 comments
Labels
area:linting Linting warnings enabled with -W or -Xlint compat:scala2 itype:bug

Comments

@lrytz
Copy link
Member

lrytz commented Mar 5, 2024

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.
@lrytz lrytz added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 5, 2024
@Gedochao Gedochao added compat:scala2 area:linting Linting warnings enabled with -W or -Xlint and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 6, 2024
@povder
Copy link
Contributor

povder commented Mar 7, 2024

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
Copy link
Member Author

lrytz commented Mar 7, 2024

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

  • In the case of multiple -Wconf options, Scala 2 prepends; the leftmost matching applies
  • Scala 3 appends, the rightmost matching applies

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
Copy link
Contributor

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
Copy link
Member Author

lrytz commented Mar 8, 2024

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
Copy link
Contributor

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:linting Linting warnings enabled with -W or -Xlint compat:scala2 itype:bug
Projects
None yet
Development

No branches or pull requests

5 participants