make refutable patterns an error in 3.4

[bishabosha]

was originally targeting 3.3 - opening this to see which community build projects fail

[bishabosha]

@sjrd so far this requires patching the sjsJUnitTests sources, what do you recommend to do here? on latest 2.12 and 2.13 these should accept case in a for comprehension

[sjrd]

The Scala.js tests have to pass with older 2.12 and 2.13 versions, all the way back to 2.12.2 and 2.13.0.

What's the magnitude of this problem? Is it a couple places, or is it everywhere? Can you give examples?

[bishabosha]

The failure in the CI points to these files:

• test-suite/js/src/test/scala/org/scalajs/testsuite/niocharset/UTF16Test.scala
• test-suite/js/src/test/scala/org/scalajs/testsuite/scalalib/ArrayTest.scala
• test-suite/js/src/test/scala/org/scalajs/testsuite/library/ObjectTest.scala
• test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/TupleTest.scala

in total there are 7 lines affected - all refutable patterns in for comprehension or pattern val bindings

[sjrd]

The ones in UTF16Test and ArrayTest can be changed to use an explicit match with a fail("message") in the default case.

In ObjectTest and TupleTest, I think there's a deeper issue. It seems they are all related to extracting js.TupleN(...) from something that is known to be a js.TupleN (with the same N), so they should be irrefutable. Perhaps we messed up the definition of those extractors? There are here:
https://github.com/scala-js/scala-js/blob/v1.12.0/library/src/main/scala/scala/scalajs/js/Tuple.scala
https://github.com/scala-js/scala-js/blob/v1.12.0/library/src/main/scala/scala/scalajs/js/Tuple.nodoc.scala

[bishabosha]

js.Tuple2.unapply (and the rest) should have Some[(T1, T2)] result, (currently its Option[...]) otherwise it's not statically known to be always matching

[bishabosha]

these community-build projects still need patching to pass community_build_c:

• utest
• scala-collection-compat
• scala-xml
• scalaz
• protoquill
  Edit: see below for updates list as of 28/09/2023

[bishabosha]

updated for 3.4 - unsure if the patches are still needed for various community projects

[bishabosha]

list of community build projects still failing:

• dotty.communitybuild.CommunityBuildTestC.protoquill
• dotty.communitybuild.CommunityBuildTestC.pprint
• dotty.communitybuild.CommunityBuildTestC.scalaz
• dotty.communitybuild.CommunityBuildTestC.scalaXml
• dotty.communitybuild.CommunityBuildTestC.requests
• dotty.communitybuild.CommunityBuildTestC.scalaCollectionCompat
• dotty.communitybuild.CommunityBuildTestC.upickle
• dotty.communitybuild.CommunityBuildTestC.cask
• dotty.communitybuild.CommunityBuildTestC.geny
• dotty.communitybuild.CommunityBuildTestC.fansi
• dotty.communitybuild.CommunityBuildTestC.oslib
• dotty.communitybuild.CommunityBuildTestC.ujson
• dotty.communitybuild.CommunityBuildTestC.utest
• dotty.communitybuild.CommunityBuildTestC.parboiled2

edit: all passing now

[bishabosha]

The problematic Scala.js tests will have to remain ignored until base Scala.js version is updated to 1.13.0+ (for a separate PR)

[sjrd]

I'm not completely sure, but I don't this ever went through the SIP process. Since it turns previously valid (and not unsound) code into an error by design, it seems to me that it affects the language spec and therefore needs a SIP.

Perhaps alternatively, it could remain a configurable error. Scala 2 has enough configuration options with -W flags and @nowarn annotations that some things can be errors by default but can be downgraded to warnings at will. This would be a good candidate for such a treatment.

[bishabosha]

Perhaps alternatively, it could remain a configurable error. Scala 2 has enough configuration options with -W flags and @nowarn annotations that some things can be errors by default but can be downgraded to warnings at will. This would be a good candidate for such a treatment.

The thing is this isn't just a lint - the code generation with this change no longer calls withFilter. Previously in 3.3 the code calls withFilter even for irrefutable patterns

[sjrd]

So this was part of https://docs.scala-lang.org/scala3/reference/changed-features/pattern-bindings.html, which was in the set of proposals that were accepted by the SIP committee while leading up to Scala 3. So seems fine to me.

[bishabosha]

I think it'd be good to address #18621 and #18622 in the same release

[bishabosha]

I rebased to fix the conflict, I'll work on only making only the for-comprehension change, in anticipation of the checkedCast SIP if that will occur

[bishabosha]

close in favor of #18842