-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
extend ?
to operate over other types
#1859
Conversation
I don't know how to make a "rendered" link anymore. =) |
text/0000-try-trait.md
Outdated
@@ -0,0 +1,478 @@ | |||
- Feature Name: (fill me in with a unique ident, my_awesome_feature) | |||
- Start Date: (fill me in with today's date, YYYY-MM-DD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you want to fill these in? ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
text/0000-try-trait.md
Outdated
### What to name the trait | ||
|
||
A number of names have been proposed for this trait. The original name | ||
was `Carrier`, as the trait "carrier" an error value. A proposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear if
as the trait "carrier" an error value
Is a typo or an example of why the previous name was poor. I'd have expected "carries"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Thank you @nikomatsakis! I am excited to watch the progress of this RFC!
Depending on the level of detail envisioned, I'd argue that doing this may be too much. Explicit return types already seem different enough from the exception-like handling that many newcomers are used to; expanding the universe immediately by making it generic might be overwhelming. I'd lean towards a throwaway sentence that indicates that the behavior of
I don't have a strong opinion on implementing |
TL;DR: I'm OK with
In the lead-up to the RFC, I expressed mild concern at the use of My main concern is against using |
A very common problem that new rustaceans hit is trying to use I totally understand if this RFC is orthogonal to the issue I'm interested in :) EDIT: Aha, found the discussion more focused on |
I personally think that enum TryResult<T, R> {
Yield(T),
Return(R),
} Than |
I agree we need to correct the error message. I also think that it's worth thinking and drafting error messages here, in this RFC -- too often we leave it as an afterthought. Do you have suggestions for the wording? That said, when you wrote "Seems like that will be a totally separate RFC than this one, correct?", I'd say that this is the overkill. That is, we can (and do!) improve error messages without the need for RFCs. I would think the message would be something like:
We could also go further and analyze the type to which I can add some text to this effect. UPDATE: Done. |
@shepmaster @clarcharr Regarding the use of
Indeed, I would remove |
Ah, @clarcharr, regarding the names |
@nikomatsakis Personally I think that throwway types are one of the things that makes Rust really powerful. In projects I tend to use enums extensively even if they only have two variants, because I think it makes things more clear, and they're not as equivalent to As far as Yield goes, maybe Continue? |
Oh, I guess in my head improving the error message seems tied up with this discussion about treating
That would be better than what's currently there, definitely. I'd also like a companion note that makes a recommendation like "Consider extracting the code where you want to use the
So I don't think "consider changing the return type to a Result" is an appropriate suggestion for I do like what you have in the text for the case when someone tries to use |
I agree, although my preference would be to implement the solutions proposed in this thread on internals, such that you could change the return type on
In any case, good point, I see no reason we cannot distinguish the context. In the case of a trait, one could also imagine that, in the case that the trait is thread-local, suggesting that one might consider modifying the trait itself.
To me it seems like overkill. In the first case, it doesn't seem worth mentioning types that do implement try, since the value in question is not of those types. In the second, it's not clear that implementing try is a good idea (or even possible, given the coherence rules). |
@carols10cents updated to address your latest suggestions |
Ah, I overlooked this. I don't quite understand what pattern you are suggesting, can you maybe given an example? |
When |
Oh, I just mean like in this comment, where your
and then |
Agreed.
I see. It seems to me that this is a lot to fit into the immediate error message, but it might be a good candidate for the extended error message. |
<3 love the latest changes in f89568b, this looks great now :) |
Thx @bluss for pointing me here. I asked in rust-lang/rust#31436 why the conversion was implemented using I thought I needed |
ping @pnkfelix needs signoff - #1859 (comment) (I want my glorious |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period is now complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(typo)
text/0000-try-trait.md
Outdated
|
||
```rust | ||
fn foo() -> Poll<T, E> { | ||
let x = bar()?; // propogate error case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"propagate"
text/0000-try-trait.md
Outdated
|
||
```rust | ||
fn foo() -> Result<T, E> { | ||
let x = bar()?; // propogate error case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"propagate"
Thanks all. I've merged the RFC and re-used the existing tracking issue. |
Lower `?` to `Try` instead of `Carrier` The easy parts of rust-lang/rfcs#1859, whose FCP completed without further comments. Just the trait and the lowering -- neither the error message improvements nor the insta-stable impl for Option nor exhaustive docs. Based on a [github search](https://github.com/search?l=rust&p=1&q=question_mark_carrier&type=Code&utf8=%E2%9C%93), this will break the following: - https://github.com/pfpacket/rust-9p/blob/00206e34c680198a0ac7c2f066cc2954187d4fac/src/serialize.rs#L38 - https://github.com/peterdelevoryas/bufparse/blob/b1325898f4fc2c67658049196c12da82548af350/src/result.rs#L50 The other results appear to be files from libcore or its tests. I could also leave Carrier around after stage0 and `impl<T:Carrier> Try for T` if that would be better. r? @nikomatsakis Edit: Oh, and it might accidentally improve perf, based on rust-lang#37939 (comment), since `Try::into_result` for `Result` is an obvious no-op, unlike `Carrier::translate`.
Lower `?` to `Try` instead of `Carrier` The easy parts of rust-lang/rfcs#1859, whose FCP completed without further comments. Just the trait and the lowering -- neither the error message improvements nor the insta-stable impl for Option nor exhaustive docs. Based on a [github search](https://github.com/search?l=rust&p=1&q=question_mark_carrier&type=Code&utf8=%E2%9C%93), this will break the following: - https://github.com/pfpacket/rust-9p/blob/00206e34c680198a0ac7c2f066cc2954187d4fac/src/serialize.rs#L38 - https://github.com/peterdelevoryas/bufparse/blob/b1325898f4fc2c67658049196c12da82548af350/src/result.rs#L50 The other results appear to be files from libcore or its tests. I could also leave Carrier around after stage0 and `impl<T:Carrier> Try for T` if that would be better. r? @nikomatsakis Edit: Oh, and it might accidentally improve perf, based on rust-lang#37939 (comment), since `Try::into_result` for `Result` is an obvious no-op, unlike `Carrier::translate`.
Introduce a trait
Try
for customizing the behavior of the?
operator when applied to types other thanResult
.Fixes #1718.
Rendered: https://github.com/rust-lang/rfcs/blob/master/text/1859-try-trait.md
[edited to link to final rendered version]