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.