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

Freeze GADTs more when comparing type member infos #15869

Closed
wants to merge 1 commit into from

Conversation

dwijnand
Copy link
Member

No description provided.

@dwijnand dwijnand requested a review from Linyxus August 16, 2022 16:57
@dwijnand dwijnand changed the title Always freeze GADTs when comparing type member infos Freeze GADTs when comparing type member non-alias infos Aug 28, 2022
@dwijnand dwijnand marked this pull request as ready for review August 28, 2022 12:03
@Linyxus
Copy link
Contributor

Linyxus commented Aug 30, 2022

Thanks for the fix!

Although this does fix the issues, I think that freezing the GADT constraints when RHS is not an alias cannot cover all unsoundness problems here--since the root cause of the unsoundness is that tp1.member here could upcast LHS implicitly. For example, we could make a slightly different example:

enum SUB[-A, +B]:
  case Refl[C]() extends SUB[C, C]

trait Foo { type A }

def foo[L, H, X <: Foo { type A >: L <: H }](e: SUB[X, Foo { type A = Int }], i: Int): L = e.match {
  case SUB.Refl() => i
}

val e0: SUB[Foo { type A = Int }, Foo { type A = Int }] =
  SUB.Refl[Foo { type A = Int }]()

def boom(i: Int): String = foo[Nothing, Any, Foo { type A = Int }](e0, i) : Nothing   // cast Int to String!

Comparing X <:< Foo { type A = Int } upcasts the LHS implicitly and results in unsound bounds, although the type member on RHS is a type alias. The safest way may be the one originally implemented in this PR--freezing the type member whenever comparing type member infos, but this seems overly restrictive and breaks existing code. So I think maybe we should merge this fix now before we find out a proper way to check whether tp1.member upcasts tp1 implicitly.

@Linyxus
Copy link
Contributor

Linyxus commented Aug 30, 2022

Btw, I think we should keep the tp1IsSingleton check, since this at least prevents the unsound cases brought by upcasting singleton types. The following code, which is modified from the code in #14728, becomes a false positive example in this PR:

enum SUB[-A, +B]:
  case Refl[C]() extends SUB[C, C]

trait Foo { type A }

def foo[L, H](x: Foo { type A >: L <: H }, e: SUB[x.type, Foo { type A = Int }], i: Int): L = e.match {
  case SUB.Refl() => i
}

val x0 = new Foo { type A = Int }
val e0: SUB[x0.type, Foo { type A = Int }] = SUB.Refl[x0.type]()

def boom(i: Int): String = foo[Nothing, Any](x0, e0, i) : Nothing   // cast Int to String!

@dwijnand dwijnand assigned dwijnand and unassigned abgruszecki Aug 31, 2022
@dwijnand dwijnand marked this pull request as draft August 31, 2022 08:22
@dwijnand dwijnand force-pushed the ptc-refined branch 5 times, most recently from 671a48c to 38e6e9e Compare September 22, 2022 13:35
@dwijnand dwijnand changed the title Freeze GADTs when comparing type member non-alias infos Freeze GADTs more when comparing type member infos Sep 22, 2022
@dwijnand dwijnand force-pushed the ptc-refined branch 4 times, most recently from 9084be9 to ee3c1e9 Compare September 26, 2022 13:04
@dwijnand dwijnand marked this pull request as ready for review September 26, 2022 20:18
def tp1IsSingleton: Boolean = tp1.isInstanceOf[SingletonType]
def allowGadt = mbr match
case _ if tp1.isInstanceOf[SingletonType] => false
case d: UniqueRefDenotation if d.prefix == NoPrefix && d.symbol != NoSymbol => false
Copy link
Member Author

Choose a reason for hiding this comment

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

@odersky this is what I found works. Can you think of another way to identify when we have a member that required widening tp1 to a non-class info and thus we can't safely infer gadt constraints?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't make sense of this condition and don't understand the logic behind this. This would at least need a fat comment explaining the issue and the attempted solution.

@dwijnand dwijnand assigned odersky and unassigned dwijnand Sep 26, 2022
@odersky
Copy link
Contributor

odersky commented Sep 29, 2022

What is the motivation for these changes?

@dwijnand
Copy link
Member Author

#15485 is the issue it fixes (in the sidebar)

def tp1IsSingleton: Boolean = tp1.isInstanceOf[SingletonType]
def allowGadt = mbr match
case _ if tp1.isInstanceOf[SingletonType] => false
case d: UniqueRefDenotation if d.prefix == NoPrefix && d.symbol != NoSymbol => false
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't make sense of this condition and don't understand the logic behind this. This would at least need a fat comment explaining the issue and the attempted solution.

@odersky odersky assigned dwijnand and unassigned odersky Oct 10, 2022
@dwijnand dwijnand closed this Oct 10, 2022
@dwijnand dwijnand deleted the ptc-refined branch October 10, 2022 15:45
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.

Comparing Refined Types can lead to invalid subtype reconstruction
4 participants