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

Fix #10747: Raise type error on unreducible match type #12768

Merged
merged 9 commits into from
Sep 14, 2021

Conversation

OlivierBlanvillain
Copy link
Contributor

The second commit is needed to that we don't unnecessarily try to reduce a match type right-hand side when that's not required. One example where thing go wrong otherwise is tuple's Elem type:

/** Type of the element at position N in the tuple X */
type Elem[X <: Tuple, N <: Int] = X match {
  case x *: xs =>
    N match {
      case 0 => x
      case S[n1] => Elem[xs, n1]
    }
}

Given a concrete instantiation of X, trying to reduce the right-hand side of this type results in a type error because Elem[Unit, n1] is a match error. Obviously, we could circumvent that issue by adding a case _ => Nothing, but getting a type error leads to better type safety. Furthermore, it matches the match expression "execution model" where a right-hand side is only executed if the overall expression reduces to that case.

@johnynek
Copy link

johnynek commented Jun 29, 2021

note that: #12974 might be a good example of a test to add. I hope this PR could fix that issue as well.

Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Nice work! Sorry for taking so long to review.

@odersky odersky removed their assignment Aug 30, 2021
@odersky
Copy link
Contributor

odersky commented Aug 30, 2021

Needs a rebase.

Don't consider types to be fullyInstanciated when containing a TypeParamRef.
The previous check was too conservative, is we have a proof that A and B are
disjoint, then A and B in Inv[A] and Inv[B] are clearly different types, and
Inv[A] and Inv[B] are disjoint. Also the isSameType() && fullyInstantiated
check is quite primitive, it can still be useful in some cases.
@OlivierBlanvillain
Copy link
Contributor Author

Rebased. Given that this PR changes the semantics of match types, should we still merge it directly on master or should it go to another branch?

@smarter
Copy link
Member

smarter commented Sep 8, 2021

I'd say either we should make an exception and get this in 3.1.0-RC2 or wait for 3.2

@johnynek
Copy link

johnynek commented Sep 8, 2021

I'm really happy this has been fixed. I hope it goes in sooner than later to minimize the amount of broken code that might be written before the fix is out.

Currently there is very little code in the wild using match types. The longer we wait the less true that is.

@dwijnand dwijnand added the backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" label Sep 13, 2021
@smarter
Copy link
Member

smarter commented Sep 13, 2021

Assigned to @anatoliykmetyuk for merging and backporting.

@anatoliykmetyuk anatoliykmetyuk merged commit b3ef04f into scala:master Sep 14, 2021
@anatoliykmetyuk anatoliykmetyuk deleted the fix-10747 branch September 14, 2021 06:12
@dwijnand
Copy link
Member

#13519 is the backport.

@Kordyjan Kordyjan added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Sep 16, 2021
@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants