Use a Carrier trait with the `?` operator #33389

nrc · May 3, 2016

Allows use with Option and custom Result-like types.

Note that this part of RFC 243 did not get approved, so this PR is just for experimentation purposes.

This supports conversion between result types when they have the same number of type args, but not otherwise (e.g, Result to Option), that requires #33108

Part of #31436

rust-highfive · May 3, 2016

r? @alexcrichton

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

durka · May 3, 2016

The big thing for me isn't Result <-> Option -- I'd like to experiment with some kind of DebugResult that builds up a backtrace as it passes through question marks.

jimmycuadra · May 3, 2016

jimmycuadra reviewed May 3, 2016

View changes

jimmycuadra · May 3, 2016

src/libcore/ops.rs

@@ -2155,3 +2157,126 @@ pub trait BoxPlace<Data: ?Sized> : Place<Data> {
    /// Creates a globally fresh place.
    fn make_place() -> Self;
 }
+
+/// A trait for types which have success and error states and are meant to work
+/// with the quiestion mark operator.


quiestion --> question

durka · May 4, 2016

durka reviewed May 4, 2016

View changes

durka · May 4, 2016

src/test/run-pass/try-operator-custom.rs

+}
+
+fn g(x: i32) -> MyResult<i32, String> {
+    let _y = f(x)?;


this was supposed to be x-1 (or on line 46)

it terminates with the given input, I don't care too much about it being a sensible program

Yeah, it terminates with a stack overflow...
On May 4, 2016 4:49 PM, "Nick Cameron" notifications@github.com wrote:

In src/test/run-pass/try-operator-custom.rs
#33389 (comment):

MyResult::Terrible(e) => T::from_error(e.into()),

}

}
+}

+fn f(x: i32) -> Result<i32, String> {

if x == 0 {

Ok(42)

} else {

let y = g(x)?;

Ok(y)

}
+}

+fn g(x: i32) -> MyResult<i32, String> {

let _y = f(x)?;

it terminates with the given input, I don't care too much about it being a
sensible program

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/33389/files/0de7b23ac739307c71249116d106c5d074f30b23#r62111982

The best kind of termination!

Thanks, this is fixed now.

durka · May 4, 2016

durka reviewed May 4, 2016

View changes

durka · May 4, 2016

src/librustc/hir/lowering.rs

@@ -1566,16 +1558,25 @@ pub fn lower_expr(lctx: &LoweringContext, e: &Expr) -> P<hir::Expr> {
                // to:
                //
                // {
-                //     match <expr> {
+                //     match { Carrier::translate( { <expr> } ) } {


I'm worried about this part of the desugaring. Carrier::translate consumes the value, so the Result-ish-thing is completely destroyed and then rebuilt when leaving the match statement. This precludes writing a carrier that would like to carry something more than the success/error values (for example, a backtrace). I am wondering if there is a way we can thread some more user-configurable state through the question marks.

For the record the idea that any error Carrier/ResultCarrier type should be isomorphic to Result was very intentional. (Not suggesting that this is therefore beyond debate. Just that it wasn't an oversight.)

huonw · May 9, 2016

huonw reviewed May 9, 2016

View changes

huonw · May 9, 2016

src/libcore/ops.rs

+        where T: Carrier<Success=U, Error=V>
+    {
+        match self {
+            Ok(u) => T::from_success(u.into()),


It seems to me that these intos are useless: they're always "converting" from U to U, and V to V. Maybe the signature of translate wants to be something like:

fn translate<T>(self) where T: Carrier<Success = Self::Success>, Self::Error: Into<T::Error>,

(The thinking being converting the main value implicitly is not nearly as desirable as converting the error one.)

I think the signature is OK, but the intos shouldn't be there. I started with a more flexible approach, but decided it wasn't necessary. I think the intos are left over from that. Not really sure why it even compiles.

Makes sense.

Not really sure why it even compiles.

T: Into<T> for all (sized) T:

rust/src/libcore/convert.rs

Lines 231 to 243 in 0e7cb8b

// From implies Into

#[stable(feature = "rust1", since = "1.0.0")]

impl<T, U> Into<U> for T where U: From<T> {

fn into(self) -> U {

U::from(self)

}

}

// From (and thus Into) is reflexive

#[stable(feature = "rust1", since = "1.0.0")]

impl<T> From<T> for T {

fn from(t: T) -> T { t }

}

bors · May 10, 2016

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

glaebhoerl · May 10, 2016

FWIW newer versions of the RFC called this ResultCarrier which is a bit less jargony. Cool to see it implemented!

nikomatsakis · May 13, 2016

Crater run results: https://gist.github.com/nikomatsakis/eff9c3cb0d79e95b073283369ab37b01

4702 crates tested: 2948 working / 1358 broken / 1 regressed / 0 fixed / 395 unknown.

nikomatsakis · May 13, 2016

The 1 regression appears to be a timeout (false positive).

nrc · May 18, 2016

The language team decided that we should accept this PR, on the understanding that it is for experimentation and will have a period of discussion and examination before FCP. This discuss thread has more details.

nrc · May 18, 2016

ping @alexcrichton see comment above from the lang team, also rebased.

diwic · May 18, 2016

This does not seem to change Into handling? I would have expected the into-part to move into the trait implementation somehow so it would be possible for a struct implementing Carrier to opt in (or opt out) from the into stuff.

Otherwise, +1 :-) I hope this goes in.

diwic · May 18, 2016

diwic reviewed May 18, 2016

View changes

diwic · May 18, 2016

src/libcore/ops.rs

+    /// The type of the value when computation succeeds.
+    type Success;
+    /// The type of the value when computation errors out.
+    type Error;


Perhaps EarlyReturn could be a better name than Error, as it more closely resembles what actually happens? Returning early does not have to be an error.

bors · May 18, 2016

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

alexcrichton · May 18, 2016

r? @aturon

We discussed this in libs a day or so ago and @aturon is gonna write up our conclusions here

aturon · May 18, 2016

Sorry for the delayed writeup.

The libs team also talked about this PR, and a number of team members felt uncomfortable merging based on the accepted RFC, strongly preferring to amend the RFC with an explicit route for experimentation, especially given the contentious nature of the original RFC.

Personally, I can see arguments on both sides, and I think the high-order bit here is that the broader community needs some clear opportunity to weigh in -- whether via an RFC amendment or the new discuss threads.

Given that the two teams reached different conclusions, I propose escalating this decision to the core team, and am tagging accordingly. We'll be meeting today.

aturon · May 23, 2016

We discussed the situation in the core team meeting, and the TL;DR is that we believe an amendment RFC needs to happen before this PR is landed. While not everyone agreed that it was strictly necessary, it is the conservative choice, which seems prudent given how contentious the original RFC was. (Sorry @nrc!)

The core issue is that the original RFC did not lay out specific plans for expansion here, and while the lang team did say that we would "revisit" the question prior to stabilization, there were no details about that process. Those opposed to this generalization did not push hard on that point in the original RFC because it was taken off the table. And at least some people don't consider this generalization to be something that needs experimental evidence to decide, but rather is a question of mental model for the feature.

In the future, any "planned expansions" should be clearly laid out in the RFC and made part of the tracking issue.

It's unclear whether the amendment RFC should await some consensus on the discuss thread, or should be started now to drive traffic in that direction. I'll cross-post there for the time being.

alexcrichton · Jul 19, 2016

@nrc ping (just cleaning out some old PRs)

Should we close this for now?

nrc · Jul 19, 2016

Yeah, I guess so, we're not going to merge it without an RFC, so may as well close in the meantime

rust-highfive assigned alexcrichton May 3, 2016

nrc force-pushed the nrc:carrier branch from 691c688 to bb8e59e May 3, 2016

nrc force-pushed the nrc:carrier branch from bb8e59e to 0de7b23 May 3, 2016

alexcrichton added the T-libs label May 4, 2016

nrc force-pushed the nrc:carrier branch 2 times, most recently from e89b9e4 to 79a1f5c May 4, 2016

nrc added T-lang I-nominated S-waiting-on-crater labels May 10, 2016

nrc force-pushed the nrc:carrier branch from 79a1f5c to 937f024 May 10, 2016

nrc removed I-nominated S-waiting-on-crater labels May 18, 2016

This was referenced May 18, 2016

Open

Tracking issue for `?` operator and `try` blocks (RFC 243, `question_mark` & `try_blocks` features) #31436

Merged

First-class error handling with `?` and `catch` rust-lang/rfcs#243

nrc force-pushed the nrc:carrier branch from 937f024 to f9ff8d3 May 18, 2016

Use a Carrier trait with the `?` operator
Allows use with `Option` and custom `Result`-like types.

Loading status checks…

f9ff8d3

rust-highfive assigned aturon and unassigned alexcrichton May 18, 2016

aturon added the T-core label May 18, 2016

nrc closed this Jul 19, 2016

rust-lang/rust

Join GitHub today

Use a Carrier trait with the `?` operator #33389

Conversation

nrc commented May 3, 2016

rust-highfive assigned alexcrichton May 3, 2016

This comment has been minimized.

rust-highfive commented May 3, 2016

nrc force-pushed the nrc:carrier branch from 691c688 to bb8e59e May 3, 2016

This comment has been minimized.

durka commented May 3, 2016

jimmycuadra reviewed May 3, 2016 View changes

This comment has been minimized.

nrc force-pushed the nrc:carrier branch from bb8e59e to 0de7b23 May 3, 2016

alexcrichton added the T-libs label May 4, 2016

durka reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

durka reviewed May 4, 2016 View changes

This comment has been minimized.

This comment has been minimized.

nrc force-pushed the nrc:carrier branch 2 times, most recently from e89b9e4 to 79a1f5c May 4, 2016

huonw reviewed May 9, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nrc added T-lang I-nominated S-waiting-on-crater labels May 10, 2016

nrc force-pushed the nrc:carrier branch from 79a1f5c to 937f024 May 10, 2016

This comment has been minimized.

bors commented May 10, 2016

This comment has been minimized.

glaebhoerl commented May 10, 2016

This comment has been minimized.

nikomatsakis commented May 13, 2016

This comment has been minimized.

nikomatsakis commented May 13, 2016

This comment has been minimized.

nrc commented May 18, 2016

nrc removed I-nominated S-waiting-on-crater labels May 18, 2016

This was referenced May 18, 2016

nrc force-pushed the nrc:carrier branch from 937f024 to f9ff8d3 May 18, 2016

This comment has been minimized.

nrc commented May 18, 2016

This comment has been minimized.

diwic commented May 18, 2016

diwic reviewed May 18, 2016 View changes

This comment has been minimized.

This comment has been minimized.

bors commented May 18, 2016

This comment has been minimized.

alexcrichton commented May 18, 2016

rust-highfive assigned aturon and unassigned alexcrichton May 18, 2016

This comment has been minimized.

aturon commented May 18, 2016

aturon added the T-core label May 18, 2016

This comment has been minimized.

aturon commented May 23, 2016

This comment has been minimized.

alexcrichton commented Jul 19, 2016

This comment has been minimized.

nrc commented Jul 19, 2016

nrc closed this Jul 19, 2016

nrc force-pushed the nrc:carrier branch from `691c688` to `bb8e59e` May 3, 2016

jimmycuadra reviewed May 3, 2016

View changes

nrc force-pushed the nrc:carrier branch from `bb8e59e` to `0de7b23` May 3, 2016

durka reviewed May 4, 2016

View changes

durka reviewed May 4, 2016

View changes

nrc force-pushed the nrc:carrier branch 2 times, most recently from `e89b9e4` to `79a1f5c` May 4, 2016

huonw reviewed May 9, 2016

View changes

nrc force-pushed the nrc:carrier branch from `79a1f5c` to `937f024` May 10, 2016

nrc force-pushed the nrc:carrier branch from `937f024` to `f9ff8d3` May 18, 2016

diwic reviewed May 18, 2016

View changes