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

Spec for name based and irrefutable patterns #9351

Merged
merged 1 commit into from Feb 18, 2021

Conversation

@martijnhoekstra
Copy link
Contributor

@martijnhoekstra martijnhoekstra commented Nov 30, 2020

Name based patterns were unspecced, and irrefutable patterns were underspecced.

This brings the spec beyond the current implementation, to where, I think, it should be.

Some-based irrefutable extractors and name-based extractors were part of scalac but unspecced, and are now part of the spec.

Name-based irrefutable extractors are pending per #9343

true-based irrefutable extractors may be broken/may have never worked per this comment: scala/bug#12232 (comment) and should probably be looked in to further.

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Nov 30, 2020
@martijnhoekstra martijnhoekstra marked this pull request as draft Dec 1, 2020
@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Dec 1, 2020

Name-based extractors for multiple parameters is not yet right.

@liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Dec 1, 2020

FYI, Dotty also has some documentation about this: http://dotty.epfl.ch/docs/reference/changed-features/pattern-matching.html

@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Dec 1, 2020

Thanks, I'll take a look @liufengyun

What I did wrong here is the n > 1 case for unapply with _1 to _n rather than a tuple.

@liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Dec 1, 2020

Thanks, I'll take a look @liufengyun

What I did wrong here is the n > 1 case for unapply with _1 to _n rather than a tuple.

In Scala 3, it currently only checks ._1 and ._n for name-based matches. However, it checks T <: Product for product-based matches. If necessary, I guess Scala 3 can also adapt so that Scala 2 and 3 agree on the same protocol.

@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Dec 1, 2020

@liufengyun Requiring T <: Product for extractors in scala 2 will likely break code in the wild in scala 2 and sounds to me like a non-starter as it will probably break code in the wild. As for what should happen in scala 3, dropping the product requirement would probably lessen the amount of migration friction.

The scala 2 compiler team at Lightbend and the dotty teams may want to discuss whether and how that should be aligned -- I'm just a random community passer-by trying to align the spec with the implementation and scala 3. When it's decision making time, it's up to the Powers that Be.

@liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Dec 1, 2020

@liufengyun Requiring T <: Product for extractors in scala 2 will likely break code in the wild in scala 2 and sounds to me like a non-starter as it will probably break code in the wild. As for what should happen in scala 3, dropping the product requirement would probably lessen the amount of migration friction.

I'm not sure if we are talking about the same thing. By product-based matches, I mean this feature, which seems not supported by Scala 2. This might be a relief for the concern about migration.

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 1, 2020

Doesn't Name-based Match (which is supported already) subsume Product Match?

@liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Dec 1, 2020

Doesn't Name-based Match (which is supported already) subsume Product Match?

They are different, Product Match is always irrefutable and does not go through the following interface used in name-based match:

{
  def isEmpty: Boolean
  def get: S
}

In product match, the return type of unapply is a product.

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 1, 2020

Ah, I see - thank you. Seems a little redundant, language-wise, but perhaps it's very practical and convenient. Do you know what the motivation for it was? (Sorry for the tangential discussion but I've been wanting to know what Scala 3 changed/added here.)

@liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Dec 1, 2020

Ah, I see - thank you. Seems a little redundant, language-wise, but perhaps it's very practical and convenient. Do you know what the motivation for it was? (Sorry for the tangential discussion but I've been wanting to know what Scala 3 changed/added here.)

It's used in case classes, the unapply now can simply return the object, without boxing it in an Option and unboxing.

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Dec 1, 2020

Ah right, yeah. It's good that the pattern matcher would, therefore, use the real, generated unapply, instead of how it accesses things directly on it in Scalac 2. 👍 thank you

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 4, 2020

should this still be marked as "draft"?

@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Dec 4, 2020

@SethTisue yes, I still want to double-check some of it vs the implementation and scala3's implementation

@martijnhoekstra martijnhoekstra force-pushed the specNameBasedExtractors branch 2 times, most recently from f754321 to caf5eb1 Dec 7, 2020
@martijnhoekstra martijnhoekstra marked this pull request as ready for review Dec 7, 2020
@martijnhoekstra martijnhoekstra force-pushed the specNameBasedExtractors branch from caf5eb1 to b72ff83 Dec 7, 2020
@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 8, 2020

remaining steps here, afaict:

@dwijnand dwijnand marked this pull request as draft Jan 26, 2021
@dwijnand
Copy link
Member

@dwijnand dwijnand commented Jan 26, 2021

Draft because of the PR dependency. Also needs a rebase it seems.

@SethTisue SethTisue removed this from the 2.13.5 milestone Feb 9, 2021
@SethTisue SethTisue added this to the 2.13.6 milestone Feb 9, 2021
@dwijnand dwijnand removed this from the 2.13.6 milestone Feb 17, 2021
@dwijnand dwijnand added this to the 2.13.5 milestone Feb 17, 2021
@dwijnand dwijnand marked this pull request as ready for review Feb 17, 2021
@dwijnand dwijnand force-pushed the specNameBasedExtractors branch from b72ff83 to 36fa494 Feb 17, 2021
@dwijnand
Copy link
Member

@dwijnand dwijnand commented Feb 17, 2021

A fairly painful rebase... Only partially reviewed it, focussed more on just completing the rebase so we can review it together.

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Feb 17, 2021

... so I might've made some mistakes rebasing, feel free to amend and push.

@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Feb 18, 2021

Source wise nothing stands out as mis-rebased, but I haven't looked at the generated pdf and html yet.

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Feb 18, 2021

OK, I plan on reviewing the change overall and getting it merged by Monday, Seth-time.

@martijnhoekstra
Copy link
Contributor Author

@martijnhoekstra martijnhoekstra commented Feb 18, 2021

I know Seth travels a lot, but I didn't realize they'd given him his own timezone. @liufengyun might still have something to comment about whether this is right from the dotty side of things too.

Copy link
Contributor

@liufengyun liufengyun left a comment

LGTM 👍

spec/08-pattern-matching.md Outdated Show resolved Hide resolved
@dwijnand dwijnand force-pushed the specNameBasedExtractors branch from f4c7e33 to 28765ed Feb 18, 2021
@dwijnand dwijnand requested a review from SethTisue Feb 18, 2021
Copy link
Member

@dwijnand dwijnand left a comment

Fixed a rebase error in duplicating the n>1 section. LGTM, asking Seth if there are any spec-ese mistakes or any other improvements.

@SethTisue SethTisue merged commit 2aac322 into scala:2.13.x Feb 18, 2021
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants