diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 84bcef856d06..f0d7dd23a678 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 c77554fb47b2..0f525dd294c9 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 8fed64ac6e3b..a1db8375d95b 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 18edbce2f44b..87d0cd3e14cf 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