Skip to content

Commit

Permalink
Auto merge of #11802 - dswij:issue-11765, r=xFrednet
Browse files Browse the repository at this point in the history
`needless_return_with_question_mark` ignore let-else

Fixes #11765

This PR makes `needless_return_with_question_mark` to ignore expr inside let-else.

changelog: [`needless_return_with_question_mark`] ignore let-else
  • Loading branch information
bors committed Nov 15, 2023
2 parents 662ea3f + 48f38eb commit 7ad3373
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 2 deletions.
3 changes: 2 additions & 1 deletion clippy_lints/src/returns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then, span_lin
use clippy_utils::source::{snippet_opt, snippet_with_context};
use clippy_utils::sugg::has_enclosing_paren;
use clippy_utils::visitors::{for_each_expr_with_closures, Descend};
use clippy_utils::{fn_def_id, is_from_proc_macro, path_to_local_id, span_find_starting_semi};
use clippy_utils::{fn_def_id, is_from_proc_macro, is_inside_let_else, path_to_local_id, span_find_starting_semi};
use core::ops::ControlFlow;
use rustc_errors::Applicability;
use rustc_hir::intravisit::FnKind;
Expand Down Expand Up @@ -170,6 +170,7 @@ impl<'tcx> LateLintPass<'tcx> for Return {
&& let ItemKind::Fn(_, _, body) = item.kind
&& let block = cx.tcx.hir().body(body).value
&& let ExprKind::Block(block, _) = block.kind
&& !is_inside_let_else(cx.tcx, expr)
&& let [.., final_stmt] = block.stmts
&& final_stmt.hir_id != stmt.hir_id
&& !is_from_proc_macro(cx, expr)
Expand Down
37 changes: 37 additions & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1487,6 +1487,43 @@ pub fn is_else_clause(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
}
}

/// Checks if the given expression is a part of `let else`
/// returns `true` for both the `init` and the `else` part
pub fn is_inside_let_else(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
let mut child_id = expr.hir_id;
for (parent_id, node) in tcx.hir().parent_iter(child_id) {
if let Node::Local(Local {
init: Some(init),
els: Some(els),
..
}) = node
&& (init.hir_id == child_id || els.hir_id == child_id)
{
return true;
}

child_id = parent_id;
}

false
}

/// Checks if the given expression is the else clause of a `let else` expression
pub fn is_else_clause_in_let_else(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool {
let mut child_id = expr.hir_id;
for (parent_id, node) in tcx.hir().parent_iter(child_id) {
if let Node::Local(Local { els: Some(els), .. }) = node
&& els.hir_id == child_id
{
return true;
}

child_id = parent_id;
}

false
}

/// Checks whether the given `Expr` is a range equivalent to a `RangeFull`.
/// For the lower bound, this means that:
/// - either there is none
Expand Down
22 changes: 22 additions & 0 deletions tests/ui/needless_return_with_question_mark.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
clippy::no_effect,
clippy::unit_arg,
clippy::useless_conversion,
clippy::diverging_sub_expression,
unused
)]

Expand Down Expand Up @@ -35,5 +36,26 @@ fn main() -> Result<(), ()> {
with_span! {
return Err(())?;
}

// Issue #11765
// Should not lint
let Some(s) = Some("") else {
return Err(())?;
};

let Some(s) = Some("") else {
let Some(s) = Some("") else {
return Err(())?;
};

return Err(())?;
};

let Some(_): Option<()> = ({
return Err(())?;
}) else {
panic!();
};

Err(())
}
22 changes: 22 additions & 0 deletions tests/ui/needless_return_with_question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
clippy::no_effect,
clippy::unit_arg,
clippy::useless_conversion,
clippy::diverging_sub_expression,
unused
)]

Expand Down Expand Up @@ -35,5 +36,26 @@ fn main() -> Result<(), ()> {
with_span! {
return Err(())?;
}

// Issue #11765
// Should not lint
let Some(s) = Some("") else {
return Err(())?;
};

let Some(s) = Some("") else {
let Some(s) = Some("") else {
return Err(())?;
};

return Err(())?;
};

let Some(_): Option<()> = ({
return Err(())?;
}) else {
panic!();
};

Err(())
}
2 changes: 1 addition & 1 deletion tests/ui/needless_return_with_question_mark.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: unneeded `return` statement with `?` operator
--> $DIR/needless_return_with_question_mark.rs:27:5
--> $DIR/needless_return_with_question_mark.rs:28:5
|
LL | return Err(())?;
| ^^^^^^^ help: remove it
Expand Down

0 comments on commit 7ad3373

Please sign in to comment.