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:x,y is -Wconf:x -Wconf:y #10708

Open
wants to merge 2 commits into
base: 2.13.x
Choose a base branch
from
Open

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Mar 8, 2024

-Wconf applies the "last matching" configuration, or equivalently, the "first matching" in reverse order. User config has precedence over default config.

However, previously, two configs in a "comma-separated" setting were taken such that the "first matching" in order as written had precedence. This is different from Scala 3 and possibly unintuitive or awkward to document.

This behavior is corrected so that a config always overrides or has precedence over previous configs, including a comma-separated setting as read from left to right.

Example:

-Wconf:A,B -Wconf:C
is now the same as
-Wconf:A -Wconf:B -Wconf:C
where "the last applicable configuration wins".
Previously, it was taken as
-Wconf:B -Wconf:A -Wconf:C

If configs A and B are "overlapping" filters, then swapping the order will change the resulting behavior.

Don't reverse before prependedAll.

Noticed at scala/scala3#19885 (comment)

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Mar 8, 2024
@som-snytt som-snytt marked this pull request as ready for review March 8, 2024 20:36
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Mar 8, 2024
@SethTisue SethTisue requested a review from lrytz March 8, 2024 23:41
@lrytz
Copy link
Member

lrytz commented Mar 11, 2024

This looks like the right solution, but a breaking change. In short, -Wconf:a,b,c needs to be changed to -Wconf:c,b,a in case there are overlapping filters. -Wconf:a -Wconf:b remains the same.

@SethTisue what do you think?

Small usability thing:

➜ sandbox spr 10708 -Wconf:msg=ArrowAssoc:e,msg=instead:s

scala> :settings
-Wconf = List(msg=instead:s, msg=ArrowAssoc:e, cat=deprecation:ws, cat=feature:ws, cat=optimizer:ws)

they are displayed the other way around

@SethTisue SethTisue self-assigned this Mar 11, 2024
@SethTisue
Copy link
Member

SethTisue commented Mar 13, 2024

I think Scala 3 chose the right design here and we should align with it, especially since -Wconf:x,y probably isn't super common in practice. It's always unfortunate to make a breaking change, but it's more permissible, at least, to break compile-time things.

I can make sure we very prominently release-note the change.

But let's hold it for 2.13.15, to keep 2.13.14 focused on bugfixes.

@SethTisue SethTisue modified the milestones: 2.13.14, 2.13.15 Mar 13, 2024
@lrytz
Copy link
Member

lrytz commented Mar 13, 2024

Scala 3 chose the right design here

It was by mistake, but yes :-)

break compile-time things

There are two modes in which it can break

  • in a:s,b:e, some messages are no longer silenced and the build can start failing
  • in a:e,b:s, some messages are no longer fatal. This can go unnoticed, unfortunately.

@SethTisue
Copy link
Member

SethTisue commented Mar 13, 2024

some messages are no longer fatal. This can go unnoticed, unfortunately

Yeah. Let's be very explicit about that in the PR description and release notes.

@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 13, 2024

it's "just a bug".

I could [not -Ed.] add a red flag warning for the comma-separated option. It's "probably" used "rarely".

The PR includes important debug toString as well. I should change the PR title.

Edit: the highly engineered settings infrastructure doesn't have a way to report warnings.

Edit: I tried to update the PR comment, but was unable to make it sound like anything but the ravings of a lunatic.

@som-snytt
Copy link
Contributor Author

Relatedly scala/bug#12984

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants