Skip to content

Fix logic error introduced in #30334 #30443

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 1 commit into from
Mar 18, 2019
Merged

Conversation

ahejlsberg
Copy link
Member

This PR fixes a nasty little logic error I inadvertently introduced in #30334. This logic error is the cause of the DT failures reported in #30372.

I see that we've merged #30375 which doesn't actually fix the core issue. I don't like the fact that a bunch of circularity errors disappear with #30375 and I'd like for us to revert that PR.

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.

Err, I don't see it. What's logically different between the before and after here?

Anyways, you wanna revert the checker changes from #30375 but keep the test so we can see this fix the thing?

@ahejlsberg
Copy link
Member Author

@weswigham I didn't see it either for quite a while. What's different is that when the !contains(...) check fails for contraCandidates (i.e. when the candidate is already in set of contravariant candidates), we proceed to the else clause which will add it to the covariant candidate set instead of doing nothing! And once we do that, who knows what happens. But the crazy thing is that it only goes wrong when we make two or more identical contravariant inferences.

@ahejlsberg
Copy link
Member Author

@weswigham Yes, let's keep the test from #30375 but otherwise revert the PR.

@weswigham
Copy link
Member

Ahhhhhh, I get it. Looking at the diff alone that's not obvious, lol.

@ahejlsberg ahejlsberg merged commit 02963b4 into master Mar 18, 2019
@ahejlsberg ahejlsberg deleted the fixTypeInferenceLogicError branch March 18, 2019 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants