-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix GADT casting when typing if expressions #15646
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
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
042b7e3
insert GADT casts for if branches
Linyxus a6a943f
add GADT approximation of the stable type in GADT casting
Linyxus 95193e7
refactor GADT cast insertion into a separate method
Linyxus 3d52d1e
add test case for #14776
Linyxus 2255fdb
add documentation and point to #15646
Linyxus 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
trait Expr[T] | ||
case class IntExpr() extends Expr[Int] | ||
|
||
def flag: Boolean = ??? | ||
|
||
def foo[T](ev: Expr[T]): Int | T = ev match | ||
case IntExpr() => | ||
if flag then | ||
val i: T = ??? | ||
i | ||
else | ||
(??? : Int) |
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,13 @@ | ||
enum SUB[-A, +B]: | ||
case Refl[S]() extends SUB[S, S] | ||
|
||
trait R { | ||
type Data | ||
} | ||
trait L extends R | ||
|
||
def f(x: L): x.Data = ??? | ||
|
||
def g[T <: R](x: T, ev: T SUB L): x.Data = ev match | ||
case SUB.Refl() => | ||
f(x) |
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,15 @@ | ||
trait T1 | ||
trait T2 extends T1 | ||
|
||
trait Expr[T] { val data: T = ??? } | ||
case class Tag2() extends Expr[T2] | ||
|
||
def flag: Boolean = ??? | ||
|
||
def foo[T](e: Expr[T]): T1 = e match { | ||
case Tag2() => | ||
flag match | ||
case true => new T2 {} | ||
case false => e.data | ||
} | ||
|
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,16 @@ | ||
trait T1 | ||
trait T2 extends T1 | ||
|
||
trait Expr[T] { val data: T = ??? } | ||
case class Tag2() extends Expr[T2] | ||
|
||
def flag: Boolean = ??? | ||
|
||
def foo[T](e: Expr[T]): T1 = e match { | ||
case Tag2() => | ||
if flag then | ||
new T2 {} | ||
else | ||
e.data | ||
} | ||
|
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 was considering whether we should push this logic into
assignType(If..
. But we're already rolling our own here, by assigning the type after calling the tree copier.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.
Yes, it seems that the original code is the only callsite of
assignType(If...
(not fully verified since metals often lies to me ;)). It would be good to push the logic intoassignType
.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.
But it seems that pushing this logic into
TypeAssigner
will require us to also refactor the methods related to GADT casts (gadtAdaptBranch
andTyper.insertGadtCast
) intoTypeAssigner
. I am not sure whether it is good to put them there.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.
There's also the calls from
tpd
. The top-level defsIf
andInlineIf
, and theIf
in the tree copier.In terms of pushing logic, I was quite surprised to find some not-plain-basic code in TypeAssigner in
assignType(Apply..)
which is called by innocent-lookingtpd.Apply(..)
(code to do with substituting type parameters). So now I'm not sure if it's better to continue to hide away your logic here for whenever an If is constructed or copied, or whether it's best to break the convention and not move it... 🤔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.
@abgruszecki do you hold an opinion on this?
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 will quickly mention it during the lab meeting.Edit: just realized that I mistaken the time of the lab meeting. I thought Alex was saying that I could raise the topic during the capture calculus meeting (but it is actually on Friday). I guess Alex is actually talking to @dwijnand? :P
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.
Yep yep, I am talking about the Thursday meeting @ 15:00.
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.
You should come to the meeting on Thursday and raise it! 😄 (Or I can, np)
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'd be glad to join this lab meeting but Thursday 15:00 this week does not work well for me. :( I will appreciate knowing the results of your discussion on this topic! :D
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.
Martin raised a good point: seeing as this involves GADT information, that doesn't survive post typing, there's little sense to put it in a more general place like TypeAssigners: leaving it in Typer makes sense.