Skip to content

Conversation

@eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Mar 5, 2018

Ref scala/bug#4364

The specification clarification that is needed is this: 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. See Animal example below.

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 sent in the change as Make extractor patterns null safe #6485.

I think Paul is suggesting option 1 in scala/bug#4364, not spec change. Given that there are no outcry of null handling besides scala/bug#2241, I think that's fine too.

MyNull

scala> object MyNull {
     |   def unapply(x: AnyRef): Option[AnyRef] =
     |     if (x eq null) Some(x) else None
     | }
defined object MyNull

scala> null match { case MyNull(_) => "yes"; case _ => "no" }
res4: String = yes

Animal

scala> object Animal {
     |   def unapply(x: AnyRef): Option[AnyRef] =
     |     if (x.toString == "Animal") Some(x)
     |     else None
     | }
defined object Animal

scala> null match { case Animal(_) => "yes"; case _ => "no" }
java.lang.NullPointerException
  at Animal$.unapply(<console>:13)
  ... 37 elided

@eed3si9n eed3si9n requested a review from adriaanm March 5, 2018 01:20
@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Mar 5, 2018
}
val z = null match {
case Pair(_, _) => "a pair"
caes _ => "not a pair"
Copy link
Contributor

Choose a reason for hiding this comment

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

caes -> case

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks.

Ref scala/bug#4364

The specification clarification that is needed is this: 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`. See `Animal` example below.

There are potentially two things we can do.
1. Be ok with the status quo. Make sure we handle `null` in all our extractors.
2. Not be ok with the status quo. Suppose we would want extractor pattern to be non-null values only, then that's a spec change and change in pattern matcher's behavior. It might also break existing code that handled `null` in ever so subtle way.

I think Paul is suggesting option 1 in scala/bug#4364, not spec change. Given that there are no outcry of `null` handling besides scala/bug#2241, I think that's fine too.

### MyNull

```scala
scala> object MyNull {
     |   def unapply(x: AnyRef): Option[AnyRef] =
     |     if (x eq null) Some(x) else None
     | }
defined object MyNull

scala> null match { case MyNull(_) => "yes"; case _ => "no" }
res4: String = yes
```

### Animal

```scala
scala> object Animal {
     |   def unapply(x: AnyRef): Option[AnyRef] =
     |     if (x.toString == "Animal") Some(x)
     |     else None
     | }
defined object Animal

scala> null match { case Animal(_) => "yes"; case _ => "no" }
java.lang.NullPointerException
  at Animal$.unapply(<console>:13)
  ... 37 elided
```
@eed3si9n eed3si9n force-pushed the wip/null-in-extractor branch from 0600275 to 3e698f4 Compare March 5, 2018 01:23
@som-snytt
Copy link
Contributor

Another sample outcry: scala/bug#8787

eed3si9n added a commit to eed3si9n/scala that referenced this pull request 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 scala#6374.
This implements option 2.
eed3si9n added a commit to eed3si9n/scala that referenced this pull request 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 scala#6374.
This implements option 2.
@adriaanm
Copy link
Contributor

Closing in favor of status co-quo.

@adriaanm adriaanm closed this Apr 10, 2018
@SethTisue SethTisue removed this from the 2.13.0-M4 milestone Apr 11, 2018
eed3si9n added a commit to eed3si9n/scala that referenced this pull request Apr 12, 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 scala#6374.
This implements option 2.
eed3si9n added a commit to eed3si9n/scala that referenced this pull request May 4, 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 scala#6374.
This implements option 2.
@eed3si9n eed3si9n deleted the wip/null-in-extractor branch January 15, 2019 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants