Changes to -Xfatal-warnings lead to a UX regression in Scala tooling

[vasilmkd]

This ticket is a follow up UX report/discussion following the changes in
#18634
and specifically, #19245 (this is the final PR, but the other PRs are relevant as well).

Compiler version

3.4.1-RC1 and above

Minimized code

def fn(n: Int): String =
  val abc = 123
  val dfe = 456
  val xyz = 789
  n.toString

Necessary compiler flags

scalac -Xfatal-warnings -Wunused:locals

Output Scala 3.3.3

-- Error: warnings.scala:2:6 ---------------------------------------------------
2 |  val abc = 123
  |      ^^^
  |      unused local definition
-- Error: warnings.scala:3:6 ---------------------------------------------------
3 |  val dfe = 456
  |      ^^^
  |      unused local definition
-- Error: warnings.scala:4:6 ---------------------------------------------------
4 |  val xyz = 789
  |      ^^^
  |      unused local definition
3 errors found

Output Scala 3.4.1-RC2

-- Warning: warnings.scala:2:6 -------------------------------------------------
2 |  val abc = 123
  |      ^^^
  |      unused local definition
-- Warning: warnings.scala:3:6 -------------------------------------------------
3 |  val dfe = 456
  |      ^^^
  |      unused local definition
-- Warning: warnings.scala:4:6 -------------------------------------------------
4 |  val xyz = 789
  |      ^^^
  |      unused local definition
No warnings can be incurred under -Werror (or -Xfatal-warnings)
3 warnings found
1 error found

Screenshots from Metals

1. When using Bloop as the build server, Bloop manually upgrades warnings to fatal errors, in the presence of -Xfatal-warnings (but not -Werror, separate issue):

2. When using sbt as the build server, the warnings stay warnings, and the No warnings can be incurred under -Werror (or -Xfatal-warnings) error message is not reported anywhere. I believe this is due to this error message being issued without any source and position information.

3. A control screenshot of using Scala 3.3.3, to establish a baseline expecation (captured using sbt as the build server, but Bloop is the same):

Discussion (originally posted in a Scala Center - VirtusLab - JetBrains collaboration channel, copied almost verbatim)

Let’s examine the -Xfatal-warnings/-Werror change at a high level. We used to have compiler messages that could be treated as self-contained pieces of compilation information. This was achieved through the xsbti.Problem interface, which contains all info that we might need to present a compiler issued message, be it in a build tool, or an editor.

With this latest change, a precedent has been set, each xsbti.Problem is no longer self-contained. Instead, multiple xsbti.Problem instances are part of a larger context, and need to be evaluated together, in order to decide whether an xsbti.Problem needs to be modified after the fact.

And we already have inconsistencies because of this. Sbt (in the terminal) shows warnings and a single error at the end, just like the compiler issues the messages (i.e. does no further post processing). When using Metals with sbt, this is what the user will see as well (without the No warnings can be incurred error message, whose absence makes things more confusing). From a user experience perspective, this is a regression. Scala 3.3 with -Xfatal-warnings issues errors, and Scala 3.4 with the same -Xfatal-warnings issues warnings, so the user will start seeing warning highlights in the editor, where they used to see error highlights prior to updating the Scala version. This will seem surprising to users, thinking that -Xfatal-warnings is ignored.

The same is happening in IntelliJ IDEA as well, when using the compiler for error highlighting.

On the other hand, Bloop manually upgrades the warnings into errors. This has been confirmed by @tgodzik , and is an old feature of Bloop. I'm glad it exists and I'm using it as an illustration of UX inconsistencies in Scala tooling, in the hopes of converging on a consistent presentation to users.

What I would like to discuss is, given that this precedent has been set, how do we converge on a consistent UX across the tools and editors. And given that there are intentions to make Scala 3.4 even more resilient in the face of compilation errors, will this be the only case of “manually upgrading warnings to errors”, or will there be other, different combinations of messages where we need to do post processing on the compiler messages.

I’m tagging @szymon-rd and @nicolasstucki as the people who were directly involved in the implementation and review of this change, but anyone else is more than welcome to join the discussion.

Thanks in advance.

[nicolasstucki]

When using sbt as the build server, the warnings stay warnings, and the No warnings can be incurred under -Werror (or -Xfatal-warnings) error message is not reported anywhere. I believe this is due to this error message being issued without any source and position information.

We could try to set the source position to the start of the file that had the warnings

https://github.com/scala/scala3/pull/19245/files#diff-4394290a15677dcdaf17527ae209ee5651a5e5c169832896b506fa98dfc15ed3R214

[vasilmkd]

I would say that this ticket should be marked as a blocker and Scala 3.4.1 should not be released without a clear resolution to this ticket.

I don't mind whatever decision is reached, as long as there are clear action points that all tooling will collectively take on this matter.

That's what I'm hoping this discussion will help bring.

Currently, the compiler is presenting a clear regression compared to previous releases and it will be a burden on the tooling maintainers to work around and explain the situation to confused users.

[sjrd]

/cc @Kordyjan and @Gedochao about release management (see comment above this one).

[som-snytt]

A first step would be to use the name -Werror in preference to the old name -Xfatal-warnings.

The scalac help says:

  -Werror                        Fail the compilation if there are any warnings. [false]

The javac help says:

  -Werror                      Terminate compilation if warnings occur

The scalac help also says:

  -Wconf:<patterns>              Configure reporting of compiler warnings; use `help` for details.

The -Wconf:help doesn't have a verb for what an action does, but it means "how to report a given warning selected in some way by the config".

The options serve different purposes. -Wconf is for "elevating warning to error" or "suppressing warning or making it mere info".

-Werror is just "fail compilation if there were warnings".

[LucySMartin]

Honestly, I'm very surprised that under the original ticket, there was not a new level of message added (potentially transparently to the end user) to split ERRORs where we can keep going somewhat, and ERRORs where a firm stop is needed.
This is not just going to impact immediate tools, but a lot of downstream things as well.
I know that I have a system that will email me any ERROR log lines from the weekly builds, and I either have to fire up an investigation myself, or triage it purely based on the error messages (and an auto generated blame file of the surrounding lines). I'm thus of the opinion that any issue that will deterministically cause the build to fail should be listed as an error in the output logs.
While it is a more systemic change than is perhaps suitable if a release is being held, could we consider having an internal split of ERROR_STOP / ERROR / WARNING such that internally we treat ERROR as a WARNING under the current system, but send it to the output logs as an ERROR, so that existing tools will pick up on all of the locations that have eventually lead to an actual ERROR_STOP.

[Gedochao]

@vasilmkd Hey, so this change was intentional on our side. It's also intentional for it to land in 3.4.1, no minor version bump is necessary, it doesn't break any compatibility guarantees. It's an internal API behaviour change, so it shouldn't be considered as a regression.

Changes on the tooling side of things will be required.

I suppose we could have communicated it stronger, however the 3.4.1-RC1 & 3.4.1-RC2 have been out for a while now, available to be tested with tooling.

I would say that this ticket should be marked as a blocker and Scala 3.4.1 should not be released without a clear resolution to this ticket.

The 3.4.1 release process is tracked in here:

• 3.4.1 & 3.4.2-RC1 Release procedure #20024
  3.4.1 artifacts are available on Maven right now, the announcement will go soon.
  I see no reason to halt the release process at this point.

What I would like to discuss is, given that this precedent has been set, how do we converge on a consistent UX across the tools and editors. And given that there are intentions to make Scala 3.4 even more resilient in the face of compilation errors, will this be the only case of “manually upgrading warnings to errors”, or will there be other, different combinations of messages where we need to do post processing on the compiler messages.

There could be more changes similar to this one in future versions, including patch versions. We will try to communicate them better, and also they should be available to test with tooling via the RC versions. Generally speaking, any such change is available for testing at least 1 month before a release happen.

Now, as for the changes required on the tooling side. Manually upgrading warnings to errors as you have suggested is definitely one of the possible solutions. I am not sure if it'll be the optimal course of action, it needs to be judged case by case. Every tool might want to do it differently.

The current behaviour is especially useful for terminal-based tools (i.e. sbt), because it clearly distinguishes warnings from errors, as they are treated as different entities even with -Xfatal-warnings enabled.

[vasilmkd]

@Gedochao thanks for clearing things up.

We will manually promote the warnings to errors in IntelliJ IDEA. I suppose that other tooling will do the same.

I'm personally satisfied with that outcome, as I had indicated in my comment.

I no longer consider this a blocker.

[unkarjedy]

AFAIK the best public place describing compiler options behaviour is
https://docs.scala-lang.org/scala3/guides/migration/options-lookup.html
right?

If so, it will require clarifications for 3.4.x as it states

-Werror elevates warnings to errors

and not

-Werror adds a new compilation error identifying that there are unexpected warnings

[unkarjedy]

@Gedochao
From original issue

When warnings are reported in a phase and -Werror (XfatalWarnings) is on, they will be reported as errors and compilation will crash. Because of that, instead of reporting all warnings from linting in a unit, the compilation will exit with an error after just one phase - therefore it does not report warnings from future phases. it would make more sense to aggregate these warnings and report them as errors after the linting phases.

... they will be reported as errors and compilation will crash. Because of that ...

Have you considered changing the compiler in a way that it does not crash on some errors?
E.g. something like described in the comment: #20041 (comment)
It could be completely compiler-internal and for the public API it could still be an ERROR

[vasilmkd]

To be completely clear, I welcome the change and think that aggregating all warnings to issue together is a great improvement.

I'm just not at all convinced that leaving the warnings as warnings, and issuing an obscure message at the end, just to satisfy a requirement that the compilation finishes with an error is good UX, regardless of which tool will consume the messages. It is clearly confusing in the presence of -Werror, the way the flag is documented.

I would be very interested to hear arguments for keeping the warnings as warnings, and not promoting them to errors.

[rochala]

It is clearly confusing in the presence of -Werror, the way the flag is documented.

The documentation I see states that:

val XfatalWarnings: Setting[Boolean] = BooleanSetting(WarningSetting, "-Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings"))

As I see it, it does exactly the thing. It states nothing about promoting warnings to errors. But that is not the point.

I have one argument that may or may not convince you:
Warnings are usually of way less significance than errors in your code, thus when you are working on code they can be very easily mistaken under -Werror. Sometimes a single change can result in multiple warnings being emitted, such as removing a file can result in unused.
This can easily create a mass of errors, where some of them are related to the actual change you've made and some are just warnings of very low importance (unused).

In sbt shell, it is all working together, as you can see the No warnings can be incurred under -Werror (or -Xfatal-warnings) error at the end of the compilation.

If we consider it from the IDE perspective, thou it may be a bit problematic. I see 3 ways as of today on how they can display them in "Problems" tab:

1. Promote them to error - this is the old behaviour, it works, but I think it can be better.
2. Mark them in different way e.g - orange (but this may not be easily done in lets say Metals as we rely on DiagnosticKinds)
3. Show them as warnings + add error message from -Werror to the list.

I see the 3rd option as best one but the problem is, where to put this error ?
If we could do any change no matter what, I'd personally would group them together and label the group with -Werror message.

[vasilmkd]

Your argument is very reasonable.

As I said, it is a precedent to no longer promote errors. I'm not making a judgement on whether this is a good or a bad thing.

I'm sorry I had a wrong notion of the -Werror documentation.

In sbt shell, it is all working together, as you can see the No warnings can be incurred under -Werror (or -Xfatal-warnings) error at the end of the compilation.

This is what happens in the IDEA build window too.

My concerns are more around the presentation in the editor itself. I would like all tooling to be consistent here, and why I raised the discussion in the first place.

With the Bloop vs sbt inconsistency in Metals, I could easily see a confusion arising with people using one build server and reporting an issue, and the maintainers having trouble reproducing the issue because the project/tooling config has not been fully specified in the report. It happens all the time with user reports in IDEA.

[Gedochao]

Seems like the core concerns have been addressed.
I'm closing the issue.

[unkarjedy]

@Gedochao Please take a look at my seemingly-missed comment
#20041 (comment)

The page requires a simple patch, updating the -Werror description, just to avoid any potential confusion in future