From 8e96adedd56993eec0609276124ff17d4866b94b Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 14 Feb 2023 11:31:42 +0800 Subject: [PATCH 1/2] fix [`needless_return`] incorrect suggestion when returning if sequence --- clippy_lints/src/returns.rs | 96 +++++++++++++++++++-------------- tests/ui/needless_return.fixed | 4 ++ tests/ui/needless_return.rs | 4 ++ tests/ui/needless_return.stderr | 10 +++- 4 files changed, 72 insertions(+), 42 deletions(-) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 84a0c6b955853..84bcef856d06c 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -69,31 +69,35 @@ declare_clippy_lint! { "using a return statement like `return expr;` where an expression would suffice" } -#[derive(PartialEq, Eq, Copy, Clone)] +#[derive(PartialEq, Eq, Clone)] enum RetReplacement { Empty, Block, Unit, + IfSequence(String), + Expr(String), } impl RetReplacement { fn sugg_help(self) -> &'static str { match self { - Self::Empty => "remove `return`", + Self::Empty | Self::Expr(_) => "remove `return`", Self::Block => "replace `return` with an empty block", Self::Unit => "replace `return` with a unit value", + Self::IfSequence(_) => "remove `return` and wrap the sequence with parentheses", } } } impl ToString for RetReplacement { fn to_string(&self) -> String { - match *self { - Self::Empty => "", - Self::Block => "{}", - Self::Unit => "()", + match self { + Self::Empty => String::new(), + Self::Block => "{}".to_string(), + Self::Unit => "()".to_string(), + Self::IfSequence(inner) => format!("({inner})"), + Self::Expr(inner) => inner.clone(), } - .to_string() } } @@ -210,13 +214,37 @@ fn check_final_expr<'tcx>( match &peeled_drop_expr.kind { // simple return is always "bad" ExprKind::Ret(ref inner) => { - // if desugar of `do yeet`, don't lint - if let Some(inner_expr) = inner - && let ExprKind::Call(path_expr, _) = inner_expr.kind - && let ExprKind::Path(QPath::LangItem(LangItem::TryTraitFromYeet, _, _)) = path_expr.kind - { - return; - } + // check if expr return nothing + let ret_span = if inner.is_none() && replacement == RetReplacement::Empty { + extend_span_to_previous_non_ws(cx, peeled_drop_expr.span) + } else { + peeled_drop_expr.span + }; + + let replacement = if let Some(inner_expr) = inner { + // if desugar of `do yeet`, don't lint + if let ExprKind::Call(path_expr, _) = inner_expr.kind + && let ExprKind::Path(QPath::LangItem(LangItem::TryTraitFromYeet, _, _)) = path_expr.kind + { + return; + } + + let (snippet, _) = snippet_with_context( + cx, + inner_expr.span, + ret_span.ctxt(), + "..", + &mut Applicability::MachineApplicable, + ); + if expr_contains_if(inner_expr) { + RetReplacement::IfSequence(snippet.to_string()) + } else { + RetReplacement::Expr(snippet.to_string()) + } + } else { + replacement + }; + if !cx.tcx.hir().attrs(expr.hir_id).is_empty() { return; } @@ -224,14 +252,8 @@ fn check_final_expr<'tcx>( if borrows { return; } - // check if expr return nothing - let ret_span = if inner.is_none() && replacement == RetReplacement::Empty { - extend_span_to_previous_non_ws(cx, peeled_drop_expr.span) - } else { - peeled_drop_expr.span - }; - emit_return_lint(cx, ret_span, semi_spans, inner.as_ref().map(|i| i.span), replacement); + emit_return_lint(cx, ret_span, semi_spans, replacement); }, ExprKind::If(_, then, else_clause_opt) => { check_block_return(cx, &then.kind, peeled_drop_expr.span, semi_spans.clone()); @@ -253,29 +275,21 @@ fn check_final_expr<'tcx>( } } -fn emit_return_lint( - cx: &LateContext<'_>, - ret_span: Span, - semi_spans: Vec, - inner_span: Option, - replacement: RetReplacement, -) { +fn expr_contains_if<'tcx>(expr: &'tcx Expr<'tcx>) -> bool { + match expr.kind { + ExprKind::If(..) => true, + ExprKind::Binary(_, left, right) => expr_contains_if(left) || expr_contains_if(right), + _ => false, + } +} + +fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec, replacement: RetReplacement) { if ret_span.from_expansion() { return; } - let mut applicability = Applicability::MachineApplicable; - let return_replacement = inner_span.map_or_else( - || replacement.to_string(), - |inner_span| { - let (snippet, _) = snippet_with_context(cx, inner_span, ret_span.ctxt(), "..", &mut applicability); - snippet.to_string() - }, - ); - let sugg_help = if inner_span.is_some() { - "remove `return`" - } else { - replacement.sugg_help() - }; + let applicability = Applicability::MachineApplicable; + let return_replacement = replacement.to_string(); + let sugg_help = replacement.sugg_help(); span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| { diag.span_suggestion_hidden(ret_span, sugg_help, return_replacement, applicability); // for each parent statement, we need to remove the semicolon diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index 079e3531def1b..c77554fb47b25 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -297,4 +297,8 @@ fn issue10051() -> Result { } } +fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 { + (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }) +} + fn main() {} diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index c1c48284f0869..8fed64ac6e3b4 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -307,4 +307,8 @@ fn issue10051() -> Result { } } +fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 { + return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; +} + fn main() {} diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 08b04bfe9d8bf..18edbce2f44ba 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -418,5 +418,13 @@ LL | return Err(format!("err!")); | = help: remove `return` -error: aborting due to 50 previous errors +error: unneeded `return` statement + --> $DIR/needless_return.rs:311:5 + | +LL | return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` and wrap the sequence with parentheses + +error: aborting due to 51 previous errors From 8b93eb8a9b31fcaadf22ea0fbacfcea4450a5894 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 15 Feb 2023 11:26:30 +0800 Subject: [PATCH 2/2] add some adjustment regarding review suggestion --- clippy_lints/src/returns.rs | 60 ++++++++++++++++++--------------- tests/ui/needless_return.fixed | 10 ++++-- tests/ui/needless_return.rs | 10 ++++-- tests/ui/needless_return.stderr | 16 ++++++--- 4 files changed, 61 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 84bcef856d06c..f0d7dd23a6786 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -14,6 +14,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::def_id::LocalDefId; use rustc_span::source_map::Span; use rustc_span::{BytePos, Pos}; +use std::borrow::Cow; declare_clippy_lint! { /// ### What it does @@ -70,33 +71,39 @@ declare_clippy_lint! { } #[derive(PartialEq, Eq, Clone)] -enum RetReplacement { +enum RetReplacement<'tcx> { Empty, Block, Unit, - IfSequence(String), - Expr(String), + IfSequence(Cow<'tcx, str>, Applicability), + Expr(Cow<'tcx, str>, Applicability), } -impl RetReplacement { +impl<'tcx> RetReplacement<'tcx> { fn sugg_help(self) -> &'static str { match self { - Self::Empty | Self::Expr(_) => "remove `return`", + Self::Empty | Self::Expr(..) => "remove `return`", Self::Block => "replace `return` with an empty block", Self::Unit => "replace `return` with a unit value", - Self::IfSequence(_) => "remove `return` and wrap the sequence with parentheses", + Self::IfSequence(..) => "remove `return` and wrap the sequence with parentheses", + } + } + fn applicability(&self) -> Option { + match self { + Self::Expr(_, ap) | Self::IfSequence(_, ap) => Some(*ap), + _ => None, } } } -impl ToString for RetReplacement { +impl<'tcx> ToString for RetReplacement<'tcx> { fn to_string(&self) -> String { match self { Self::Empty => String::new(), Self::Block => "{}".to_string(), Self::Unit => "()".to_string(), - Self::IfSequence(inner) => format!("({inner})"), - Self::Expr(inner) => inner.clone(), + Self::IfSequence(inner, _) => format!("({inner})"), + Self::Expr(inner, _) => inner.to_string(), } } } @@ -208,7 +215,7 @@ fn check_final_expr<'tcx>( expr: &'tcx Expr<'tcx>, semi_spans: Vec, /* containing all the places where we would need to remove semicolons if finding an * needless return */ - replacement: RetReplacement, + replacement: RetReplacement<'tcx>, ) { let peeled_drop_expr = expr.peel_drop_temps(); match &peeled_drop_expr.kind { @@ -229,17 +236,12 @@ fn check_final_expr<'tcx>( return; } - let (snippet, _) = snippet_with_context( - cx, - inner_expr.span, - ret_span.ctxt(), - "..", - &mut Applicability::MachineApplicable, - ); - if expr_contains_if(inner_expr) { - RetReplacement::IfSequence(snippet.to_string()) + let mut applicability = Applicability::MachineApplicable; + let (snippet, _) = snippet_with_context(cx, inner_expr.span, ret_span.ctxt(), "..", &mut applicability); + if expr_contains_conjunctive_ifs(inner_expr) { + RetReplacement::IfSequence(snippet, applicability) } else { - RetReplacement::Expr(snippet.to_string()) + RetReplacement::Expr(snippet, applicability) } } else { replacement @@ -275,19 +277,23 @@ fn check_final_expr<'tcx>( } } -fn expr_contains_if<'tcx>(expr: &'tcx Expr<'tcx>) -> bool { - match expr.kind { - ExprKind::If(..) => true, - ExprKind::Binary(_, left, right) => expr_contains_if(left) || expr_contains_if(right), - _ => false, +fn expr_contains_conjunctive_ifs<'tcx>(expr: &'tcx Expr<'tcx>) -> bool { + fn contains_if(expr: &Expr<'_>, on_if: bool) -> bool { + match expr.kind { + ExprKind::If(..) => on_if, + ExprKind::Binary(_, left, right) => contains_if(left, true) || contains_if(right, true), + _ => false, + } } + + contains_if(expr, false) } -fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec, replacement: RetReplacement) { +fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, semi_spans: Vec, replacement: RetReplacement<'_>) { if ret_span.from_expansion() { return; } - let applicability = Applicability::MachineApplicable; + let applicability = replacement.applicability().unwrap_or(Applicability::MachineApplicable); let return_replacement = replacement.to_string(); let sugg_help = replacement.sugg_help(); span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| { diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index c77554fb47b25..0f525dd294c9b 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -297,8 +297,14 @@ fn issue10051() -> Result { } } -fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 { - (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }) +mod issue10049 { + fn single() -> u32 { + if true { 1 } else { 2 } + } + + fn multiple(b1: bool, b2: bool, b3: bool) -> u32 { + (if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }) + } } fn main() {} diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index 8fed64ac6e3b4..a1db8375d95b9 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -307,8 +307,14 @@ fn issue10051() -> Result { } } -fn issue10049(b1: bool, b2: bool, b3: bool) -> u32 { - return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; +mod issue10049 { + fn single() -> u32 { + return if true { 1 } else { 2 }; + } + + fn multiple(b1: bool, b2: bool, b3: bool) -> u32 { + return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; + } } fn main() {} diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index 18edbce2f44ba..87d0cd3e14cfa 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -419,12 +419,20 @@ LL | return Err(format!("err!")); = help: remove `return` error: unneeded `return` statement - --> $DIR/needless_return.rs:311:5 + --> $DIR/needless_return.rs:312:9 | -LL | return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | return if true { 1 } else { 2 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove `return` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:316:9 + | +LL | return if b1 { 0 } else { 1 } | if b2 { 2 } else { 3 } | if b3 { 4 } else { 5 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: remove `return` and wrap the sequence with parentheses -error: aborting due to 51 previous errors +error: aborting due to 52 previous errors