Conversation
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
I'm pretty sure I helped suggest what ultimately became this change, which is why I requested someone else's review on it initially. So naturally, I'm OK with it. But someone else should really review this and try to understand why this fixes the bug without breaking anything.
|
For what it's worth, our codebase (which exhibited this issue in 3.6+, and seemed to be resolved by merging this PR) no longer triggers this assertion with 3.8. |
|
Yes, it appears that something in 3.8.?? fixes this, but it still breaks in 3.7.?? and below. Do we want to patch older versions (so we should keep this fix). |
|
I'm pretty sure @ahejlsberg shipped a fix or two to union assignability in 3.8. He definitely encountered this assert along the way, so I think the improvement in 3.8 is intentional. Let's trim this PR down to the test. The assertion should happen only in cases where the compiler previously failed to give an error when it should. I guess I'd rather have the assert than a failure to error, and in any case we would only ship an extremely urgent fix to 3.7. I don't think that #33732 is that urgent. |
5 participants
Fixes #33732