Workaround for Scala 3 regression - #178
Open
raquo wants to merge 3 commits into
Open
Conversation
Contributor
Author
|
Oh, CI fails because |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Heyo! It turns out that newer Scala versions (verified in 3.8.4, 3.9.0, and 3.10-nightly) have a regression that makes Tuplez 0.5.0-M* throw unwarranted "unreachable case" compiler warnings when using in typical Airstream patterns – simple ones, nothing weird. I haven't noticed until now because 3.3.8 is unaffected.
Scala has a number of unresolved issues about "unreachable case" false positives, I'm not sure which one of them (if any) applies. I'll try to come up with a zero-dependency Scala reproduction later, but I'm out of tokens for now to polish it.
I do have a self-contained reproduction in Tuplez though. If you check out the "Add regression test for scala 3 bug" commit, it's just 0.5.0-M3. Run the test with
sbt tuplezScala3Bug/compile, and you'll see it throw warnings on perfectly valid code:In the next commit, I have replaced Tuplez Scala 3 implementation with its Scala 2 implementation. If you run
sbt tuplezScala3Bug/compileon that, the test will pass.The setup of the test as a separate module is weird – that's because Claude determined that we need a compilation boundary to trigger the bug. I'll see if I can bypass that requirement later, but in practice, users of tuplez would be affected either way.
The downside of using Scala 2 implementation is that it's quite a bit more verbose – 4K lines. Claude proposed an alternative Scala 3 implementation that was approx 300 lines – using
*:instead ofTuple.Concat, but even though it passed existing tests, I think it wasn't quite finished and required additional testing for edge cases. I haven't submitted that, I don't know if you have time to review all that. The Scala 2 implementation is at least proven.I'm getting ready to release the next and probably final pre-release version of Laminar v18, and ran into this blocker.
I have verified that this branch fixed the compilation warnings on my codebase. I had 4 warnings in our pretty large codebase. The pattern was as in the test – a tuplez-using Airstream operator like combineWith followed by map or collect with a
case ....argument.If you want to test this yourself, remember that URL-DSL also depends on Tuplez. I checked and its master compiles fine with this branch, although I assume it's not binary compatible, so would need a publishLocal if you use it.
My suggested plan of action is to release this version of Tuplez, and later file a bug with Scala, and bring back your original Scala 3 implementation when the Scala bug is eventually fixed – I assume that'll take a while seeing that they've just released 3.9.0.
Also, please don't bump Tuplez Scala 3 version to 3.9.0 just yet, I haven't confirmed that all Laminar stuff supports it yet.
The "Add sbt-buildkit-dynver plugin" commit is not related to any of this. I just used it to automatically get reasonable dev version (0.5.0-M4-SNAPSHOT) when publishLocal-ing. Feel free to discard that commit or I can remove it if you'd rather not introduce a dependency for it. I'm adding it to all my projects as I got tired of copy-pasting PlayFramework's VersionHelper.scala file into every repo.