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

Prevent coercions from polluting the fulfillment context #26324

Closed
wants to merge 1 commit into from

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 15, 2015

This adds transaction support to fulfill. I can't use it in #26282
because that would require nested transactions.

This doesn't seem to cause compilation time to regress.

Fixes #24819.

This adds transaction support to fulfill. I can't use it in rust-lang#26282
because that would require nested transactions.

This doesn't seem to cause compilation time to regress.

Fixes rust-lang#24819.
@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 15, 2015

r? @nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 18, 2015

☔ The latest upstream changes (presumably #26326) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

So, now that the fulfillment context has moved into the inference context, I'd like to integrate it fully into the transactional mechanism there. Sounds like you have a similar ambition. There are other changes I'd like to make -- for example, to track more fully the "parentage" of implications so that we can handle the global caching in a better way, and give better error reports -- and I have the feeling a complete rewrite is in order.

I am torn between saying let's just do the "Right Thing" and landing this PR in the interim. I don't like adding another variant on commit and so forth, but closing an ICE is always nice. So I guess I'm mostly happy to r+ but I'd like to move sooner rather than later on a more full rewrite. (Happy to talk about what that entails on IRC.)

cc @jroesch

@Gankra
Copy link
Contributor

Gankra commented Jul 27, 2015

Any progress on the decision?

@steveklabnik
Copy link
Member

Triage ping. Looks like this isn't going anywhere, should it be closed? Or did some sort of decision happen?

@arielb1
Copy link
Contributor Author

arielb1 commented Sep 27, 2015

@steveklabnik

@jroesch is working on a replacement for this patch.

@nikomatsakis
Copy link
Contributor

going to close in favor of @jroesch's anticipated PR, thanks @arielb1

@jroesch
Copy link
Member

jroesch commented Sep 28, 2015

@nikomatsakis sounds good, should be soon finally settled in (and have importantly have internet).

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.

8 participants