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

Fix issue #24085 #24360

Closed
wants to merge 2 commits into from
Closed

Fix issue #24085 #24360

wants to merge 2 commits into from

Conversation

bkoropoff
Copy link
Contributor

Perform trait selection for builtin traits with a throw-away inference context, restoring the behavior prior to 83ef304. This prevents unwanted region constraints that lead to later errors.

This is the simplest way to fix the issue, but I'm not sure it's the correct one. I suspect Copy impl selection is generating overly-strict region equality constraints, so another approach might be to throw more type/region variables at it until inference is happy.

cc @nikomatsakis

Perform trait selection for builtin traits with a throw-away
inference context, restoring the behavior prior to 83ef304.
This prevents unwanted region constraints that lead to later
errors.
@rust-highfive
Copy link
Collaborator

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@huonw
Copy link
Member

huonw commented Apr 13, 2015

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned huonw Apr 13, 2015
@nikomatsakis
Copy link
Contributor

@bkoropoff sorry for dropping the ball on investigating this, thanks for digging into it. I agree with you that I am not 100% sure if this is the right fix. I have to dig into the example a bit to sort out my thinking.

@bkoropoff
Copy link
Contributor Author

@nikomatsakis I have an alternative fix that inserts an inference variable. It seems to work and produces a region graph that looks similar to what I get from invoking an ordinary trait method. I'll submit it as a PR once I get a clean test run.

@nikomatsakis
Copy link
Contributor

Having investigated this, I don't think that this is the correct fix. I'm going to open my own more targeted PR shortly. In particular, the region constraints being added are correct and real, it's just that the region inference is handling them incorrectly. Thanks @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.

4 participants