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 match type reduction with avoided types #18043

Merged
merged 7 commits into from
Jul 3, 2023
Merged

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Jun 23, 2023

No description provided.

The "infoStr" I felt caused more noise and confusion, so I remove it.
I also pushed the type mismatch message done 1 line, so it displays
better multi-line (the "Found" and "Required" are alined).
dwijnand and others added 2 commits June 23, 2023 15:45
TypeComparer's compareAtoms assumes it has an answer to subtyping if one
of the types has atoms and the other doesn't.  But an unreduced match
alias won't have atoms, so we shouldn't bail early.

Co-Authored-By: Matt Bovel <matthieu@bovel.net>
If the skolem-widened scrutinees are the same and the cases correspond,
then the match types are the same.

Also, with inlining the type embeded in inlining's Typed expression will
be without going through type-avoidance, so they won't be skolems.  So
we look to widen inline proxy term refs, in addition to skolems.

Co-Authored-By: Matt Bovel <matthieu@bovel.net>
@mbovel
Copy link
Member

mbovel commented Jun 23, 2023

I was initially worried about widening term refs and then realized that your are only doing it for inline proxies, so that is probably fine.

We could add a test to make sure that term refs in scrutinees are not widened in general:

val x: Int = 42
val y: Int = 43
val z: Int = 44

type IsX[T] =
    T match
        case x.type => true
        case _ => false
def test = summon[IsX[y.type] =:= IsX[z.type]] // error

def test2 = summon[
  (
    y.type match
      case x.type => true
      case _ => false
  ) =:= (
    z.type match
          case x.type => true
          case _ => false
  )
] // error

Co-Authored-By: Matt Bovel <matthieu@bovel.net>
@mbovel
Copy link
Member

mbovel commented Jun 23, 2023

I unfortunately found at least one case where widening skolems is not desirable:

import scala.util.Random
val x = 42

type IsX[T] =
    T match
        case x.type => true
        case _ => false

def bothXOrNot(a: Int, b: Int)(using IsX[a.type] =:= IsX[b.type]) = ???

def test = bothXOrNot(Random.nextInt(), Random.nextInt())

It compiles with your change, while it didn't compile before. Before, it would it would not find an evidence for IsX[(?1 : Int)] =:= IsX[(?2 : Int)], now it can.

From the Match Type paper, typing rule S-Match5 requires that the LHS
scrutinee "Ss" is a subtype of the RHS scrutinee "Ts": Ss <: Ts.  It's
not required that they are the same type.  This covers widening a skolem
or inline proxy tp1 scrutinee, without erroneously widening the RHS tp2
scrutinee.
@dwijnand
Copy link
Member Author

Forgot to put in the commit message: I think this code might come from before match types were made invariant.

@dwijnand dwijnand requested a review from mbovel June 28, 2023 09:11
@dwijnand dwijnand removed their assignment Jun 28, 2023
@dwijnand dwijnand marked this pull request as ready for review June 28, 2023 09:12
Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

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

As far as I am concerned, this looks great!

As discussed: allowing match types to be generally "covariant in their scrutinees" would break algorithmic subtyping transitivity, so we probably want to avoid it for now.

However, this PR only adresses a narrower case: skolems and inline proxies. As these cannot be written by users directly in match types patterns, it seems to me that it should pretty safe. I tried to come up with problematic examples but couldn't.

The bug in compareAtoms is also an important one and we should definitely merge its fix as soon as possible.

@dwijnand dwijnand merged commit cdc9fe8 into scala:main Jul 3, 2023
17 checks passed
@dwijnand dwijnand deleted the mt/skolem branch July 3, 2023 15:02
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
@Kordyjan
Copy link
Contributor

It fails under Fixes regression introduced before Scala 3.3 -> Can change types in correct Scala 3 code path in our backporting procedure. So, I'm marking it as Backport Rejected with the possibility of changing the decision based on user requests.

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.

Regression in gekomad/itto-csv Regression on Tuple.Union when using Tuple.toList
3 participants