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

Jump to bottom

Make extractor patterns null safe #6485

Merged
merged 1 commit into from Jun 12, 2018
Merged

Make extractor patterns null safe #6485

merged 1 commit into from Jun 12, 2018

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Apr 1, 2018

Ref scala/bug#4364
This is a general fix for scala/bug#2241 and scala/bug#8787.

scala/bug#4364 raises the question of what should the pattern matcher do when encountering a null and an extractor pattern. As it stands, SLS does not say anything about null for extractor pattern, which leads:

  • Pattern matcher will happily pass null into your extractor.
  • I can write a null-matching extractor MyNull.
  • Thus, all extractor authors must check for null if they would like to access the passed in argument x.

There are potentially two things we can do.

  1. Be ok with the status quo. Clarify in SLS that all extractor authors must handle null.
  2. Not be ok with the status quo. Change SLS and implementation such that null is treated as no match (empty).

I've already sent option 1 as #6374.
This implements option 2.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Apr 1, 2018
This was referenced Apr 1, 2018
Closed
Closed
@adriaanm
Copy link
Contributor

adriaanm commented Apr 3, 2018

See also: https://github.com/scala/scala/pull/6485/files#diff-352de7f9c77a7e21df6940d6a1bcdc7eL115, where a type test is potentially emitted before an extractor call, which could also rule out null args.

@smarter
Copy link
Member

smarter commented Apr 6, 2018

Note that Dotty already has the behavior implemented by this PR.

eed3si9n
eed3si9n commented Apr 7, 2018
View reviewed changes
src/compiler/scala/tools/nsc/transform/patmat/MatchTranslation.scala
} else {
val nonNullTest = NonNullTestTreeMaker(typeTest.nextBinder, paramType, pos)
val unappBinder = nonNullTest.nextBinder
(typeTest :: nonNullTest :: treeMakers(unappBinder, pos), unappBinder)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adriaanm Instead of doing null check in ExtractorTreeMaker (like I had in b6a007d), I've created a new tree maker NonNullTestTreeMaker. This is positioned after typeTest. If binderKnowNonNull is true, I am skipping the test.

I've removed null checking in ProductExtractorTreeMaker and passing binderKnowNonNull around since it's now redundant.

@eed3si9n eed3si9n changed the title Stop passing null to extractor pattern Stop passing null to extractor pattern [ci: last-only] Apr 7, 2018
@eed3si9n
Copy link
Member Author

Rebased and squashed.

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
@adriaanm
Copy link
Contributor

adriaanm commented Jun 1, 2018

Rebased and slightly reworded.

@eed3si9n
Extractor pattern no longer matches null
a60eaeb 
Until now, the spec did not say anything about `null` for extractor pattern, so that:
  - The pattern matcher would happily pass `null` into your extractor.
  - One could write a `null`-matching extractor `MyNull`.
  - But all extractor authors must consider `null` as a possible argument value.

No more! The pattern matcher inserts a non-`null` check before invoking an extractor,
so that you don't have to.

This is a general fix for scala/bug#2241, scala/bug#8787. See scala/bug#4364.
eed3si9n
eed3si9n commented Jun 2, 2018
View reviewed changes
test/files/run/patmatnew.scala
List((1,2),(3,4),(5,6)) == z
}

def run(): Unit = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated test for procedure syntax deprecation.

@eed3si9n
Copy link
Member Author

Ping

@adriaanm adriaanm added the release-notes worth highlighting in next release notes label Jun 12, 2018
@adriaanm
Copy link
Contributor

LGTM -- checked diff for sbt -Dstarr.version=2.13.0-pre-{a60eaeb,f16eacc}-SNAPSHOT compile

@adriaanm adriaanm merged commit 9d67389 into scala:2.13.x Jun 12, 2018
@SethTisue SethTisue removed the release-notes worth highlighting in next release notes label Aug 22, 2018
@eed3si9n eed3si9n deleted the wip/null-in-extractor2 branch September 20, 2018 02:49
@retronym retronym added the release-notes worth highlighting in next release notes label Oct 29, 2018
@retronym retronym mentioned this pull request Nov 8, 2018
Closed
@SethTisue SethTisue mentioned this pull request Apr 8, 2019
Closed
@SethTisue SethTisue changed the title Stop passing null to extractor pattern [ci: last-only] Make extractor patterns null safe Jun 18, 2019
@bbjubjub2494 bbjubjub2494 mentioned this pull request Jun 23, 2020
Open
@smarter smarter mentioned this pull request Nov 19, 2020
Closed
@lrytz lrytz mentioned this pull request May 31, 2021
Merged
@eed3si9n eed3si9n mentioned this pull request Jul 2, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriaanm adriaanm adriaanm approved these changes

Assignees

@adriaanm adriaanm

Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.0-M5
6 participants
@eed3si9n @adriaanm @smarter @retronym @SethTisue @scala-jenkins