-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 regression in exhaustivity of HK types #18303
Conversation
30731c1
to
09df3ef
Compare
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.
Dale and I discussed this over Zoom yesterday and today. The story has grown a bit long and complicated (original ticket, original PR, regression ticket, this PR) and there are multiple overlapping concerns (which is why a partial reversion is even an option).
I'm satisfied that this PR should be merged, since (to make a long story short) 1) it fixes the regression, 2) it's only reverting code that was added only quite recently anyway. So this seems mergeable and desirable to me, even if it's not necessairily the final chapter in this story.
So I'm hitting approve, but let's summon @liufengyun. Fengyun, you approved the original PR, do you want to take a look at this one as well?
@Kordyjan note the backport-nominated label here. This partially reverts a PR that was merged for 3.3.1, so it should at least be considered for 3.3.2 IMO.
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.
LGTM
Should #16451 be re-opened?
@@ -4,5 +4,7 @@ case class Fun[A, B](f: Exp[A => B]) extends Exp[A => B] | |||
class Test { | |||
def eval(e: Fun[Int, Int]) = e match { | |||
case Fun(x: Fun[Int, Double]) => ??? // error | |||
case Fun(x: Exp[Int => String]) => ??? // error | |||
case _ => |
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.
Not sure why the 2nd case was removed before, restoring it seems to be more reasonable. Otherwise, it seems not easy to justify why the 1st case receives a warning.
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.
The warning on the first one became (and now revert away from) an unchecked warning, on the unchecked type test.
The motivating case (i16451) is complicated, because it involves unchecked type arguments. To fix the regression, I'm reverting the fix.
09df3ef
to
0839123
Compare
As issues are assigned to release milestone, I'd rather just open a new issue - which I'll do after merging. |
There is still a regression in 3.3.1-RC7 after this was backported: #18507 |
Reverts a key part of #16958, which is a complicated case, to fix the regression(s).