From dc13016ac2df1ce2663660389409b15eb2cf7e40 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Wed, 3 Jun 2020 01:13:57 +0200 Subject: [PATCH 1/3] Make let_and_return a late lint pass --- clippy_lints/src/let_and_return.rs | 82 ++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 8 +-- clippy_lints/src/returns.rs | 82 ++---------------------------- src/lintlist/mod.rs | 2 +- 4 files changed, 91 insertions(+), 83 deletions(-) create mode 100644 clippy_lints/src/let_and_return.rs diff --git a/clippy_lints/src/let_and_return.rs b/clippy_lints/src/let_and_return.rs new file mode 100644 index 0000000000000..8b877f696afea --- /dev/null +++ b/clippy_lints/src/let_and_return.rs @@ -0,0 +1,82 @@ +use if_chain::if_chain; +use rustc_errors::Applicability; +use rustc_hir::{Block, ExprKind, PatKind, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +use crate::utils::{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); + 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"); + } + }, + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6f8923b266092..6bc9e23bac5dd 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 35464f629c364..3c93974417356 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 d5d07ccb2eb0d..bb191f9be92ca 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", From 9c205d7b1baa982ae7063d57b18088ecf28df83b Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sat, 6 Jun 2020 21:51:41 +0200 Subject: [PATCH 2/3] Rename let_and_return test for consistency with the lint name --- tests/ui/{let_return.rs => let_and_return.rs} | 0 tests/ui/{let_return.stderr => let_and_return.stderr} | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename tests/ui/{let_return.rs => let_and_return.rs} (100%) rename tests/ui/{let_return.stderr => let_and_return.stderr} (89%) diff --git a/tests/ui/let_return.rs b/tests/ui/let_and_return.rs similarity index 100% rename from tests/ui/let_return.rs rename to tests/ui/let_and_return.rs diff --git a/tests/ui/let_return.stderr b/tests/ui/let_and_return.stderr similarity index 89% rename from tests/ui/let_return.stderr rename to tests/ui/let_and_return.stderr index 128a22c86e360..eacf948b3927a 100644 --- a/tests/ui/let_return.stderr +++ b/tests/ui/let_and_return.stderr @@ -1,5 +1,5 @@ error: returning the result of a `let` binding from a block - --> $DIR/let_return.rs:7:5 + --> $DIR/let_and_return.rs:7:5 | LL | let x = 5; | ---------- unnecessary `let` binding @@ -14,7 +14,7 @@ LL | 5 | error: returning the result of a `let` binding from a block - --> $DIR/let_return.rs:13:9 + --> $DIR/let_and_return.rs:13:9 | LL | let x = 5; | ---------- unnecessary `let` binding From dac8a3c1ca19d2b5934ecbe2ed79ae6c156fd885 Mon Sep 17 00:00:00 2001 From: Eduardo Broto Date: Sun, 7 Jun 2020 00:30:39 +0200 Subject: [PATCH 3/3] let_and_return: do not lint if last statement borrows --- clippy_lints/src/let_and_return.rs | 61 ++++++++++++++++++++++++++- clippy_lints/src/utils/mod.rs | 2 +- tests/ui/let_and_return.rs | 68 ++++++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/let_and_return.rs b/clippy_lints/src/let_and_return.rs index 8b877f696afea..6d3fb317bcfc5 100644 --- a/clippy_lints/src/let_and_return.rs +++ b/clippy_lints/src/let_and_return.rs @@ -1,8 +1,12 @@ use if_chain::if_chain; use rustc_errors::Applicability; -use rustc_hir::{Block, ExprKind, PatKind, StmtKind}; +use rustc_hir::def_id::DefId; +use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; +use rustc_hir::{Block, Expr, ExprKind, PatKind, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::subst::GenericArgKind; use rustc_session::{declare_lint_pass, declare_tool_lint}; use crate::utils::{in_macro, match_qpath, snippet_opt, span_lint_and_then}; @@ -49,6 +53,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetReturn { if let PatKind::Binding(.., ident, _) = local.pat.kind; if let ExprKind::Path(qpath) = &retexpr.kind; if match_qpath(qpath, &[&*ident.name.as_str()]); + if !last_statement_borrows(cx, initexpr); if !in_external_macro(cx.sess(), initexpr.span); if !in_external_macro(cx.sess(), retexpr.span); if !in_external_macro(cx.sess(), local.span); @@ -80,3 +85,57 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetReturn { } } } + +fn last_statement_borrows<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + let mut visitor = BorrowVisitor { cx, borrows: false }; + walk_expr(&mut visitor, expr); + visitor.borrows +} + +struct BorrowVisitor<'a, 'tcx> { + cx: &'a LateContext<'a, 'tcx>, + borrows: bool, +} + +impl BorrowVisitor<'_, '_> { + fn fn_def_id(&self, expr: &Expr<'_>) -> Option { + match &expr.kind { + ExprKind::MethodCall(..) => self.cx.tables.type_dependent_def_id(expr.hir_id), + ExprKind::Call( + Expr { + kind: ExprKind::Path(qpath), + .. + }, + .., + ) => self.cx.tables.qpath_res(qpath, expr.hir_id).opt_def_id(), + _ => None, + } + } +} + +impl<'tcx> Visitor<'tcx> for BorrowVisitor<'_, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx Expr<'_>) { + if self.borrows { + return; + } + + if let Some(def_id) = self.fn_def_id(expr) { + self.borrows = self + .cx + .tcx + .fn_sig(def_id) + .output() + .skip_binder() + .walk() + .any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_))); + } + + walk_expr(self, expr); + } + + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } +} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 06638e7187bae..39410acea4e70 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -400,7 +400,7 @@ pub fn method_calls<'tcx>( /// Matches an `Expr` against a chain of methods, and return the matched `Expr`s. /// /// For example, if `expr` represents the `.baz()` in `foo.bar().baz()`, -/// `matched_method_chain(expr, &["bar", "baz"])` will return a `Vec` +/// `method_chain_args(expr, &["bar", "baz"])` will return a `Vec` /// containing the `Expr`s for /// `.bar()` and `.baz()` pub fn method_chain_args<'a>(expr: &'a Expr<'_>, methods: &[&str]) -> Option]>> { diff --git a/tests/ui/let_and_return.rs b/tests/ui/let_and_return.rs index 23645d48fe799..09614b8c1ad78 100644 --- a/tests/ui/let_and_return.rs +++ b/tests/ui/let_and_return.rs @@ -67,4 +67,72 @@ macro_rules! tuple_encode { tuple_encode!(T0, T1, T2, T3, T4, T5, T6, T7); +mod no_lint_if_stmt_borrows { + 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 + } + } + + mod issue_3324 { + use std::cell::RefCell; + use std::rc::{Rc, Weak}; + + fn test(value: Weak>) -> u32 { + let value = value.upgrade().unwrap(); + let ret = value.borrow().baz(); + ret + } + + struct Bar {} + + impl Bar { + fn new() -> Self { + Bar {} + } + fn baz(&self) -> u32 { + 0 + } + } + + fn main() { + let a = Rc::new(RefCell::new(Bar::new())); + let b = Rc::downgrade(&a); + test(b); + } + } + + mod free_function { + struct Inner; + + struct Foo<'a> { + inner: &'a Inner, + } + + impl Drop for Foo<'_> { + fn drop(&mut self) {} + } + + impl Foo<'_> { + fn value(&self) -> i32 { + 42 + } + } + + fn some_foo(inner: &Inner) -> Foo<'_> { + Foo { inner } + } + + fn test() -> i32 { + let x = Inner {}; + let value = some_foo(&x).value(); + value + } + } +} + fn main() {}