Skip to content

Fix #33732 #36077

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 3 commits into from
Mar 17, 2020
Merged

Fix #33732 #36077

merged 3 commits into from
Mar 17, 2020

Conversation

jack-williams
Copy link
Collaborator

Fixes #33732

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

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.

@weswigham weswigham assigned sandersn and unassigned weswigham Mar 10, 2020
@arcanis
Copy link

arcanis commented Mar 10, 2020

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.

@jack-williams
Copy link
Collaborator Author

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).
If not, I can remove the change but I suggest keeping the test for regressions.

@sandersn
Copy link
Member

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.

@sandersn sandersn merged commit c600aa7 into microsoft:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Error: Debug Failure. False expression: parameter should have errors when reporting errors
5 participants