Skip to content

Commit

Permalink
[question_mark]: also trigger on return statements
Browse files Browse the repository at this point in the history
  • Loading branch information
y21 committed Dec 23, 2023
1 parent dd857f8 commit dfb4ff8
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 15 deletions.
4 changes: 1 addition & 3 deletions clippy_lints/src/matches/manual_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@ where
return None;
}

let Some(some_expr) = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) else {
return None;
};
let some_expr = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt)?;

// These two lints will go back and forth with each other.
if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit
Expand Down
4 changes: 1 addition & 3 deletions clippy_lints/src/matches/redundant_pattern_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ fn find_method_and_type<'tcx>(

if is_wildcard || is_rest {
let res = cx.typeck_results().qpath_res(qpath, check_pat.hir_id);
let Some(id) = res.opt_def_id().map(|ctor_id| cx.tcx.parent(ctor_id)) else {
return None;
};
let id = res.opt_def_id().map(|ctor_id| cx.tcx.parent(ctor_id))?;
let lang_items = cx.tcx.lang_items();
if Some(id) == lang_items.result_ok_variant() {
Some(("is_ok()", try_get_generic_ty(op_ty, 0).unwrap_or(op_ty)))
Expand Down
24 changes: 19 additions & 5 deletions clippy_lints/src/question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,18 +96,32 @@ enum IfBlockType<'hir> {
),
}

fn find_let_else_ret_expression<'hir>(block: &'hir Block<'hir>) -> Option<&'hir Expr<'hir>> {
if let Block {
stmts: &[],
expr: Some(els),
..
} = block
{
Some(els)
} else if let [stmt] = block.stmts
&& let StmtKind::Semi(expr) = stmt.kind
&& let ExprKind::Ret(..) = expr.kind
{
Some(expr)
} else {
None
}
}

fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
if let StmtKind::Local(Local {
pat,
init: Some(init_expr),
els: Some(els),
..
}) = stmt.kind
&& let Block {
stmts: &[],
expr: Some(els),
..
} = els
&& let Some(els) = find_let_else_ret_expression(els)
&& let Some(inner_pat) = pat_and_expr_can_be_question_mark(cx, pat, els)
{
let mut applicability = Applicability::MaybeIncorrect;
Expand Down
4 changes: 1 addition & 3 deletions clippy_lints/src/undocumented_unsafe_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -680,9 +680,7 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
})
.filter(|(_, text)| !text.is_empty());

let Some((line_start, line)) = lines.next() else {
return None;
};
let (line_start, line) = lines.next()?;
// Check for a sequence of line comments.
if line.starts_with("//") {
let (mut line, mut line_start) = (line, line_start);
Expand Down
13 changes: 13 additions & 0 deletions tests/ui/manual_let_else_question_mark.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,16 @@ fn foo() -> Option<()> {

Some(())
}

// lint not just `return None`, but also `return None;` (note the semicolon)
fn issue11993(y: Option<i32>) -> Option<i32> {
let x = y?;

// don't lint: more than one statement in the else body
let Some(x) = y else {
todo!();
return None;
};

None
}
15 changes: 15 additions & 0 deletions tests/ui/manual_let_else_question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,18 @@ fn foo() -> Option<()> {

Some(())
}

// lint not just `return None`, but also `return None;` (note the semicolon)
fn issue11993(y: Option<i32>) -> Option<i32> {
let Some(x) = y else {
return None;
};

// don't lint: more than one statement in the else body
let Some(x) = y else {
todo!();
return None;
};

None
}
10 changes: 9 additions & 1 deletion tests/ui/manual_let_else_question_mark.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,13 @@ error: this could be rewritten as `let...else`
LL | let v = if let Some(v_some) = g() { v_some } else { return None };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return None };`

error: aborting due to 6 previous errors
error: this `let...else` may be rewritten with the `?` operator
--> $DIR/manual_let_else_question_mark.rs:71:5
|
LL | / let Some(x) = y else {
LL | | return None;
LL | | };
| |______^ help: replace it with: `let x = y?;`

error: aborting due to 7 previous errors

0 comments on commit dfb4ff8

Please sign in to comment.