Backport #18503 to the `3.3.x` LTS series

[BalmungSan]

I am aware of https://github.com/scala/scala3/pull/20282#issuecomment-2211981177

However, IMHO, the already "wrong" order that Scala 3 had until #18503 was merged should be considered like a bug and fixed. While I understand the current order doesn't need to be considered "wrong" per se, note that at least IME it is common to have general rules defined at the top of your build definitions and then provide more specific rules on specific modules. Such an approach, at least in sbt, leads to more specific rules being added later than the general ones; meaning they appear at their right. And sincerely, I don't want to mess with my build just to change the order of flags; especially when in future versions the order will be changed again. I also, want to point out that since the behavior is different from Scala 2, making a cross-build for 2.13.15 & 3.3.4 would be extremely painful because now you need to maintain two different orders.

I do understand the rationale of not changing semantics in a patch release. But I do wonder if anyone is finding the current semantics useful and preferable over the other one. I wonder, how many projects would break with the change, and how hard would be fixing them. I also wonder how many projects are not using the Scala 3 linting to its full potential because they need this.

---

For the record, we found this problem while trying to use the new linting options in Scala 3: https://github.com/neotypes/neotypes/pull/745 We wanted to make all warnings verbose but silence UnusedNonUnitValue for ScalaTest Assertions, and at the end we simply decided to opt out of the verbosity: https://github.com/neotypes/neotypes/pull/745/commits/2253fb3dc96ef9e3f2e538bf705622337ab98d87

[SethTisue]

note that at the time this was previously considered (on 20282), Scala 2 hadn't yet aligned with the Scala 3.4+ behavior (in 2.13.15), so circumstances are different now, as Luis points out by writing:

making a cross-build for 2.13.15 & 3.3.4 would be extremely painful because now you need to maintain two different orders

I'm rather torn now about what's best. changing behavior in a patch release still feels wrong, but Luis's arguments are strong, also.

[Gedochao]

We will discuss this on the next core meeting.

[SethTisue]

@WojciechMazur has pointed out that a workaround exists for cross-building: never use the comma form -Wconf:x,y, always use separate options -Wconf:x -Wconf:y. if you never use the comma, then all the Scala versions agree on the behavior

[SethTisue]

At core meeting, opinion was initially about evenly divided. Everyone acknowledged that there are strong arguments on both sides.

But once we realized the workaround exists for cross-building, that tipped sentiment against backporting.

[SethTisue]

Based on this, I added an "Advice on cross-building" section to the PR description of https://github.com/scala/scala/pull/10708

[BalmungSan]

has pointed out that a workaround exists for cross-building: never use the comma form -Wconf:x,y, always use separate options -Wconf:x -Wconf:y

@SethTisue that is just plain wrong. We don't use the comma form, we only use the separate options form, that is the one that is broken, the one I reported in my example.

The problem is that in 3.3.x doing -Wconf:x -Wconf:y means that x wins over y, whereas in Scala 2 the opposite is true. That is what #18503 fixed.

[SethTisue]

The problem is that in 3.3.x doing -Wconf:x -Wconf:y means that x wins over y

are you sure? @prolativ did some experiments and I don't think that's what his results showed

Michał, maybe you could attach your code and the table of the results?

[BalmungSan]

@SethTisue maybe sbt is needed to trigger the wrong behavior? If so, then could this be a sbt bug instead? But why would it only affect Scala 3?

I know a full project is far from a minimal reproducer; I can work in one if you all want / need one. But, take a quick look at this PR on neotypes: https://github.com/neotypes/neotypes/pull/745 the upgraded sbt-tpolecat version enabled UnusedNonUnitValue warning in Scala 3 (as expected and wanted). However, we need to silence it only for ScalaTest Assertions. For Scala 2 we used this: -Wconf:cat=other-pure-statement&msg=Assertion:s, and for Scala 3 we did this: -Wconf:name=UnusedNonUnitValue&msg=Assertion:s. But, that didn't work.

After some investigation, I found that we also made all warnings verbose using: -Wconf:any:verbose, this is part of a lazy val commonSettings = Def.settings(...) at the top of the build. And then, in the tests module, we do .settings(commonSettings) BEFORE the .settings(...) block where we add the silence config. I found that if I commented / removed that config then the silence would actually work: https://github.com/neotypes/neotypes/commit/2253fb3dc96ef9e3f2e538bf705622337ab98d87#diff-5634c415cd8c8504fdb973a3ed092300b43c4b8fc1e184f7249eb29a55511f91R66

AFAIK, all sbt is doing here is concatenating the scalacOptions and thus, -Wconf:name=UnusedNonUnitValue&msg=Assertion:s should appear AFTER -Wconf:any:verbose.

[SethTisue]

@BalmungSan well, the devil's in the details. show scalacOptions, and/or running compile in debug mode, should show you what actual flags are actually being passed to the compiler in what order

I can work in one if you all want / need one

yes please. any decision on this ticket hinges on whether an acceptable workaround exists

[SethTisue]

here is an example showing the rightmost separate flag taking precedence in 3.3.4:

% scala-cli -S 3.3.4 -Wconf:cat=deprecation:s -Wconf:cat=deprecation:e -e "scala.Proxy"
Compiling project (Scala 3.3.4, JVM (17))
[error] snippet:1:1
[error] object Proxy in package scala is deprecated since 2.13.0: All members of this object are deprecated.
Error compiling project (Scala 3.3.4, JVM (17))
Compilation failed
% scala-cli -S 3.3.4 -Wconf:cat=deprecation:e -Wconf:cat=deprecation:s -e "scala.Proxy"
Compiling project (Scala 3.3.4, JVM (17))
Compiled project (Scala 3.3.4, JVM (17))

[BalmungSan]

@SethTisue

show scalacOptions

Output

sbt:root> clean;reload
[success] Total time: 0 s, completed Oct 23, 2024, 11:54:41 AM
[info] welcome to sbt 1.10.2 (GraalVM Community Java 21.0.2)
[info] loading settings for project global-plugins from plugins.sbt ...
[info] loading global plugins from /home/balmungsan/.sbt/1.0/plugins
[info] loading settings for project neotypes-build from Plugins.sbt ...
[info] loading project definition from /home/balmungsan/neotypes/project
[info] loading settings for project root from build.sbt,version.sbt ...
[info] resolving key references (18917 settings) ...
[info] set current project to root (in build file:/home/balmungsan/neotypes/)
sbt:root> ++3.3.4
[info] Setting Scala version to 3.3.4 on 16 projects.
[info] Reapplying settings...
[info] set current project to root (in build file:/home/balmungsan/neotypes/)
sbt:root> show tests/Test/scalacOptions
[info] * -encoding
[info] * utf8
[info] * -deprecation
[info] * -feature
[info] * -unchecked
[info] * -language:experimental.macros
[info] * -language:higherKinds
[info] * -language:implicitConversions
[info] * -Ykind-projector
[info] * -Wvalue-discard
[info] * -Wnonunit-statement
[info] * -Wunused:implicits
[info] * -Wunused:explicits
[info] * -Wunused:imports
[info] * -Wunused:locals
[info] * -Wunused:params
[info] * -Wunused:privates
[info] * -Xfatal-warnings
[info] * -release:17
[info] * -Wconf:any:verbose
[info] * -Yretain-trees
[info] * -Wconf:name=UnusedNonUnitValue&msg=Assertion:s
[success] Total time: 1 s, completed Oct 23, 2024, 11:55:06 AM

running compile in debug mode

Output

[info] compiling 28 Scala sources to /home/balmungsan/neotypes/tests/target/scala-3.3.4/test-classes ...
[debug] Returning already retrieved and compiled bridge: /home/balmungsan/.cache/coursier/v1/https/repo1.maven.org/maven2/org/scala-lang/scala3-sbt-bridge/3.3.4/scala3-sbt-bridge-3.3.4.jar.
[debug] [zinc] Running cached compiler 25ee4645 for Scala Compiler version 3.3.4
[debug] [zinc] The Scala compiler is invoked with:
[debug]   -encoding
[debug]   utf8
[debug]   -deprecation
[debug]   -feature
[debug]   -unchecked
[debug]   -language:experimental.macros
[debug]   -language:higherKinds
[debug]   -language:implicitConversions
[debug]   -Ykind-projector
[debug]   -Wvalue-discard
[debug]   -Wnonunit-statement
[debug]   -Wunused:implicits
[debug]   -Wunused:explicits
[debug]   -Wunused:imports
[debug]   -Wunused:locals
[debug]   -Wunused:params
[debug]   -Wunused:privates
[debug]   -Xfatal-warnings
[debug]   -release:17
[debug]   -Wconf:any:verbose
[debug]   -Yretain-trees
[debug]   -Wconf:name=UnusedNonUnitValue&msg=Assertion:s
[debug]   -classpath
// Extremely long classpath ommited
[error] -- [E176] Potential Issue Error: /home/balmungsan/neotypes/tests/src/test/scala/neotypes/AsyncGuaranteeSpec.scala:18:45
[error] 18 |      withClue("Finalizer was not called -") {
[error]    |      ^
[error]    |      unused value of type org.scalatest.compatible.Assertion
[error] 19 |        this.counter should not be 0
[error] 20 |      }
[error] Matching filters for @nowarn or -Wconf:
[error]   - id=E176
[error]   - name=UnusedNonUnitValue
// More errors

[SethTisue]

@BalmungSan works for me:

% scala-cli -S 3.3.4 -Wnonunit-statement -Wconf:any:verbose -Wconf:name=UnusedNonUnitValue:s -e "def foo = { scala.Option ; 0 }"
Compiling project (Scala 3.3.4, JVM (17))
Compiled project (Scala 3.3.4, JVM (17))

whatever is going on in your codebase must more specific? if you get to the bottom of it, you can report it as a bug

maybe it's specific to msg= filtering somehow?

[SethTisue]

In all of this, we have been assuming that we can trust scala-cli to preserve compiler option order, but what if we can't trust it?

% cat S.scala
def foo = { scala.Option; 0 }
% /usr/local/scala/scala3-3.3.3/bin/scalac -Wnonunit-statement -Wconf:any:verbose  -Wconf:name=UnusedNonUnitValue:s  S.scala
-- [E176] Potential Issue Warning: S.scala:1:18 --------------------------------
1 |def foo = { scala.Option; 0 }
  |            ^^^^^^^^^^^^
  |            unused value of type Option.type
Matching filters for @nowarn or -Wconf:
  - id=E176
  - name=UnusedNonUnitValue
1 warning found
% scala-cli --version
Scala CLI version: 1.5.1
Scala version (default): 3.5.1
% scala-cli compile -S 3.3.3 -Wnonunit-statement -Wconf:any:verbose  -Wconf:name=UnusedNonUnitValue:s S.scala  
Compiling project (Scala 3.3.3, JVM (17))
Compiled project (Scala 3.3.3, JVM (17))

😱😱😱😱😱

[SethTisue]

I think this means we now need to question pretty much everything we have said or thought about this.

Oy.

[SethTisue]

@Gedochao perhaps another fix similar to https://github.com/VirtusLab/scala-cli/pull/2667 is needed?

[BalmungSan]

@SethTisue minimized repository: https://github.com/BalmungSan/reproduce-scala3-bug-21818

You can see that if you clone and run sbt compile you will get a warning despite the silence. Then, if you comment the -Wconf:any:verbose in build.sbt and clean;reload;compile it does compile without warnings now.

[som-snytt]

I haven't caught up with this conversation yet, but compare https://github.com/scala/bug/issues/12984, recently fixed on Scala 2, for an example where unexpected application order of settings state can be very confusing. I don't think I compared the behavior on Scala 3 at that time.

-Wconf:help on Scala 2:

User-defined configurations override previous settings, such that the last matching configuration defines the action for a given diagnostic message.

Oh but after upgrading to 3.5.2 the help works now but still says:

User-defined configurations are added to the left. The leftmost rule matching a warning message defines the action.

which is the opposite of what it should say, or at least obscure. I think it intends to say that rules are accumulated in reverse order, where the "defaults" wind up on the right, and user config on the left with last config in first position. (Edit: I see I updated the words on Scala 2, but both help messages are describing the same behavior.)

The repo shows 3.3.x not yet working such that the last config wins.

(Edit: I'm not sure whether "first wins" or "last wins" is more intuitive, in terms of "order of args".)

[Gedochao]

@Gedochao perhaps another fix similar to VirtusLab/scala-cli#2667 is needed?

@SethTisue Yep, seems like it, let me investigate a bit...

[prolativ]

I checked it too now and indeed it looks the results of my experiment were wrong because of a bug in scala-cli, which ignores all repeated -Wconf settings except for the last one (when passing options by command line) or except for the first one (when passing options via a directive in a file)

[prolativ]

I reran my experiment with the fixed version of scala-cli and here are the results:

	2.13.14	2.13.15	3.3.4	3.5.2
-Wconf:cat=deprecation:e -Wconf:cat=deprecation:s	S	S	E	S
-Wconf:cat=deprecation:e,cat=deprecation:s	E	S	E	S
-Wconf:cat=deprecation:s -Wconf:cat=deprecation:e	E	E	S	E
-Wconf:cat=deprecation:s,cat=deprecation:e	S	E	S	E

(S - silent, E - error)

And here's the script used to generate this table in markdown in case someone has some doubts about the results:

//> using scala 3.5.2

import java.nio.file.{Files, Paths}

val testedSource = """
object WConfTest {
  def main(args: Array[String]): Unit = Deprecated.deprecated
}

object Deprecated {
  @deprecated("", "")
  def deprecated = ()
}
"""

val scalaVersions = List("2.13.14", "2.13.15", "3.3.4", "3.5.2")
val compilerOptionsLists = List(
  List("-Wconf:cat=deprecation:e", "-Wconf:cat=deprecation:s"),
  List("-Wconf:cat=deprecation:e,cat=deprecation:s"),
  List("-Wconf:cat=deprecation:s", "-Wconf:cat=deprecation:e"),
  List("-Wconf:cat=deprecation:s,cat=deprecation:e")
)

@main def test() =
  import scala.sys.process.*

  // prepare sources

  val sourceFilePath = "/tmp/WConfTest.scala"

  Files.write(
    Paths.get(sourceFilePath),
    testedSource.getBytes
  )

  val wasSuccess = scala.collection.mutable.Map.empty[(String, List[String]), Boolean]

  ////////////////////

  // run tests

  for 
    scalaVersion <- scalaVersions
    compilerOptionsList <- compilerOptionsLists
  do
    println(s"Scala: $scalaVersion")
    println(s"Compiler options: ${compilerOptionsList.mkString(" ")}")
    val command = List(
      "scala-cli", "--cli-version", "1.5.1-29-gc083024e3-SNAPSHOT", "compile" , sourceFilePath, "-S", scalaVersion
    ) ++ compilerOptionsList
    val commandResult = command.!
    val compiledSuccessfully = commandResult == 0
    wasSuccess((scalaVersion, compilerOptionsList)) = compiledSuccessfully
    if compiledSuccessfully then
      println("Compiled successfully")
    else
      println("Compilation error")

  /////////////////////

  // print results summary in markdown

  val headers = "" +: scalaVersions
  val headersLine = headers.mkString("| ", " | ", " |")
  val headersSeparatorsLine = List.fill(headers.length)("---").mkString("| ", " | ", " |")
  val rows = compilerOptionsLists.map { optionsList =>
    val versionResult = scalaVersions.map(version => if wasSuccess((version, optionsList)) then "S" else "E")
    optionsList.mkString(" ") +: versionResult
  }
  val rowLines = rows.map(_.mkString("| ", " | ", " |"))

  val resultsMarkdownTable =
    (headersLine +: headersSeparatorsLine +: rowLines).mkString("\n")

  println(resultsMarkdownTable)

[prolativ]

The issue was rediscussed by the compiler's core team and the general consensus this time (given that there's no workaround or an alternative smooth migration path) was that this should be backported to scala 3.3.5. Even though there's a theoretical possibility to break somebody's code when bumping the compiler version from the current LTS to 3.3.5, such a situation was assessed to be not very likely. On the other hand backporting might help one avoid problems when migrating from 2.13.15 to 3.LTS or from 3.LTS to 3.Next

[som-snytt]

The reason BalmungSan is not still complaining is that the backport is in 3.3.5.

https://github.com/scala/scala3/pull/21985

I confirmed the sample repo works the same on Scala 2, 3.3.5, 3.6.4, 3.7. I'll go ahead and delete my local clone now, just spring cleaning.