diff --git a/clippy_lints/src/let_and_return.rs b/clippy_lints/src/let_and_return.rs new file mode 100644 index 000000000000..3fc918aa1f7d --- /dev/null +++ b/clippy_lints/src/let_and_return.rs @@ -0,0 +1,171 @@ +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{ + AnonConst, Block, Expr, ExprKind, HirId, ImplItem, ImplItemKind, Item, ItemKind, Node, PatKind, StmtKind, TraitFn, + TraitItem, TraitItemKind, +}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::mir::{Body, Rvalue, Statement, StatementKind}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; + +use crate::utils::{get_enclosing_block, in_macro, match_qpath, snippet_opt, span_lint_and_then}; + +declare_clippy_lint! { + /// **What it does:** Checks for `let`-bindings, which are subsequently + /// returned. + /// + /// **Why is this bad?** It is just extraneous code. Remove it to make your code + /// more rusty. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// fn foo() -> String { + /// let x = String::new(); + /// x + /// } + /// ``` + /// instead, use + /// ``` + /// fn foo() -> String { + /// String::new() + /// } + /// ``` + pub LET_AND_RETURN, + style, + "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block" +} + +declare_lint_pass!(LetReturn => [LET_AND_RETURN]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetReturn { + fn check_block(&mut self, cx: &LateContext<'a, 'tcx>, block: &'tcx Block<'_>) { + // we need both a let-binding stmt and an expr + if_chain! { + if let Some(retexpr) = block.expr; + if let Some(stmt) = block.stmts.iter().last(); + if let StmtKind::Local(local) = &stmt.kind; + if local.ty.is_none(); + if local.attrs.is_empty(); + if let Some(initexpr) = &local.init; + if let PatKind::Binding(.., ident, _) = local.pat.kind; + if let ExprKind::Path(qpath) = &retexpr.kind; + if match_qpath(qpath, &[&*ident.name.as_str()]); + if !in_external_macro(cx.sess(), initexpr.span); + if !in_external_macro(cx.sess(), retexpr.span); + if !in_external_macro(cx.sess(), local.span); + if !in_macro(local.span); + if !last_statement_borrows_locals(cx, block); + then { + span_lint_and_then( + cx, + LET_AND_RETURN, + retexpr.span, + "returning the result of a `let` binding from a block", + |err| { + err.span_label(local.span, "unnecessary `let` binding"); + + if let Some(snippet) = snippet_opt(cx, initexpr.span) { + err.multipart_suggestion( + "return the expression directly", + vec![ + (local.span, String::new()), + (retexpr.span, snippet), + ], + Applicability::MachineApplicable, + ); + } else { + err.span_help(initexpr.span, "this expression can be directly returned"); + } + }, + ); + } + } + } +} + +// Check if we can suggest turning the last statement into a block tail expression without hitting +// "does not live long enough" errors. +fn last_statement_borrows_locals(cx: &LateContext<'_, '_>, block: &'_ Block<'_>) -> bool { + // Search for the enclosing fn-like node to retrieve the MIR for. + fn enclosing_node(cx: &LateContext<'_, '_>, hir_id: HirId) -> Option { + for (hir_id, node) in cx.tcx.hir().parent_iter(hir_id) { + match node { + Node::Expr(Expr { + kind: ExprKind::Closure(..), + .. + }) + | Node::Item(Item { + kind: ItemKind::Fn(..) | ItemKind::Static(..) | ItemKind::Const(..), + .. + }) + | Node::ImplItem(ImplItem { + kind: ImplItemKind::Fn(..) | ImplItemKind::Const(..), + .. + }) + | Node::TraitItem(TraitItem { + kind: TraitItemKind::Fn(.., TraitFn::Provided(_)) | TraitItemKind::Const(.., Some(_)), + .. + }) + | Node::AnonConst(AnonConst { .. }) => { + return Some(hir_id); + }, + _ => {}, + } + } + + None + } + + // Find the span of the outmost block where locals can't be borrowed. + fn scope_filter(cx: &LateContext<'_, '_>, block: &Block<'_>) -> Span { + fn is_parent_tail_expr(hir_id: HirId, parent: &Expr<'_>) -> bool { + matches!(parent.kind, ExprKind::Block(Block { hir_id: tail_id, .. }, _) if *tail_id == hir_id) + } + + let mut outer_block = block; + + while let Some(parent_block) = get_enclosing_block(cx, outer_block.hir_id) { + match parent_block.expr { + Some(tail_expr) if is_parent_tail_expr(outer_block.hir_id, tail_expr) => {}, + _ => break, + } + + outer_block = parent_block; + } + + outer_block.span + } + + // Search for `_2 = &_1` where _2 is a temporary and _1 is a local inside the relevant span. + fn is_relevant_assign(body: &Body<'_>, statement: &Statement<'_>, span: Span) -> bool { + if let StatementKind::Assign(box (assigned_place, Rvalue::Ref(_, _, borrowed_place))) = statement.kind { + let assigned = &body.local_decls[assigned_place.local]; + let borrowed = &body.local_decls[borrowed_place.local]; + + !assigned.is_user_variable() && borrowed.is_user_variable() && span.contains(borrowed.source_info.span) + } else { + false + } + } + + if let Some(last_stmt) = block.stmts.iter().last() { + if let Some(node_hir_id) = enclosing_node(cx, block.hir_id) { + let def_id = cx.tcx.hir().local_def_id(node_hir_id); + let body = cx.tcx.optimized_mir(def_id.to_def_id()); + let span = scope_filter(cx, block); + + return body.basic_blocks().iter().any(|bbdata| { + bbdata + .statements + .iter() + .any(|stmt| last_stmt.span.contains(stmt.source_info.span) && is_relevant_assign(body, stmt, span)) + }); + } + } + + false +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7e9a52fe69f3..0eeed4980678 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -239,6 +239,7 @@ mod large_const_arrays; mod large_enum_variant; mod large_stack_arrays; mod len_zero; +mod let_and_return; mod let_if_seq; mod let_underscore; mod lifetimes; @@ -596,6 +597,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &large_stack_arrays::LARGE_STACK_ARRAYS, &len_zero::LEN_WITHOUT_IS_EMPTY, &len_zero::LEN_ZERO, + &let_and_return::LET_AND_RETURN, &let_if_seq::USELESS_LET_IF_SEQ, &let_underscore::LET_UNDERSCORE_LOCK, &let_underscore::LET_UNDERSCORE_MUST_USE, @@ -772,7 +774,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ®ex::INVALID_REGEX, ®ex::REGEX_MACRO, ®ex::TRIVIAL_REGEX, - &returns::LET_AND_RETURN, &returns::NEEDLESS_RETURN, &returns::UNUSED_UNIT, &serde_api::SERDE_API_MISUSE, @@ -1022,6 +1023,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| box formatting::Formatting); store.register_early_pass(|| box misc_early::MiscEarlyLints); store.register_early_pass(|| box returns::Return); + store.register_late_pass(|| box let_and_return::LetReturn); store.register_early_pass(|| box collapsible_if::CollapsibleIf); store.register_early_pass(|| box items_after_statements::ItemsAfterStatements); store.register_early_pass(|| box precedence::Precedence); @@ -1265,6 +1267,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&large_enum_variant::LARGE_ENUM_VARIANT), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), + LintId::of(&let_and_return::LET_AND_RETURN), LintId::of(&let_underscore::LET_UNDERSCORE_LOCK), LintId::of(&lifetimes::EXTRA_UNUSED_LIFETIMES), LintId::of(&lifetimes::NEEDLESS_LIFETIMES), @@ -1390,7 +1393,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(®ex::INVALID_REGEX), LintId::of(®ex::REGEX_MACRO), LintId::of(®ex::TRIVIAL_REGEX), - LintId::of(&returns::LET_AND_RETURN), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), LintId::of(&serde_api::SERDE_API_MISUSE), @@ -1474,6 +1476,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&inherent_to_string::INHERENT_TO_STRING), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), + LintId::of(&let_and_return::LET_AND_RETURN), LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING), LintId::of(&loops::EMPTY_LOOP), LintId::of(&loops::FOR_KV_MAP), @@ -1526,7 +1529,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES), LintId::of(®ex::REGEX_MACRO), LintId::of(®ex::TRIVIAL_REGEX), - LintId::of(&returns::LET_AND_RETURN), LintId::of(&returns::NEEDLESS_RETURN), LintId::of(&returns::UNUSED_UNIT), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 35464f629c36..3c9397441735 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -8,7 +8,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use rustc_span::BytePos; -use crate::utils::{in_macro, match_path_ast, snippet_opt, span_lint_and_sugg, span_lint_and_then}; +use crate::utils::{snippet_opt, span_lint_and_sugg, span_lint_and_then}; declare_clippy_lint! { /// **What it does:** Checks for return statements at the end of a block. @@ -36,33 +36,6 @@ declare_clippy_lint! { "using a return statement like `return expr;` where an expression would suffice" } -declare_clippy_lint! { - /// **What it does:** Checks for `let`-bindings, which are subsequently - /// returned. - /// - /// **Why is this bad?** It is just extraneous code. Remove it to make your code - /// more rusty. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// fn foo() -> String { - /// let x = String::new(); - /// x - /// } - /// ``` - /// instead, use - /// ``` - /// fn foo() -> String { - /// String::new() - /// } - /// ``` - pub LET_AND_RETURN, - style, - "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block" -} - declare_clippy_lint! { /// **What it does:** Checks for unit (`()`) expressions that can be removed. /// @@ -90,7 +63,7 @@ enum RetReplacement { Block, } -declare_lint_pass!(Return => [NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT]); +declare_lint_pass!(Return => [NEEDLESS_RETURN, UNUSED_UNIT]); impl Return { // Check the final stmt or expr in a block for unnecessary return. @@ -105,7 +78,7 @@ impl Return { } } - // Check a the final expression in a block if it's a return. + // Check the final expression in a block if it's a return. fn check_final_expr( &mut self, cx: &EarlyContext<'_>, @@ -186,54 +159,6 @@ impl Return { }, } } - - // Check for "let x = EXPR; x" - fn check_let_return(cx: &EarlyContext<'_>, block: &ast::Block) { - let mut it = block.stmts.iter(); - - // we need both a let-binding stmt and an expr - if_chain! { - if let Some(retexpr) = it.next_back(); - if let ast::StmtKind::Expr(ref retexpr) = retexpr.kind; - if let Some(stmt) = it.next_back(); - if let ast::StmtKind::Local(ref local) = stmt.kind; - // don't lint in the presence of type inference - if local.ty.is_none(); - if local.attrs.is_empty(); - if let Some(ref initexpr) = local.init; - if let ast::PatKind::Ident(_, ident, _) = local.pat.kind; - if let ast::ExprKind::Path(_, ref path) = retexpr.kind; - if match_path_ast(path, &[&*ident.name.as_str()]); - if !in_external_macro(cx.sess(), initexpr.span); - if !in_external_macro(cx.sess(), retexpr.span); - if !in_external_macro(cx.sess(), local.span); - if !in_macro(local.span); - then { - span_lint_and_then( - cx, - LET_AND_RETURN, - retexpr.span, - "returning the result of a `let` binding from a block", - |err| { - err.span_label(local.span, "unnecessary `let` binding"); - - if let Some(snippet) = snippet_opt(cx, initexpr.span) { - err.multipart_suggestion( - "return the expression directly", - vec![ - (local.span, String::new()), - (retexpr.span, snippet), - ], - Applicability::MachineApplicable, - ); - } else { - err.span_help(initexpr.span, "this expression can be directly returned"); - } - }, - ); - } - } - } } impl EarlyLintPass for Return { @@ -254,7 +179,6 @@ impl EarlyLintPass for Return { } fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) { - Self::check_let_return(cx, block); if_chain! { if let Some(ref stmt) = block.stmts.last(); if let ast::StmtKind::Expr(ref expr) = stmt.kind; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index d5d07ccb2eb0..bb191f9be92c 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1023,7 +1023,7 @@ pub static ref ALL_LINTS: Vec = vec![ group: "style", desc: "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block", deprecation: None, - module: "returns", + module: "let_and_return", }, Lint { name: "let_underscore_lock", diff --git a/tests/ui/let_return.rs b/tests/ui/let_return.rs index 23645d48fe79..3c30a45689a8 100644 --- a/tests/ui/let_return.rs +++ b/tests/ui/let_return.rs @@ -67,4 +67,62 @@ macro_rules! tuple_encode { tuple_encode!(T0, T1, T2, T3, T4, T5, T6, T7); +// Avoid linting if the suggestion would cause a "does not live long enough" error. +// Usually temporaries are destroyed at the end of the statement, but in the case of +// a block tail expression, they are destroyed after the block locals, so code that works +// fine will trigger a compiler error after applying the suggestion. +mod issue_3792 { + use std::io::{self, BufRead, Stdin}; + + fn read_line() -> String { + let stdin = io::stdin(); + let line = stdin.lock().lines().next().unwrap().unwrap(); + line + } + + // Nested blocks "carry" where temporaries are destroyed... + fn nested_and_local_in_outer_scope() { + let _ = { + { + let stdin = io::stdin(); + { + let line = stdin.lock().lines().next().unwrap().unwrap(); + line + } + } + }; // <- temporaries would be destroyed here + } + + fn nested_and_local_in_outer_scope_2() { + let _ = { + let stdin = io::stdin(); + { + { + let line = stdin.lock().lines().next().unwrap().unwrap(); + line + } + } + }; + } + + // This one should be linted because `stdin` outlives the temporaries that borrow it. + fn block_tail_but_local_outlives() { + let stdin = io::stdin(); + let _ = { + { + { + let line = stdin.lock().lines().next().unwrap().unwrap(); + line + } + } + }; + } + + // This one should be linted too as parameters are destroyed last. + fn function_parameters(stdin: Stdin) -> String { + let line = stdin.lock().lines().next().unwrap().unwrap(); + line + } +} + fn main() {} diff --git a/tests/ui/let_return.stderr b/tests/ui/let_return.stderr index 128a22c86e36..b91b292a106d 100644 --- a/tests/ui/let_return.stderr +++ b/tests/ui/let_return.stderr @@ -27,5 +27,33 @@ LL | LL | 5 | -error: aborting due to 2 previous errors +error: returning the result of a `let` binding from a block + --> $DIR/let_return.rs:115:21 + | +LL | let line = stdin.lock().lines().next().unwrap().unwrap(); + | --------------------------------------------------------- unnecessary `let` binding +LL | line + | ^^^^ + | +help: return the expression directly + | +LL | +LL | stdin.lock().lines().next().unwrap().unwrap() + | + +error: returning the result of a `let` binding from a block + --> $DIR/let_return.rs:124:9 + | +LL | let line = stdin.lock().lines().next().unwrap().unwrap(); + | --------------------------------------------------------- unnecessary `let` binding +LL | line + | ^^^^ + | +help: return the expression directly + | +LL | +LL | stdin.lock().lines().next().unwrap().unwrap() + | + +error: aborting due to 4 previous errors