-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable stricter pattern binding warnings by default #14294
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
ccb3db1
to
121abc5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Beginning in Scala 3.2, refutable pattern bindings will warn by default, see https://dotty.epfl.ch/docs/reference/changed-features/pattern-bindings.html Since the scalajs-ir code is unpacked and recompiled from source as part of the Scala 3 build, which uses -Xfatal-warnings, we suppress the warning here with an @unchecked annotation to unblock the dotty PR (scala/scala3#14294) introducing this behavioral change. This commit does not address such usage in any other portion of the codebase, which may need to be addressed at a future date.
These changes are slated to occur with the release of Scala 3.2 (see scala/scala3#14294) and did not happen in 3.1.
Beginning in Scala 3.2, refutable pattern bindings will warn by default, see https://dotty.epfl.ch/docs/reference/changed-features/pattern-bindings.html Since the scalajs-ir code is unpacked and recompiled from source as part of the Scala 3 build, which uses -Xfatal-warnings, we suppress the warning here with an @unchecked annotation to unblock the dotty PR (scala/scala3#14294) introducing this behavioral change. This commit does not address such usage in any other portion of the codebase, which may need to be addressed at a future date.
4aa03a3
to
ba0685e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
ba0685e
to
69e3543
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
else if sourceVersion.isAtLeast(future) then GenCheckMode.Check | ||
else GenCheckMode.FilterNow // filter for now, to keep backwards compat | ||
if casePat then GenCheckMode.FilterAlways | ||
else if sourceVersion.isAtLeast(`3.2`) && !sourceVersion.isMigrating then GenCheckMode.Check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at the old code future-migration
would use the new behaviour, so I don't think we need the "not migrating" check - besides, the current change would return false for future-migration
, so I suggest we remove the condition:
else if sourceVersion.isAtLeast(`3.2`) && !sourceVersion.isMigrating then GenCheckMode.Check | |
else if sourceVersion.isAtLeast(`3.2`) then GenCheckMode.Check |
If I understand this change correctly, it means that (when -source is not used) a piece of code that previously compiled without warnings can now fail with an error. To ease migration, could we instead emit a warning by default in 3.2 and only turn it into an error in 3.3? I imagine that the alternative is to recommend using |
This has been rebased and review comments addressed. I'm happy to rebase again just prior to merge in case another PR happens to introduce a refutable pattern binding into the codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple of points
- automatic patches should only run specifically for the migration versions that we have declared the "grace period" for updating code
- Feature.warnOnMigration should no longer consider
3.2
as equivalent to3.0
Is this just a policy decision or are there technical reasons behind it? I had left the migration patches open ended as I wanted to avoid |
this is an interesting point, I was saying this because for example patches for |
Honestly I'm not sure, but whatever we do should be documented in https://dotty.epfl.ch/docs/reference/language-versions/source-compatibility.html |
I had a discussion with @sjrd , I think it is safe to say that |
6b37935
to
dc5d1d7
Compare
need someone else to review my last commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last two commits LGTM.
community build c is failing due to #15205 |
The first step of a phased-in approach to tighten the rules on pattern bindings, as described at https://docs.scala-lang.org/scala3/reference/changed-features/pattern-bindings.html
In 3.2: (this PR), emit warnings by default for refutable pattern bindings. Keep the existing desugaring of for generators.
In 3.3 (or later), the above warnings become errors, and
case
is required on for generators to get filtering behavior.Rewrites are available (under
-source 3.2-migration -rewrite
) to ease migration.Closes #13832
Closes #14615