Skip to content

Make extractor patterns null safe #6485

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.

Sign up for GitHub

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
Merged
adriaanm merged 1 commit into from
Jun 12, 2018
Merged

Make extractor patterns null safe #6485

adriaanm 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
@eed3si9n eed3si9n force-pushed the wip/null-in-extractor2 branch from 2dc5ec1 to b6a007d Compare April 1, 2018 17:58
@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

@eed3si9n eed3si9n Apr 7, 2018

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 eed3si9n force-pushed the wip/null-in-extractor2 branch from cee9f3a to f08f7b5 Compare April 12, 2018 02:57
@eed3si9n
Copy link
Member Author

eed3si9n commented Apr 12, 2018

Rebased and squashed.

@adriaanm adriaanm modified the milestones: 2.13.0-M4, 2.13.0-M5 Apr 30, 2018
@eed3si9n eed3si9n force-pushed the wip/null-in-extractor2 branch from f08f7b5 to df7fc06 Compare May 4, 2018 16:25
@adriaanm adriaanm force-pushed the wip/null-in-extractor2 branch from df7fc06 to 1f0c8a7 Compare June 1, 2018 12:02
@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 force-pushed the wip/null-in-extractor2 branch from 1f0c8a7 to a60eaeb Compare June 2, 2018 06:19
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

@eed3si9n eed3si9n Jun 2, 2018

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

eed3si9n commented Jun 10, 2018

Ping

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

adriaanm commented Jun 12, 2018

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
jaydoane pushed a commit to cloudant-labs/clouseau that referenced this pull request Feb 14, 2025
@iilyak @iilyak-ibm
Fix null encoding
002421b 
It turned out that scala extractor pattern doesn't handle null correctly.
See scala/scala#6485. As a result the handling of null
has to move from extractor into the main match block. The problem was not noticed
earlier because the test for null encoding was missed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@adriaanm adriaanm adriaanm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adriaanm adriaanm

Labels

release-notes worth highlighting in next release notes

Projects

None yet

Milestone

2.13.0-M5

Development

Successfully merging this pull request may close these issues.

6 participants

@eed3si9n @adriaanm @smarter @retronym @SethTisue @scala-jenkins