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

Allow name-based extractors to be irrefutable #9343

Merged
merged 4 commits into from Feb 17, 2021

Conversation

martijnhoekstra
Copy link
Contributor

@martijnhoekstra martijnhoekstra commented Nov 24, 2020

Extractors (SLS 2.13 8.1.9) are objects with an unapply and/or unapplySeq methods that can be used as patterns in match statements.

Name-based extractors are extractors where the unapply method doesn't return Option but some other type, with a contract that it has (1) a method get returning the extracted thing and (2) a method isEmpty returning false if the extractor matches and true if it doesn't.

Prior to this change, having the static return type Some instead of Option for unapply indicated that the extractor is "irrefutable", meaning the compiler knows the pattern always matches, which it uses for reachability and exhaustiveness analysis. This PR adds two more forms of irrefutable extractors:

  1. "Boolean Match" extractors, where unapply returns the singleton type true instead of Boolean
  2. Name-based extractors, where isEmpty returns the singleton type false instead of Boolean

With these changes, using such extractors removes what would otherwise be spurious false positive exhaustivity warnings.


PR implementation notes:

Irrefutable name-based extractors on isEmpty: false

  • Check/update spec wontfix in this PR. Neither name-based extractors nor irrefutable extractors are specced.
  • Align with dotty
  • more tests, both neg and pos
  • check if this removes the necessity for List special casing
  • add tests from Vector.unapplySeq is refutable bug#12240, which is the motivating bug

Dotty: https://github.com/lampepfl/dotty/blob/cda0a9d496cc8fd6c10cb4c90c0a521a2c6820df/compiler/src/dotty/tools/dotc/transform/patmat/Space.scala#L292-L320 : constant false isEmpty is irrefutable

List special casing: Will still be needed as is, probably won't be needed anymore if we can check if all arities of unapplySeq have cases.

spun out the added testcases in 12240 to their own bug in scala/bug#12252

Fixes scala/bug#12240
Fixes scala/bug#12254

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Nov 24, 2020
@martijnhoekstra martijnhoekstra force-pushed the namebasedirrefutable branch 2 times, most recently from cc27bfc to 6ac807b Compare November 24, 2020 14:17
@dwijnand

This comment has been minimized.

@martijnhoekstra
Copy link
Contributor Author

Supporting exhaustiveness/unused checking over multiple arities of unapplySeq extractors is quite a bit more complicated: if element arity exhaustiveness is checked at all yet, I would appreciate a pointer to it. Otherwise, the ambition level of covering understanding that case Vector(), case Vector(_) and case Vector(_, _, _*) together cover all of it is probably too high.

@martijnhoekstra martijnhoekstra force-pushed the namebasedirrefutable branch 11 times, most recently from add8a01 to d1f40cd Compare November 28, 2020 12:26
@martijnhoekstra martijnhoekstra marked this pull request as ready for review November 28, 2020 12:27
@dwijnand dwijnand changed the title irrefutable name-based extractors Irrefutable name-based extractors Nov 30, 2020
@dwijnand dwijnand changed the title Irrefutable name-based extractors Allow name-based extractors to be irrefutable Nov 30, 2020
@martijnhoekstra
Copy link
Contributor Author

martijnhoekstra commented Dec 1, 2020

now also fixes scala/bug#12254:

allow true as an extractor type, and make it irrefutable

@dwijnand

This comment has been minimized.

@martijnhoekstra
Copy link
Contributor Author

Oh, I didn't see that the : true change broke a neg test output -- throw new Exception now infers as a valid extractor type as subtype of Boolean

I -update-check'ed that, and for pushed the fixup.

Happy for review in either state. Whether the error message should complain about an extractor of Nothing not having a get method as a name-based extractor or not supporting 2 extracted members as a Boolean extractor probably doesn't really matter in the end.

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Dec 1, 2020
@martijnhoekstra

This comment has been minimized.

@martijnhoekstra

This comment has been minimized.

@martijnhoekstra
Copy link
Contributor Author

what's so weird is that obviously the isEmpty of UnapplySeqWrapper was constant folded, but I wasn't able to construct any other source where isEmpty was getting constant folded.

@dwijnand
Copy link
Member

See my run/patmat-no-inline-isEmpty.scala test: it only occurs under separate compilation, that's why I compile twice. I didn't get into why that was true or if it should be true.

@martijnhoekstra
Copy link
Contributor Author

Ah, I see! That's the darndest thing -- and scary too. Maybe it doesn't just apply to this situation.

@dwijnand
Copy link
Member

dwijnand commented Feb 16, 2021

Lukas wondered if the unapply could be constant folded too, so I added a similar test for that and it's not constant folded - so all is good.

@dwijnand dwijnand force-pushed the namebasedirrefutable branch 3 times, most recently from 24a14d6 to af59cc3 Compare February 16, 2021 21:25
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Just some minor suggestions.

@dwijnand dwijnand merged commit c475777 into scala:2.13.x Feb 17, 2021
@SethTisue
Copy link
Member

🎉 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
5 participants