FP: needless_return_with_question_mark with implicit Error Conversion
#12021
Conversation
|
r? @Alexendoo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
Return with a question mark was triggered in situations where the `?` desuraging was performing error conversion via `Into`/`From`. The desugared `?` produces a match over an expression with type `std::ops::ControlFlow<B,C>` with `B:Result<Infallible, E:Error>` and `C:Result<_, E':Error>`, and the arms perform the conversion. The patch adds another check in the lint that checks that `E == E'`. If `E == E'`, then the `?` is indeed unnecessary. changelog: False Positive: `needless_return_with_question_mark` when implicit Error Conversion occurs.
|
@rustbot r? clippy |
clippy_lints/src/returns.rs
Outdated
| /// The expression of the desugared `try` operator is a match over an expression with type: | ||
| /// `ControlFlow<A:Result<Infallible, E>, B:Result<_, E'>>`, with final type `B`. | ||
| /// If E and E' are the same type, then there is no error conversion happening. | ||
| /// Error conversion happens when E can be transformed into E' via a `From` or `Into` conversion. |
There was a problem hiding this comment.
@Jarcho You usually know about utils in rustc/Clippy. Do you happen to know of a better way to do this, that doesn't require matching on the exact desugaring of the ? operator?
There was a problem hiding this comment.
If not, this LGTM and you can r=me
There was a problem hiding this comment.
Isn't the lint suggesting to remove the return but leave the ?? There isn't a need to check for type changes in FromResidual for that. There is an error with the lint where it doesn't verify the existence of the Err constructor before linting.
There was a problem hiding this comment.
Oh right. The ? is a red herring here... The suggestion of removing the return in the given test/example compiles just fine. It only gives an additional warning that the result is unused. But that is because the map turns the Result<()> returned by bar into a Result<Result<()>>, which is a bit nonsensical.
With the given examples and reproducers, I would call this a non-issue and that the lint does the right thing here. I think I need a more realistic example to understand what the actual issue is in this case. The lint definitely doesn't suggest to remove the ?.
There was a problem hiding this comment.
The better question is: Why does it suggest to remove the return here in the first place: The lint documentation only talks about the explicit case of return Err(...) and not more complex expressions, like in the example. Either the documentation is outdated, or the lint is misbehaving a lot.
There was a problem hiding this comment.
Could it be that the desugaring is actually causing the misbehaviour here?
There was a problem hiding this comment.
I'm not sure. I'm not familiar with the code of this lint, sadly :/
|
Closed because the idea doesn't actually work. |
|
I have tried a new approach that I think is simpler, needs fewer details, and correctly matches more stuff. Sorry for any confusion. |
|
Thanks! This LGTM now. @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Return with a question mark was triggered in situations where the
?desuraging was performing error conversion viaInto/From.The desugared
?produces a match over an expression with typestd::ops::ControlFlow<B,C>withB:Result<Infallible, E:Error>andC:Result<_, E':Error>, and the arms perform the conversion. The patch adds another check in the lint that checks thatE == E'. IfE == E', then the?is indeed unnecessary.changelog: False Positive: [
needless_return_with_question_mark] when implicit Error Conversion occurs.fixes: #11982