Rust: [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:

```Rust
    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:

https://github.com/rust-lang/rust/blob/253205658edc477a0b429f3ce25a92099dc7ddc4/src/librustc/hir/lowering.rs#L3533-L3629

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 https://github.com/rust-lang/rust/issues/51588#issuecomment-397921611, NLL is only half of the solution, we also need to change the inference so that Err(e)? is inferred as Err::<!, _>(e)?.