Suggest coercion of Result using ?

[estebank]

Fix #47560.

[rustbot]

r? @oli-obk

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

[compiler-errors]

This phrasing feels a bit roundabout... Also, it should really mention that it's doing control flow, since it's not equivalent to .map_err(Into::into), which is what this currently reads like.

[compiler-errors]

In return position, it's fine to ignore the fact that this is doing control flow, but even if you hadn't applied my suggestion to extend this to all type mismatch suggestions, it's still happening on any block-tail usage of CoerceMany, which is not always in the return of an fn.

[estebank]

Before the requested change, this was checking the obligation cause code explicitly for the cases where this was happening in return statements and function tails exclusively :-/

[compiler-errors]

where this was happening in return statements and function tails exclusively :-/

Wait, like I said in:

but even if you hadn't applied my suggestion to extend this to all type mismatch suggestions, it's still happening on any block-tail usage of CoerceMany, which is not always in the return of an fn.

This concern still applies to the old code too.

The original commit bb0f8c1098d7191f1c27ce7ee3ee2bafcf99c6ca used the BlockTailExpression obligation to gate this suggestion, which isn't equivalent to checking if something is in the function tail exclusively. For example:

struct A;
struct B;

impl From<A> for B {
    fn from(_: A) -> Self { B }
}

fn foo() -> Result<(), B> {
    let y = Err(A);
    let z: Result<(), B> = { y };
    Ok(())
}

Gives:

help: you can rely on the implicit conversion that `?` does to transform the error type
   |
10 |     let z: Result<(), B> = { Ok(y?) };
   |                              +++ ++

Since the expression is in the tail of that block expression. This happens on that earlier commit, which still really deserves either fixing (making the suggestion only apply in tail position), or mentioning control flow.

[estebank]

Fixed

[estebank]

Regarding the wording, do you have one in mind? I'm somewhat relying on prior understanding of what ? does, but because the suggestion is always for returned values, it should never affect behavior, regardless of understanding.

[oli-obk]

r? @compiler-errors

[compiler-errors]

Maybe something like

Suggested change

	"you can rely on the implicit conversion that `?` does to transform the error type",
	"use `?` then wrap the value in `Ok` to return an `Err` result and coerce the `Ok` variant into the right `Result` type",

?? no idea if that's clearer 😅

[estebank]

I'm not too happy with that explanation because I feel it is too much. I want us to take into consideration the context we are in: we have some Result<_, A> and we expected Result<_, B> where B: Into<A>. Because of that I want to focus on "writing the following you can turn the value you have into the expected type". Maybe "you can use ? to coerce and return an appropriate Err, and wrap the resulting value in Ok so the expression remains of type Result"?

[compiler-errors]

I'm fine with that wording minus the "you can". I personally prefer suggestions worded in the imperative mood, but r=me either way.

[estebank]

@bors r=compiler-errors

[bors]

📌 Commit 62aff3b has been approved by compiler-errors

It is now in the queue for this repository.

[riking]

This test case has a mistake - you meant to test Err(A) not Err(A);.

[estebank]

It works as a negative test :)

I don't recall now if I intended for that to be the case, which is likely not.