Skip to content
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

Alternate fix for issue #24085 #24404

Closed
wants to merge 2 commits into from

Conversation

bkoropoff
Copy link
Contributor

This implements my suggestion in #24360 of throwing inference variables at the problem instead. It works, though at the cost of making inference do more work.

r? @nikomatsakis

Insert an inference variable before performing trait selection for
builtin traits.  This prevents overly-restrictive region constraints
that lead to later inference failures.
@nikomatsakis
Copy link
Contributor

Hmm, I definitely prefer this version. I was just planning to checkout your other branch and dig more deeply and try to make sure I understood what was happening! I'll do that anyway.

@nikomatsakis
Copy link
Contributor

OK, so I dug into this test. I think the bug has to do with one of the darker corners of region inference (the "givens" field, specifically). I have a fix that extends the current hack, but it's really getting high time to rewrite the region inference code -- and in particular the way it handles closures. (I've also been sketching a design of that that I will try to write-up and prototype, but that's a separate thing...)

@steveklabnik
Copy link
Member

@nikomatsakis @bkoropoff how do you feel about this patch today?

@nikomatsakis
Copy link
Contributor

I think we should close this in favor of #24527. I've been criminally negligent in not benchmarking and rebasing that. I'll try to get to that today. Sorry @bkoropoff :(

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.

3 participants