Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(needless_return): do not trigger on ambiguous match arms return #10593

Merged
merged 3 commits into from
Apr 10, 2023
Merged

fix(needless_return): do not trigger on ambiguous match arms return #10593

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c12748f
fix(needless_return): do not trigger on ambiguous match arms return
feniljain Apr 1, 2023
b499b7d
test: add test for match as stmt no triggering needless_return
feniljain Apr 5, 2023
9cf57d0
fix(needles_return): correct span selection for text replacement
feniljain Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions clippy_lints/src/returns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_hir::intravisit::FnKind;
use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, LangItem, MatchSource, PatKind, QPath, StmtKind};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::subst::GenericArgKind;
use rustc_middle::ty::{self, subst::GenericArgKind, Ty};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::def_id::LocalDefId;
use rustc_span::source_map::Span;
Expand Down Expand Up @@ -175,7 +175,7 @@ impl<'tcx> LateLintPass<'tcx> for Return {
} else {
RetReplacement::Empty
};
check_final_expr(cx, body.value, vec![], replacement);
check_final_expr(cx, body.value, vec![], replacement, None);
},
FnKind::ItemFn(..) | FnKind::Method(..) => {
check_block_return(cx, &body.value.kind, sp, vec![]);
Expand All @@ -188,11 +188,11 @@ impl<'tcx> LateLintPass<'tcx> for Return {
fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>, sp: Span, mut semi_spans: Vec<Span>) {
if let ExprKind::Block(block, _) = expr_kind {
if let Some(block_expr) = block.expr {
check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty);
check_final_expr(cx, block_expr, semi_spans, RetReplacement::Empty, None);
} else if let Some(stmt) = block.stmts.iter().last() {
match stmt.kind {
StmtKind::Expr(expr) => {
check_final_expr(cx, expr, semi_spans, RetReplacement::Empty);
check_final_expr(cx, expr, semi_spans, RetReplacement::Empty, None);
},
StmtKind::Semi(semi_expr) => {
// Remove ending semicolons and any whitespace ' ' in between.
Expand All @@ -202,7 +202,7 @@ fn check_block_return<'tcx>(cx: &LateContext<'tcx>, expr_kind: &ExprKind<'tcx>,
span_find_starting_semi(cx.sess().source_map(), semi_span.with_hi(sp.hi()));
semi_spans.push(semi_span_to_remove);
}
check_final_expr(cx, semi_expr, semi_spans, RetReplacement::Empty);
check_final_expr(cx, semi_expr, semi_spans, RetReplacement::Empty, None);
},
_ => (),
}
Expand All @@ -216,6 +216,7 @@ fn check_final_expr<'tcx>(
semi_spans: Vec<Span>, /* containing all the places where we would need to remove semicolons if finding an
* needless return */
replacement: RetReplacement<'tcx>,
match_ty_opt: Option<Ty<'_>>,
) {
let peeled_drop_expr = expr.peel_drop_temps();
match &peeled_drop_expr.kind {
Expand Down Expand Up @@ -244,7 +245,22 @@ fn check_final_expr<'tcx>(
RetReplacement::Expr(snippet, applicability)
}
} else {
replacement
match match_ty_opt {
Some(match_ty) => {
match match_ty.kind() {
// If the code got till here with
// tuple not getting detected before it,
// then we are sure it's going to be Unit
// type
ty::Tuple(_) => RetReplacement::Unit,
// We don't want to anything in this case
// cause we can't predict what the user would
// want here
_ => return,
}
},
None => replacement,
}
};

if !cx.tcx.hir().attrs(expr.hir_id).is_empty() {
Expand All @@ -268,8 +284,9 @@ fn check_final_expr<'tcx>(
// note, if without else is going to be a type checking error anyways
// (except for unit type functions) so we don't match it
ExprKind::Match(_, arms, MatchSource::Normal) => {
let match_ty = cx.typeck_results().expr_ty(peeled_drop_expr);
for arm in arms.iter() {
check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Unit);
check_final_expr(cx, arm.body, semi_spans.clone(), RetReplacement::Empty, Some(match_ty));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be mistaken, but I feel like this is not quite the right fix for the issue. The problem, as I understand it, is that the return value has a different type than the other match arms. From my understanding, this can only happen, if the match is at the end of the function and has a semicolon, making it a statement. This therefore also implies that the function returns the unit type (), as otherwise there would be something after the statement or the types would be compatible.

fn test_match_as_stmt() {
    let x = 9;
    match x {
        1 => 2,
        _ => return,
    }; // The semicolon makes it a statement, and allows the branches to be non-unit
    // This makes the branches incompatible
}

fn does_not_compile() {
    let x = 9;
    match x {
        1 => 2,
        _ => return,
    }
    // The first branch is incompatible with the function signature.
}

fn does_compile_because_types_are_compatible() -> u32 {
    let x = 9;
    match x {
        1 => 2,
        _ => return 3,
    }
    // Should be linted since the types are compatible
}

(Maybe there are some other edge cases with impl Trait, that might be worth testing)

Based on this, I think the proper fix would be to check the type of the match statement. If it's () the types are compatible. If not and the return has a value than they would still be compatible. It would only cause problems, if the match type is != () while the function is -> ().

Does this make sense?

Copy link
Contributor Author

@feniljain feniljain Apr 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Thanks for the awesome in-depth review

I think the proper fix would be to check the type of the match statement

That's what I am doing currently in the function check_final_expr here:
https://github.com/feniljain/rust-clippy/blob/c12748fab3a14beae4958f13551e7e6c52298490/clippy_lints/src/returns.rs#L250-L260

😅

If not and the return has a value than they would still be compatible.

We would not reach this part of code ( one I linked just above ) if the return is not empty, in which case we just tell clippy to ignore it, at least that's what we discussed in zulip here

Do help me if I am still misunderstanding things 🙇🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh okay, if that part is never reached, then it should be fine, that slipped my radar, during my first review. :)

}
},
// if it's a whole block, check it
Expand All @@ -293,6 +310,7 @@ fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec<Span>,
if ret_span.from_expansion() {
return;
}

let applicability = replacement.applicability().unwrap_or(Applicability::MachineApplicable);
let return_replacement = replacement.to_string();
let sugg_help = replacement.sugg_help();
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/needless_return.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ fn test_void_if_fun(b: bool) {
fn test_void_match(x: u32) {
feniljain marked this conversation as resolved.
Show resolved Hide resolved
match x {
0 => (),
_ => (),
_ =>(),
}
}

Expand All @@ -91,7 +91,7 @@ fn test_nested_match(x: u32) {
1 => {
let _ = 42;
},
_ => (),
_ =>(),
}
}

Expand Down Expand Up @@ -196,7 +196,7 @@ async fn async_test_void_if_fun(b: bool) {
async fn async_test_void_match(x: u32) {
match x {
0 => (),
_ => (),
_ =>(),
}
}

Expand Down
12 changes: 6 additions & 6 deletions tests/ui/needless_return.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,10 @@ LL | | return;
= help: remove `return`

error: unneeded `return` statement
--> $DIR/needless_return.rs:87:14
--> $DIR/needless_return.rs:87:13
|
LL | _ => return,
| ^^^^^^
| ^^^^^^^
|
= help: replace `return` with a unit value

Expand All @@ -136,10 +136,10 @@ LL | | return;
= help: remove `return`

error: unneeded `return` statement
--> $DIR/needless_return.rs:98:14
--> $DIR/needless_return.rs:98:13
|
LL | _ => return,
| ^^^^^^
feniljain marked this conversation as resolved.
Show resolved Hide resolved
| ^^^^^^^
|
= help: replace `return` with a unit value

Expand Down Expand Up @@ -296,10 +296,10 @@ LL | | return;
= help: remove `return`

error: unneeded `return` statement
--> $DIR/needless_return.rs:207:14
--> $DIR/needless_return.rs:207:13
|
LL | _ => return,
| ^^^^^^
| ^^^^^^^
|
= help: replace `return` with a unit value

Expand Down