Skip to content

Commit

Permalink
FP: needless_return_with_question_mark with implicit Error Conversion
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
m-rph committed Dec 26, 2023
1 parent 9dd2252 commit 57dd25e
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 1 deletion.
28 changes: 27 additions & 1 deletion clippy_lints/src/returns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,37 @@ fn stmt_needs_never_type(cx: &LateContext<'_>, stmt_hir_id: HirId) -> bool {
})
}

///
/// 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.
fn desugar_expr_performs_error_conversion(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
let ty = cx.typeck_results().expr_ty(expr);

if let ty::Adt(_, generics) = ty.kind()
&& let Some(brk) = generics.first()
&& let Some(cont) = generics.get(1)
&& let Some(brk_type) = brk.as_type()
&& let Some(cont_type) = cont.as_type()
&& let ty::Adt(_, brk_generics) = brk_type.kind()
&& let ty::Adt(_, cont_generics) = cont_type.kind()
&& let Some(brk_err) = brk_generics.get(1)
&& let Some(cont_err) = cont_generics.get(1)
&& let Some(brk_err_type) = brk_err.as_type()
&& let Some(cont_err_type) = cont_err.as_type()
{
return brk_err_type != cont_err_type;
}
false
}

impl<'tcx> LateLintPass<'tcx> for Return {
fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
if !in_external_macro(cx.sess(), stmt.span)
&& let StmtKind::Semi(expr) = stmt.kind
&& let ExprKind::Ret(Some(ret)) = expr.kind
&& let ExprKind::Match(.., MatchSource::TryDesugar(_)) = ret.kind
&& let ExprKind::Match(match_expr, _, MatchSource::TryDesugar(..)) = ret.kind
// Ensure this is not the final stmt, otherwise removing it would cause a compile error
&& let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(expr.hir_id))
&& let ItemKind::Fn(_, _, body) = item.kind
Expand All @@ -192,6 +217,7 @@ impl<'tcx> LateLintPass<'tcx> for Return {
&& final_stmt.hir_id != stmt.hir_id
&& !is_from_proc_macro(cx, expr)
&& !stmt_needs_never_type(cx, stmt.hir_id)
&& !desugar_expr_performs_error_conversion(cx, match_expr)
{
span_lint_and_sugg(
cx,
Expand Down
27 changes: 27 additions & 0 deletions tests/ui/needless_return_with_question_mark.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,30 @@ fn issue11616() -> Result<(), ()> {
};
Ok(())
}

/// This is a false positive that occurs because of the way `?` is handled.
/// The `?` operator is also doing a conversion from `Result<T, E>` to `Result<T, E'>`.
/// In this case the conversion is needed, and thus the `?` operator is also needed.
fn issue11982() {
mod bar {
pub struct Error;
pub fn foo(_: bool) -> Result<(), Error> {
Ok(())
}
}

pub struct Error;

impl From<bar::Error> for Error {
fn from(_: bar::Error) -> Self {
Error
}
}

fn foo(ok: bool) -> Result<(), Error> {
if !ok {
return bar::foo(ok).map(|_| Ok::<(), Error>(()))?;
};
Ok(())
}
}
27 changes: 27 additions & 0 deletions tests/ui/needless_return_with_question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,30 @@ fn issue11616() -> Result<(), ()> {
};
Ok(())
}

/// This is a false positive that occurs because of the way `?` is handled.
/// The `?` operator is also doing a conversion from `Result<T, E>` to `Result<T, E'>`.
/// In this case the conversion is needed, and thus the `?` operator is also needed.
fn issue11982() {
mod bar {
pub struct Error;
pub fn foo(_: bool) -> Result<(), Error> {
Ok(())
}
}

pub struct Error;

impl From<bar::Error> for Error {
fn from(_: bar::Error) -> Self {
Error
}
}

fn foo(ok: bool) -> Result<(), Error> {
if !ok {
return bar::foo(ok).map(|_| Ok::<(), Error>(()))?;
};
Ok(())
}
}

0 comments on commit 57dd25e

Please sign in to comment.