Skip to content
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

Merged
merged 12 commits into from
May 17, 2022

Conversation

griggt
Copy link
Collaborator

@griggt griggt commented Jan 19, 2022

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

@griggt griggt added the needs-minor-release This PR cannot be merged until the next minor release label Jan 19, 2022
@dwijnand

This comment was marked as resolved.

@griggt griggt force-pushed the stricter-pattern-bindings-3.2 branch from ccb3db1 to 121abc5 Compare January 19, 2022 15:47
@griggt

This comment was marked as resolved.

@dwijnand

This comment was marked as outdated.

@sjrd

This comment was marked as outdated.

@dwijnand dwijnand added this to the 3.2.0 milestone Jan 20, 2022
@dwijnand

This comment was marked as outdated.

@griggt

This comment was marked as outdated.

griggt added a commit to griggt/scala-js that referenced this pull request Feb 9, 2022
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.
griggt added a commit to griggt/docs.scala-lang that referenced this pull request Feb 9, 2022
These changes are slated to occur with the release of Scala 3.2
(see scala/scala3#14294) and did not
happen in 3.1.
griggt added a commit to griggt/scala-js that referenced this pull request Feb 9, 2022
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.
@griggt griggt force-pushed the stricter-pattern-bindings-3.2 branch 3 times, most recently from 4aa03a3 to ba0685e Compare February 16, 2022 18:34
@griggt

This comment was marked as outdated.

@sjrd

This comment was marked as outdated.

@sjrd

This comment was marked as outdated.

@griggt griggt force-pushed the stricter-pattern-bindings-3.2 branch from ba0685e to 69e3543 Compare March 3, 2022 22:19
@griggt

This comment was marked as outdated.

@griggt

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
Copy link
Member

@bishabosha bishabosha Mar 18, 2022

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:

Suggested change
else if sourceVersion.isAtLeast(`3.2`) && !sourceVersion.isMigrating then GenCheckMode.Check
else if sourceVersion.isAtLeast(`3.2`) then GenCheckMode.Check

@smarter
Copy link
Member

smarter commented Mar 21, 2022

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 -source but this seems much more cumbersome for users (they have to be aware of the flag in the first place, and need to remember to add/remove it when appropriate). This would also potentially give time for the warning to be backported to Scala 2 if someone felt like porting it to give a heads-up to Scala 2 projects that aren't cross-compiled yet.

@griggt
Copy link
Collaborator Author

griggt commented May 11, 2022

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.

@griggt griggt requested a review from bishabosha May 11, 2022 06:14
@griggt griggt assigned bishabosha and unassigned griggt May 11, 2022
Copy link
Member

@bishabosha bishabosha left a 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 to 3.0

@griggt
Copy link
Collaborator Author

griggt commented May 11, 2022

automatic patches should only run specifically for the migration versions that we have declared the "grace period" for updating code

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 -future-migration having different behavior between now and, well, the future, on say a codebase moving from 3.0/3.1.

@bishabosha
Copy link
Member

automatic patches should only run specifically for the migration versions that we have declared the "grace period" for updating code

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 -future-migration having different behavior between now and, well, the future, on say a codebase moving from 3.0/3.1.

this is an interesting point, I was saying this because for example patches for 3.0 are only applied for 3.0-migration, i.e. not for future-migration - i.e. once you have gone beyond that version it is no longer valid source so there is nothing to patch. This is also a minor release so source compatibility could be broken. @smarter what do you think?

@smarter
Copy link
Member

smarter commented May 11, 2022

Honestly I'm not sure, but whatever we do should be documented in https://dotty.epfl.ch/docs/reference/language-versions/source-compatibility.html

@bishabosha
Copy link
Member

bishabosha commented May 16, 2022

I had a discussion with @sjrd , I think it is safe to say that future is not meant to adhere to the same compatibility guarantees of a usual language version.

@bishabosha bishabosha force-pushed the stricter-pattern-bindings-3.2 branch from 6b37935 to dc5d1d7 Compare May 16, 2022 11:57
@bishabosha bishabosha self-requested a review May 16, 2022 11:58
@bishabosha
Copy link
Member

need someone else to review my last commit

@bishabosha bishabosha assigned sjrd and unassigned bishabosha May 16, 2022
@bishabosha bishabosha requested a review from sjrd May 16, 2022 12:02
@bishabosha bishabosha requested a review from sjrd May 17, 2022 12:05
Copy link
Member

@sjrd sjrd left a 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.

@bishabosha
Copy link
Member

community build c is failing due to #15205

@griggt griggt merged commit e5abec0 into scala:main May 17, 2022
@griggt griggt deleted the stricter-pattern-bindings-3.2 branch May 17, 2022 23:00
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

introduce language imports for scala 3.2 Enable stricter pattern matching rules for 3.2
5 participants