Backport #18503 (on -Wconf ordering) to the 3.3.x LTS series
#21818
Comments
BalmungSan
added
the
stat:needs triage
|
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:
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
added
itype:bug
area:linting
|
We will discuss this on the next core meeting. |
|
@WojciechMazur has pointed out that a workaround exists for cross-building: never use the comma form |
|
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. |
|
Based on this, I added an "Advice on cross-building" section to the PR description of scala/scala#10708 |
@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 |
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? |
|
@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. After some investigation, I found that we also made all warnings verbose using: AFAIK, all sbt is doing here is concatenating the |
|
@BalmungSan well, the devil's in the details.
yes please. any decision on this ticket hinges on whether an acceptable workaround exists |
|
here is an example showing the rightmost separate flag taking precedence in 3.3.4: |
Output
Output |
|
@BalmungSan works for me: 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 |
|
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? 😱😱😱😱😱 |
|
I think this means we now need to question pretty much everything we have said or thought about this. Oy. |
|
@Gedochao perhaps another fix similar to VirtusLab/scala-cli#2667 is needed? |
SethTisue
changed the title
3.3.x LTS series-Wconf ordering) to the 3.3.x LTS series
|
@SethTisue minimized repository: https://github.com/BalmungSan/reproduce-scala3-bug-21818 You can see that if you clone and run |
|
I haven't caught up with this conversation yet, but compare scala/bug#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.
Oh but after upgrading to 3.5.2 the help works now but still says:
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".) |
@SethTisue Yep, seems like it, let me investigate a bit... |
|
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 |
|
I reran my experiment with the fixed version of scala-cli and here are the results:
(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)
|
|
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. |
I am aware of #20282 (comment)
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.4would 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: neotypes/neotypes#745
We wanted to make all warnings
verbosebut silenceUnusedNonUnitValuefor ScalaTestAssertions, and at the end we simply decided to opt out of the verbosity: neotypes/neotypes@2253fb3The text was updated successfully, but these errors were encountered: