catch blocks are not Ok-wrapping their value

[scottmcm]

Opening as #39849 (linked from tracking issue #31436) is closed.

From https://github.com/rust-lang/rfcs/blob/master/text/0243-trait-based-exception-handling.md#catch-expressions

If no exception is thrown, then the result is Ok(v) where v is the value of the block.

The current implementation does not; the following does not compile (2017-04-18):

let _: Result<i32, ()> = do catch { 4 };

Error message:

error[E0308]: mismatched types
 --> src/main.rs:3:41
  |
3 |     let _: Result<i32, ()> = do catch { 4 };
  |                                         ^ expected enum `std::result::Result`, found integral variable
  |
  = note: expected type `std::result::Result<i32, ()>`
             found type `{integer}`

Runnable: https://play.integer32.com/?gist=4e589a7b8f2ffff23f507fb66ac7f662&version=nightly

cc @nikomatsakis, who mentioned this in rust-lang/rfcs#1859 (comment)

[nikomatsakis]

cc @rust-lang/lang -- this is an interesting question. The catch { } blocks as written in the RFC would automatically "wrap" their resulting value, so one could do this:

fn main() {
    let x: Result<i32, ()> = do catch { 22 };
}

However, the catch blocks are implemented do not have this behavior, so one must do:

fn main() {
    let x: Result<i32, ()> = do catch { Ok(22) };
}

What behavior do we want here? I think the currently implemented behavior is natural, in some respects, in that inserting a fn foo() { ... } and fn foo() { do catch { ... } } both behave the same. That is, using a catch block changes the point to which ? propagates, but does not otherwise introduce any "new" behavior.

(Also, the do catch syntax is considered temporary, I'm just using it so that the examples are actually executable with the current implementation.)

[scottmcm]

To argue the opposite, I suspect the wrapping behaviour will be more convenient & consistent.

Manufactured example where I think the wrapping provides a nice consistency:

let r: Result<(), E> =
    do catch {
        foo()?;
        bar()?;
        qux()?;
    };

That's certainly better than writing this:

let r: Result<(), E> =
    do catch {
        foo()?;
        bar()?;
        qux()?;
        Ok(())
    };

But I also think it's better than this no-wrapping version, despite this being the shortest of the three:

let r: Result<(), E> =
    do catch {
        foo()?;
        bar()?;
        qux()
    };

That last call is obviously less consistent syntactically, but it's also less consistent semantically: it requires that qux return a Result<(), E> exactly, instead of just Result<T,F> where E:From<F>.

As for convenience, I'd gladly write the second ? in something like

do catch { x.checked_mul(y)?.checked_add(z)? }

in order to not have to write Some() in something like

do catch { x.checked_abs()? - 1 }

[withoutboats]

@scottmcm but then by the same token why shouldn't a function that returns a Try type attempt to wrap the final argument? I'm very used to having to write Ok(()) at the end of functions and I feel like I would expect the same thing here.

[scottmcm]

@withoutboats I think that'd be a win, yes. The ? RFC even described such a thing as a future possibility (0243-trait-based-exception-handling.md#throw-and-throws) and the new Try trait proposal is explicitly set up to support both success-wrapping and error-wrapping (rust-lang/rfcs#1859 (comment)).

And I strongly hope that ?-in-main (#1937) won't end up resulting in the guessing game tutorial needing an Ok(()) in order to use ?.

[withoutboats]

I'm a bit sympathetic to the idea but wouldn't that be a very big & breaking change?

[nikomatsakis]

@scottmcm

I would like some way to support "auto-wrapping", indeed, but I worry that the inconsistency will be confusing. I think it'll be hard to remember when you should have Ok() as the final thing and when you can avoid it.

If we are going to have this, I think I would prefer some sort of syntactic "opt-in" that one can use on a fn or a catch in the same way. (The original ? RFC had a throws keyword sort of like this.)

Alternatively, the original RFC (IIRC) also did not have catch { ... }, but rather try { ... } catch { pat => arm }. This did in a sense employ auto-wrap, but in a very particular way, since effectively the result of the try was unified with the type of the catch patterns.

In other words, with try-catch, it would work something like this:

let result_code = try {
    write!(...)?;
    0
} catch {
    Err(e) => 1,
};

This doesn't really feel like autowrapping to me, though in some desugarings autowrapping may have been involved.

[nikomatsakis]

We discussed in the @rust-lang/lang meeting, and I think the general feeling was that it is important to keep catch { x } and fn main() { x } as equivalent. However, we would like to support "auto-wrapping". One thought we had was that a useful coercion would be something like this:

• When propagating a value of type T "upward":
  • either as the tail expression in a catch block
  • or returning from a function
• If the expected type E from context implements E: Try<Ok = T>, then we could do a Try::from_ok() "auto-coercion"

@scottmcm -- would you be interested in working with a @rust-lang/lang member (probably myself) on an RFC to that effect?

[scottmcm]

I'm definitely interested. I'd been pondering what a solution could look like, but wouldn't have dared propose a new coercion.

[aturon]

@scottmcm Great! Ping me or @nikomatsakis on email or IRC to get things cooking?

[nikomatsakis]

After the long discussion on internals, I've had a change of heart here. Earlier I wrote:

I think the general feeling was that it is important to keep catch { x } and fn main() { x } as equivalent.

But now I think I was wrong to argue for that, and that we should revert to the original intent. catch { ... } ought to invoke Try::from_ok on its "natural" return value. This seems quite easy to achieve.

Why, you ask? In short, given that our attempts to find more universal coercions have failed, it seems clear that if we are going to do some form of ok-wrapping, it has to have syntactic opt-in. And, in that case, I don't see a future where catch isn't the block-level form of that opt-in.

In more detailed form:

• I think the need for some form of ok-wrapping is still fairly strong:
  • The subtle but important difference between (e.g.) catch { foo } and catch { Ok(foo?) } is both confusing and annoying -- the latter converts errors, the former doesn't.
    • the same applies, of course, to any function using ?, but leave that for now
  • Writing Ok/Some at each return site is readily forgotten, tedious, and (I believe) of little value
    • this last point can be contenious, I know
• However, doing this coercion universally in a type-based fashion is too ambiguous and has far too many hazards, so we need some syntactic opt-in.
• It seemed to me that proposal that garnered the most consensus in that thread and still met the objectives was to have some kind of "block-level" opt-in, much like catch.
  • Notably, fn foo() -> T { catch { .. } } is strikingly close to the original goals.
  • Also, applying to blocks and in the body does a good job of making it clear that this is an implementation detail and doesn't affect the "public interface".
• If that gained currency, one can imagine fn foo() -> T catch { ... } being a shorthand.
  • We would probably want to extend to fn foo() -> T unsafe { ... } (and, I suppose both).

[nikomatsakis]

Mentoring instructions

catch blocks are processed during HIR lowering. This works by building on the HIR's support for a break that targets a labeled block carrying a value. So something like

let x = do catch {
    Ok(foo?)
};

desugars to:

let x = 'a: {
    Ok(match foo {
        Ok(v) => v,
        Err(e) => break 'a Err(e.into()),
    })
};

The foo? expression, as you can see, has been converted into a match, where the Err arm uses break 'a to target the catch block.

The code to desugar a catch block lives here:

rust/src/librustc/hir/lowering.rs

Lines 2772 to 2775 in aafe7d8

	ExprKind::Catch(ref body) => {
	self.with_catch_scope(body.id, |this|
	hir::ExprBlock(this.lower_block(body, true)))
	}

The with_catch_scope method simply pushes the label for the catch block onto a stack, carried in a field of the builder:

rust/src/librustc/hir/lowering.rs

Line 105 in aafe7d8

catch_scopes: Vec<NodeId>,

Then when we desugar the ? operator:

rust/src/librustc/hir/lowering.rs

Lines 3197 to 3199 in aafe7d8

	// Desugar ExprKind::Try
	// From: `<expr>?`
	ExprKind::Try(ref sub_expr) => {

we can check this stack in the Err case and generate a break if it is non-empty:

rust/src/librustc/hir/lowering.rs

Lines 3273 to 3288 in aafe7d8

	let ret_expr = if let Some(catch_node) = catch_scope {
	P(self.expr(
	e.span,
	hir::ExprBreak(
	hir::Destination {
	label: None,
	target_id: hir::ScopeTarget::Block(catch_node),
	},
	Some(from_err_expr)
	),
	thin_attrs))
	} else {
	P(self.expr(e.span,
	hir::Expr_::ExprRet(Some(from_err_expr)),
	thin_attrs))
	};

(We don't need to alter the ? code here, but it's useful to see.)

What we want to change here is actually in the do catch itself. To review, that was this code:

rust/src/librustc/hir/lowering.rs

Lines 2772 to 2775 in aafe7d8

	ExprKind::Catch(ref body) => {
	self.with_catch_scope(body.id, |this|
	hir::ExprBlock(this.lower_block(body, true)))
	}

It seems like we want to lower:

do catch {
    ...;
    tail-expr
}

to the equivalent of

'a: {
    ...; // any `?` in here will be handled as today
    Try::from_ok(tail-expr)
}

This suggests that we want to take the return value from the function lower_block and post-process it, to insert the Try::from_ok call. I think that the existing code will change to something like:

 self.with_catch_scope(body.id, |this| {
    let mut block = this.lower_block(body, true);
    block.expr = wrap_in_from_ok_call(block.expr);
    hir::ExprBlock(block)
})

then all we have to do is write this wrap_in_from_ok_call function. For that, we want it to look something like the existing call to from_error, which is generated here:

rust/src/librustc/hir/lowering.rs

Lines 3264 to 3269 in aafe7d8

	let from_err_expr = {
	let path = &["ops", "Try", "from_error"];
	let from_err = P(self.expr_std_path(unstable_span, path,
	ThinVec::new()));
	P(self.expr_call(e.span, from_err, hir_vec![from_expr]))
	};

[scottmcm]

I'm going to take a stab at this. FIXME-laden progress so far: master...scottmcm:catch-wrapping