[control flow analysis] Special treatment for `Err( )?`

[DCjanus]

I don't know this issue should be feature request or bug report.

I have code like this:

#[derive(Debug, Fail)]
#[fail(display = "Target can't open: {:?}", path)]
struct NotExists {
    path: PathBuf
}

#[derive(Debug, Fail)]
#[fail(display = "Target is not file: {:?}", path)]
struct NotFile {
    path: PathBuf
}

fn foo() -> Result<(), failure::Error> {
    let path = PathBuf::from("Rust/is/the/best/language");
    if path.exists() {
        Err(NotExists { path })?
    }
    if path.is_dir() {
        Err(NotFile { path })?
    }
    unimplemented!()
}

The compiler error is:

24 |         Err(NotExists { path })?
   |                         ---- value moved here
25 |     }
26 |     if path.is_dir() {
   |        ^^^^ value used here after move

Obviously, Err(foo)? equals return Err(foo.into());, that means there should be no use after moved error.

There are two other ways to achieve the goal:

    if path.exists() {
        Err(NotExists { path })?
    } else if path.is_dir() {
        Err(NotFile { path })?
    } else {
        unimplemented!()
    }

or

    if path.exists() {
        return Err(NotExists { path }.into());
    }

I think with control flow analysis, compiler should be able to pass my origin code(Err(foo)? version).

[ishitatsuyuki]

Have you tried this with NLL?

[Emerentius]

This requires value based control flow analysis which is imho expecting a bit much from the compiler. During the construction of Err(_) you're consuming the PathBuf and then you're doing a conditional match on it with ?.
From the type system's view that is just the same as matching any generic Result<T, E>, it could be Ok(_). It will not desugar into an unconditional return.

I don't think we should be adding a special case for this.

[est31]

I don't think we should be adding a special case for this.

I think we should.

There are proposals to add even more error custom functionality to the language, like a throw keyword or similar. But IMO that's the wrong approach. People already have the option to use macros for that kind of thing. People already can do return Err().into(). But they still would prefer Err()? to diverge.

The analysis whether the code is dead or not is already implemented inside the compiler, for the purpose of lints. In fact, ? operator desugaring has to add #[allow(unreachable_code)] to the two match arms so that the lint isn't emitted. I think we can do better than this.

For reference, this is the desugaring inside the compiler:

rust/src/librustc/hir/lowering.rs

Lines 3533 to 3629 in 2532056

	// Desugar ExprKind::Try
	// From: `<expr>?`
	ExprKind::Try(ref sub_expr) => {
	// to:
	//
	// match Try::into_result(<expr>) {
	// Ok(val) => #[allow(unreachable_code)] val,
	// Err(err) => #[allow(unreachable_code)]
	// // If there is an enclosing `catch {...}`
	// break 'catch_target Try::from_error(From::from(err)),
	// // Otherwise
	// return Try::from_error(From::from(err)),
	// }
	let unstable_span =
	self.allow_internal_unstable(CompilerDesugaringKind::QuestionMark, e.span);
	// Try::into_result(<expr>)
	let discr = {
	// expand <expr>
	let sub_expr = self.lower_expr(sub_expr);
	let path = &["ops", "Try", "into_result"];
	let path = P(self.expr_std_path(unstable_span, path, ThinVec::new()));
	P(self.expr_call(e.span, path, hir_vec![sub_expr]))
	};
	// #[allow(unreachable_code)]
	let attr = {
	// allow(unreachable_code)
	let allow = {
	let allow_ident = Ident::from_str("allow").with_span_pos(e.span);
	let uc_ident = Ident::from_str("unreachable_code").with_span_pos(e.span);
	let uc_nested = attr::mk_nested_word_item(uc_ident);
	attr::mk_list_item(e.span, allow_ident, vec![uc_nested])
	};
	attr::mk_spanned_attr_outer(e.span, attr::mk_attr_id(), allow)
	};
	let attrs = vec![attr];
	// Ok(val) => #[allow(unreachable_code)] val,
	let ok_arm = {
	let val_ident = self.str_to_ident("val");
	let val_pat = self.pat_ident(e.span, val_ident);
	let val_expr = P(self.expr_ident_with_attrs(
	e.span,
	val_ident,
	val_pat.id,
	ThinVec::from(attrs.clone()),
	));
	let ok_pat = self.pat_ok(e.span, val_pat);
	self.arm(hir_vec![ok_pat], val_expr)
	};
	// Err(err) => #[allow(unreachable_code)]
	// return Try::from_error(From::from(err)),
	let err_arm = {
	let err_ident = self.str_to_ident("err");
	let err_local = self.pat_ident(e.span, err_ident);
	let from_expr = {
	let path = &["convert", "From", "from"];
	let from = P(self.expr_std_path(e.span, path, ThinVec::new()));
	let err_expr = self.expr_ident(e.span, err_ident, err_local.id);
	self.expr_call(e.span, from, hir_vec![err_expr])
	};
	let from_err_expr =
	self.wrap_in_try_constructor("from_error", from_expr, unstable_span);
	let thin_attrs = ThinVec::from(attrs);
	let catch_scope = self.catch_scopes.last().map(|x| *x);
	let ret_expr = if let Some(catch_node) = catch_scope {
	P(self.expr(
	e.span,
	hir::ExprBreak(
	hir::Destination {
	label: None,
	target_id: Ok(catch_node),
	},
	Some(from_err_expr),
	),
	thin_attrs,
	))
	} else {
	P(self.expr(e.span, hir::Expr_::ExprRet(Some(from_err_expr)), thin_attrs))
	};
	let err_pat = self.pat_err(e.span, err_local);
	self.arm(hir_vec![err_pat], ret_expr)
	};
	hir::ExprMatch(
	discr,
	hir_vec![err_arm, ok_arm],
	hir::MatchSource::TryDesugar,
	)
	}

We could either fix the desugaring, or alternatively, perform basic value based control flow analysis.

[nagisa]

Note that both Err and Try are library types, so doing this sort of stuff without both of them being const fn (Try::into_result isn’t) is simply not implementable in general case.

[scottmcm]

Repro without needing failure: https://play.rust-lang.org/?gist=e0f0826e1016be6bae37d8e0ad58b6cb

This could be fixed in inference, actually, since with nll this is fine:

Err::<!, _>(MyError { path })?;

https://play.rust-lang.org/?gist=3c15d85f320923710b8c772c6789a05e&version=nightly

It's only because inference defaults the type parameter to () that things go wrong.

(Another solution would be to make enum variants real types, but that's a huge change.)

[kennytm]

Please keep it open. As mentioned in #51588 (comment), NLL is only half of the solution, we also need to change the inference so that Err(e)? is inferred as Err::<!, _>(e)?.