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

Carrier trait (third attempt) #35777

Merged
merged 3 commits into from
Aug 22, 2016
Merged

Carrier trait (third attempt) #35777

merged 3 commits into from
Aug 22, 2016

Conversation

nrc
Copy link
Member

@nrc nrc commented Aug 18, 2016

This adds a Carrier trait to operate with ?. The only public implementation is for Result, so effectively the trait does not exist, however, it ensures future compatibility for the ? operator. This is not intended to be used, nor is it intended to be a long-term solution.

Although this exact PR has not been through Crater, I do not expect it to be a breaking change based on putting numerous similar PRs though Crater in the past.

cc:

r? @nikomatsakis

Allows use with `Option` and custom `Result`-like types.
@nrc
Copy link
Member Author

nrc commented Aug 18, 2016

cc @rust-lang/lang

let sub_expr = self.signal_block_expr(hir_vec![],
sub_expr,
e.span,
hir::PopUnstableBlock,
Copy link
Contributor

@nikomatsakis nikomatsakis Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this really be hir::PopUnstableBlock? I'd expect hir::DefaultBlock. I see, unstable, of course. I read this as unsafe.

@nikomatsakis
Copy link
Contributor

@nrc code looks good to me, it'd be nice though to add a test regarding the type inference effects of ? on e.g. a call to collect() (just to show it doesn't, in fact, work with trait as currently designed)

@nikomatsakis
Copy link
Contributor

r=me with test -- though there remains the policy question. I think it makes sense to get the carrier trait in, and then debate about improving it -- I guess in the interim people can use try! to influence inference if needed. Interesting that we don't use this pattern in the compiler I guess.

@nrc nrc force-pushed the carrier3 branch 2 times, most recently from 080885a to 5aa89d8 Compare August 19, 2016 01:26
@nrc
Copy link
Member Author

nrc commented Aug 19, 2016

@bors r=@nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 19, 2016

📌 Commit 5aa89d8 has been approved by @nikomatsakis

The dummy impl should ensure the same type checking behaviour as having other (real) Carrier impls.
@steveklabnik
Copy link
Member

Wait, the RFC did not include Carrier, but this is getting merged anyway?

@nrc
Copy link
Member Author

nrc commented Aug 19, 2016

@steveklabnik Due to type inference issues, we can't stabilise ? without some Carrier trait - that would mean we could never add any Carrier trait backwards compatibly. So the lang and libs teams decided that we should add this minimal Carrier with only Result and dummy impls. It's certainly not the long-term solution and is not recommended for wide use (thus no announcement, etc.). Furthermore, when we merged the RFC without the Carrier trait, we did explicitly state that we wanted to experiment in that direction; this trait comes under that label of experimentation.

@nikomatsakis
Copy link
Contributor

@steveklabnik right, as @nrc said, the intention is that if we will write an RFC with the actual design for Carrier. We are currently discussing that design in rust-lang/rfcs#1718. If that RFC is accepted, then Carrier (or whatever it winds up being named) will undergo the stabilization process. If we decide against that RFC, we'll just remove Carrier. But if we were to stabilize ? now, without Carrier, we would be ruling out the possibility of having Carrier ever.

@steveklabnik
Copy link
Member

Thanks both! I get it now.

@nrc
Copy link
Member Author

nrc commented Aug 21, 2016

@bors: r=@nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 21, 2016

📌 Commit c32456d has been approved by @nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 21, 2016

⌛ Testing commit c32456d with merge 42584d3...

bors added a commit that referenced this pull request Aug 21, 2016
Carrier trait (third attempt)

This adds a `Carrier` trait to operate with `?`. The only public implementation is for `Result`, so effectively the trait does not exist, however, it ensures future compatibility for the `?` operator. This is not intended to be used, nor is it intended to be a long-term solution.

Although this exact PR has not been through Crater, I do not expect it to be a breaking change based on putting numerous similar PRs though Crater in the past.

cc:
* [? tracking issue](#31436)
* [previous PR](#35056)
* [RFC issue](rust-lang/rfcs#1718) for discussion of long-term Carrier trait solutions.

r? @nikomatsakis
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's a bit late since this already got merged, but shouldn't there be a comment explaining why an impl for a dummy type exists? I assume that some people already know this, but others would not (like me :) ).

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.

5 participants