-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix GADT approximation #9322
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
Merged
Merged
Fix GADT approximation #9322
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c7aea0e
Fix ApproximateGadtAccumulator
abgruszecki baaaa6c
Make i7044 typecheck with old version of recover
abgruszecki 72916ea
Clean up adaptToSubType
abgruszecki 0896981
Adjust based on review feedback
abgruszecki c434949
Remove unnecessary tree attachment
abgruszecki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
object MemberHealing { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
t + 2 | ||
} | ||
} | ||
|
||
object ImplicitLookup { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
class Tag[T] | ||
|
||
implicit val ti: Tag[Int] = Tag() | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
implicitly[Tag[Int]] | ||
} | ||
} | ||
|
||
object GivenLookup { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
class Tag[T] | ||
|
||
given ti as Tag[Int] | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
summon[Tag[Int]] | ||
} | ||
} | ||
|
||
object ImplicitConversion { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
class Pow(self: Int): | ||
def **(other: Int): Int = math.pow(self, other).toInt | ||
|
||
implicit def pow(i: Int): Pow = Pow(i) | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
t ** 2 // error // implementation limitation | ||
} | ||
|
||
def bar[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
(t: Int) ** 2 // sanity check | ||
} | ||
} | ||
|
||
object GivenConversion { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
class Pow(self: Int): | ||
def **(other: Int): Int = math.pow(self, other).toInt | ||
|
||
given as Conversion[Int, Pow] = (i: Int) => Pow(i) | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
t ** 2 // error (implementation limitation) | ||
} | ||
|
||
def bar[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
(t: Int) ** 2 // sanity check | ||
} | ||
} | ||
|
||
object ExtensionMethod { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
extension (x: Int): | ||
def **(y: Int) = math.pow(x, y).toInt | ||
|
||
def foo[T](t: T, ev: T SUB Int) = | ||
ev match { case SUB.Refl() => | ||
t ** 2 | ||
} | ||
} | ||
|
||
object HKFun { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
enum HKSUB[-F[_], +G[_]]: | ||
case Refl[H[_]]() extends HKSUB[H, H] | ||
|
||
def foo[F[_], T](ft: F[T], hkev: F HKSUB Option, ev: T SUB Int) = | ||
hkev match { case HKSUB.Refl() => | ||
ev match { case SUB.Refl() => | ||
// both should typecheck - we should respect invariance of F | ||
// (and not approximate its argument) | ||
// but also T <: Int b/c of ev | ||
val x: T = ft.get | ||
val y: Int = ft.get | ||
} | ||
} | ||
|
||
enum COVHKSUB[-F[+_], +G[+_]]: | ||
case Refl[H[_]]() extends COVHKSUB[H, H] | ||
|
||
def bar[F[+_], T](ft: F[T], hkev: F COVHKSUB Option, ev: T SUB Int) = | ||
hkev match { case COVHKSUB.Refl() => | ||
ev match { case SUB.Refl() => | ||
// Sanity check for `foo` | ||
// this is an error only because we blindly approximate covariant type arguments | ||
// if it stops being an error, `foo` should be re-thought | ||
val x: T = ft.get // error | ||
val y: Int = ft.get | ||
} | ||
} | ||
} | ||
|
||
object NestedConstrained { | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
def foo[A, B](a: A, ev1: A SUB Option[B], ev2: B SUB Int) = | ||
ev1 match { case SUB.Refl() => | ||
ev2 match { case SUB.Refl() => | ||
1 + "a" | ||
a.get : Int | ||
} | ||
} | ||
} |
File renamed without changes.
File renamed without changes.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still concerned about the stuff I mentioned in #9274 (comment), it seems that if I have a method
def foo[T <: X] = ...
, then I immediately get a GADT constraintT <: X
, and based on this documentation I would guess this means that ApproximateGadtAccumulator will replaceOption[T]
byOption[X]
, even though there's no need to do any approximation and nothing GADT related is actually going on?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I now realize that approximateGADT is only called when the type of the tree is not a subtype of the expected type, so it's probably OK technically, but I think it's confusing and not great for performance that we run this stuff in situations which have nothing to do with GADTs. A few suggestions:
pt
is aSelectjonProto
andctx.gadt.nonEmpty
, so we should only run the type map when these conditions are true, this is better for performance and for understanding what's going on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. documentation: I'll add the doc to code in
adaptToSubType
. We need to approximate GADTs inadapt
in order to look up members that should be there b/c of GADT constraints. We cannot take GADT constraints into account when doing normal member lookup b/c of cache. Sample code is https://github.com/lampepfl/dotty/pull/9322/files#diff-70e7287c36d5e6a572c97c2b51613049R1-R9.Re. lazily performing the approximation: will do, doing it eagerly is a remainder from the old
recover
, which also used the result of approx.Re. type parameters: key issue is telling whether the type parameters are from an enclosing method, or from somewhere else (i.e. a class?) Now that I think about it, there may be some way of doing so. Question is, do we actually get any performance out of it. I'd prefer to know for sure that there's a performance issue with this code before trying to change it.
Final concern: can you take a look @ https://github.com/lampepfl/dotty/pull/9322/files#diff-1b5c8ce1eaca91d793eef217487d5a59R754 and see whether that change makes sense to you?
isNewSubType
would at most returnfalse
early before doing the type comparison, so I cannot really see any way that this change is wrong, but at the same time all the justification I have for it is that no test broke and it fixes my issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if the owner is a method, type parameters from methods which are not enclosing should not be in scope at all.
I guess that usage of isNewSubType was incorrect before? Maybe something worth asking Martin about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I was thinking about when I said "there may be some way of doing so". I'd still like an indication that we have a performance problem here.
@odersky can you take a look at the single line modified in TypeComparer in https://github.com/lampepfl/dotty/pull/9322/files#diff-1b5c8ce1eaca91d793eef217487d5a59R754 and check if the change looks OK to you? The problem that this change fixes is #9322 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's not a performance problem, I think it's nicer if GADT code doesn't run when nothing GADT related is going on: it makes it easier to know what code can be safely ignored when trying to understand or debug something, and it makes the output of debug printers easier to read (especially the subtyping printer which is hard enough to follow in the best of situations), but this isn't a blocker for this PR, and if you think it would make the resulting GADT logic too complicated then we can keep the current logic of course.