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

A few fixes for associated types #26147

Merged
merged 2 commits into from
Jun 18, 2015
Merged

A few fixes for associated types #26147

merged 2 commits into from
Jun 18, 2015

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 9, 2015

@arielb1 arielb1 force-pushed the assoc-trans branch 2 times, most recently from 49928f3 to 3569797 Compare June 13, 2015 18:04
@nikomatsakis
Copy link
Contributor

So, some thoughts:

First, I think trans shouldn't encounter this path, since we always know what trait a closure implements, but I could be wrong -- perhaps it encounter ambiguity in some other way? Presumably something is missing in this explanation, because (according to you) normalization is not needed during typeck and (according to me) this path doesn't matter during trans, but that would imply that this fix doesn't fix anything, right?

Second, I think we could make the code more robust in two ways:

  1. refusing to normalize if the result creates obligations that must be resolved (seems like an arbitrary limitation, but if this path is really only important to trans, perhaps it's enough to just "not act" until such time as enough progress has been made to resolve the ambiguity)
  2. extending the inferface to allow those obligations to be propagated back

(Longer term, as I keep mentioning, I think I'd like to move to a lazy normalization scheme.)

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 18, 2015

@nikomatsakis

Actually, we probably can't really encounter this during trans (because there is no potential ambiguous impl for ty_closure: Fn*).

@nikomatsakis
Copy link
Contributor

@arielb1 ok that's what I thought. Are you going to push an updated version with two fns, as we discussed in IRC? (If so, please do ping me when it's ready...)

@nikomatsakis
Copy link
Contributor

@bors r+ -- this looks nice. thanks for iterating with me.

@bors
Copy link
Contributor

bors commented Jun 18, 2015

📌 Commit 21fd312 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 18, 2015

⌛ Testing commit 21fd312 with merge ff8fee1...

@bors bors merged commit 21fd312 into rust-lang:master Jun 18, 2015
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