diff --git a/clippy_lints/src/let_and_return.rs b/clippy_lints/src/let_and_return.rs deleted file mode 100644 index fa560ffb9..000000000 --- a/clippy_lints/src/let_and_return.rs +++ /dev/null @@ -1,124 +0,0 @@ -use if_chain::if_chain; -use rustc_errors::Applicability; -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::{fn_def_id, 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<'tcx> LateLintPass<'tcx> for LetReturn { - fn check_block(&mut self, cx: &LateContext<'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 !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); - 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"); - } - }, - ); - } - } - } -} - -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<'tcx>, - borrows: bool, -} - -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) = fn_def_id(self.cx, 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/lib.rs b/clippy_lints/src/lib.rs index 518e6905e..f28d281a4 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -218,7 +218,6 @@ 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; @@ -311,6 +310,7 @@ mod unnested_or_patterns; mod unsafe_removed_from_name; mod unused_io_amount; mod unused_self; +mod unused_unit; mod unwrap; mod use_self; mod useless_conversion; @@ -587,7 +587,6 @@ 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, @@ -771,8 +770,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: ®ex::INVALID_REGEX, ®ex::TRIVIAL_REGEX, &repeat_once::REPEAT_ONCE, + &returns::LET_AND_RETURN, &returns::NEEDLESS_RETURN, - &returns::UNUSED_UNIT, &serde_api::SERDE_API_MISUSE, &shadow::SHADOW_REUSE, &shadow::SHADOW_SAME, @@ -843,6 +842,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, &unused_io_amount::UNUSED_IO_AMOUNT, &unused_self::UNUSED_SELF, + &unused_unit::UNUSED_UNIT, &unwrap::PANICKING_UNWRAP, &unwrap::UNNECESSARY_UNWRAP, &use_self::USE_SELF, @@ -1029,8 +1029,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_early_pass(|| box misc_early::MiscEarlyLints); store.register_early_pass(|| box redundant_closure_call::RedundantClosureCall); store.register_late_pass(|| box redundant_closure_call::RedundantClosureCall); - store.register_early_pass(|| box returns::Return); - store.register_late_pass(|| box let_and_return::LetReturn); + store.register_early_pass(|| box unused_unit::UnusedUnit); + store.register_late_pass(|| box returns::Return); store.register_early_pass(|| box collapsible_if::CollapsibleIf); store.register_early_pass(|| box items_after_statements::ItemsAfterStatements); store.register_early_pass(|| box precedence::Precedence); @@ -1288,7 +1288,6 @@ 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), @@ -1418,8 +1417,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(®ex::INVALID_REGEX), LintId::of(®ex::TRIVIAL_REGEX), LintId::of(&repeat_once::REPEAT_ONCE), + LintId::of(&returns::LET_AND_RETURN), LintId::of(&returns::NEEDLESS_RETURN), - LintId::of(&returns::UNUSED_UNIT), LintId::of(&serde_api::SERDE_API_MISUSE), LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(&slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), @@ -1466,6 +1465,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), + LintId::of(&unused_unit::UNUSED_UNIT), LintId::of(&unwrap::PANICKING_UNWRAP), LintId::of(&unwrap::UNNECESSARY_UNWRAP), LintId::of(&useless_conversion::USELESS_CONVERSION), @@ -1506,7 +1506,6 @@ 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), @@ -1561,8 +1560,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&redundant_field_names::REDUNDANT_FIELD_NAMES), LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES), 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), LintId::of(&strings::STRING_LIT_AS_BYTES), LintId::of(&tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), @@ -1571,6 +1570,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::FN_TO_NUMERIC_CAST), LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION), LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME), + LintId::of(&unused_unit::UNUSED_UNIT), LintId::of(&write::PRINTLN_EMPTY_STRING), LintId::of(&write::PRINT_LITERAL), LintId::of(&write::PRINT_WITH_NEWLINE), diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 8ed20995a..3c5541e64 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -1,14 +1,43 @@ use if_chain::if_chain; -use rustc_ast::ast; -use rustc_ast::visit::FnKind; +use rustc_ast::ast::Attribute; use rustc_errors::Applicability; -use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; +use rustc_hir::intravisit::{walk_expr, FnKind, NestedVisitorMap, Visitor}; +use rustc_hir::{Block, Body, Expr, ExprKind, FnDecl, HirId, MatchSource, 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 rustc_span::source_map::Span; -use rustc_span::BytePos; -use crate::utils::{snippet_opt, span_lint_and_sugg, span_lint_and_then}; +use crate::utils::{fn_def_id, in_macro, match_qpath, snippet_opt, span_lint_and_sugg, 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_clippy_lint! { /// **What it does:** Checks for return statements at the end of a block. @@ -16,8 +45,7 @@ declare_clippy_lint! { /// **Why is this bad?** Removing the `return` and semicolon will make the code /// more rusty. /// - /// **Known problems:** If the computation returning the value borrows a local - /// variable, removing the `return` may run afoul of the borrow checker. + /// **Known problems:** None. /// /// **Example:** /// ```rust @@ -36,248 +64,223 @@ declare_clippy_lint! { "using a return statement like `return expr;` where an expression would suffice" } -declare_clippy_lint! { - /// **What it does:** Checks for unit (`()`) expressions that can be removed. - /// - /// **Why is this bad?** Such expressions add no value, but can make the code - /// less readable. Depending on formatting they can make a `break` or `return` - /// statement look like a function call. - /// - /// **Known problems:** The lint currently misses unit return types in types, - /// e.g., the `F` in `fn generic_unit ()>(f: F) { .. }`. - /// - /// **Example:** - /// ```rust - /// fn return_unit() -> () { - /// () - /// } - /// ``` - pub UNUSED_UNIT, - style, - "needless unit expression" -} - #[derive(PartialEq, Eq, Copy, Clone)] enum RetReplacement { Empty, Block, } -declare_lint_pass!(Return => [NEEDLESS_RETURN, UNUSED_UNIT]); +declare_lint_pass!(Return => [LET_AND_RETURN, NEEDLESS_RETURN]); -impl Return { - // Check the final stmt or expr in a block for unnecessary return. - fn check_block_return(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) { - if let Some(stmt) = block.stmts.last() { - match stmt.kind { - ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => { - self.check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); - }, - _ => (), +impl<'tcx> LateLintPass<'tcx> for Return { + fn check_block(&mut self, cx: &LateContext<'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 !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); + 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"); + } + }, + ); } } } - // Check the final expression in a block if it's a return. - fn check_final_expr( + fn check_fn( &mut self, - cx: &EarlyContext<'_>, - expr: &ast::Expr, - span: Option, - replacement: RetReplacement, + cx: &LateContext<'tcx>, + kind: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: HirId, ) { - match expr.kind { - // simple return is always "bad" - ast::ExprKind::Ret(ref inner) => { - // allow `#[cfg(a)] return a; #[cfg(b)] return b;` - if !expr.attrs.iter().any(attr_is_cfg) { - Self::emit_return_lint( + match kind { + FnKind::Closure(_) => check_final_expr(cx, &body.value, Some(body.value.span), RetReplacement::Empty), + FnKind::ItemFn(..) | FnKind::Method(..) => { + if let ExprKind::Block(ref block, _) = body.value.kind { + check_block_return(cx, block); + } + }, + } + } +} + +fn attr_is_cfg(attr: &Attribute) -> bool { + attr.meta_item_list().is_some() && attr.has_name(sym!(cfg)) +} + +fn check_block_return<'tcx>(cx: &LateContext<'tcx>, block: &Block<'tcx>) { + if let Some(expr) = block.expr { + check_final_expr(cx, expr, Some(expr.span), RetReplacement::Empty); + } else if let Some(stmt) = block.stmts.iter().last() { + match stmt.kind { + StmtKind::Expr(ref expr) | StmtKind::Semi(ref expr) => { + check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty); + }, + _ => (), + } + } +} + +fn check_final_expr<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + span: Option, + replacement: RetReplacement, +) { + match expr.kind { + // simple return is always "bad" + ExprKind::Ret(ref inner) => { + // allow `#[cfg(a)] return a; #[cfg(b)] return b;` + if !expr.attrs.iter().any(attr_is_cfg) { + let borrows = inner.map_or(false, |inner| last_statement_borrows(cx, inner)); + if !borrows { + emit_return_lint( cx, span.expect("`else return` is not possible"), inner.as_ref().map(|i| i.span), replacement, ); } - }, - // a whole block? check it! - ast::ExprKind::Block(ref block, _) => { - self.check_block_return(cx, block); - }, - // an if/if let expr, check both exprs - // note, if without else is going to be a type checking error anyways - // (except for unit type functions) so we don't match it - ast::ExprKind::If(_, ref ifblock, Some(ref elsexpr)) => { - self.check_block_return(cx, ifblock); - self.check_final_expr(cx, elsexpr, None, RetReplacement::Empty); - }, - // a match expr, check all arms - ast::ExprKind::Match(_, ref arms) => { - for arm in arms { - self.check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Block); + } + }, + // a whole block? check it! + ExprKind::Block(ref block, _) => { + check_block_return(cx, block); + }, + // a match expr, check all arms + // an if/if let expr, check both exprs + // 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(_, ref arms, source) => match source { + MatchSource::Normal => { + for arm in arms.iter() { + check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Block); } }, + MatchSource::IfDesugar { + contains_else_clause: true, + } + | MatchSource::IfLetDesugar { + contains_else_clause: true, + } => { + if let ExprKind::Block(ref ifblock, _) = arms[0].body.kind { + check_block_return(cx, ifblock); + } + check_final_expr(cx, arms[1].body, None, RetReplacement::Empty); + }, _ => (), - } - } - - fn emit_return_lint(cx: &EarlyContext<'_>, ret_span: Span, inner_span: Option, replacement: RetReplacement) { - match inner_span { - Some(inner_span) => { - if in_external_macro(cx.sess(), inner_span) || inner_span.from_expansion() { - return; - } - - span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| { - if let Some(snippet) = snippet_opt(cx, inner_span) { - diag.span_suggestion(ret_span, "remove `return`", snippet, Applicability::MachineApplicable); - } - }) - }, - None => match replacement { - RetReplacement::Empty => { - span_lint_and_sugg( - cx, - NEEDLESS_RETURN, - ret_span, - "unneeded `return` statement", - "remove `return`", - String::new(), - Applicability::MachineApplicable, - ); - }, - RetReplacement::Block => { - span_lint_and_sugg( - cx, - NEEDLESS_RETURN, - ret_span, - "unneeded `return` statement", - "replace `return` with an empty block", - "{}".to_string(), - Applicability::MachineApplicable, - ); - }, - }, - } + }, + _ => (), } } -impl EarlyLintPass for Return { - fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, span: Span, _: ast::NodeId) { - match kind { - FnKind::Fn(.., Some(block)) => self.check_block_return(cx, block), - FnKind::Closure(_, body) => self.check_final_expr(cx, body, Some(body.span), RetReplacement::Empty), - FnKind::Fn(.., None) => {}, - } - if_chain! { - if let ast::FnRetTy::Ty(ref ty) = kind.decl().output; - if let ast::TyKind::Tup(ref vals) = ty.kind; - if vals.is_empty() && !ty.span.from_expansion() && get_def(span) == get_def(ty.span); - then { - lint_unneeded_unit_return(cx, ty, span); +fn emit_return_lint(cx: &LateContext<'_>, ret_span: Span, inner_span: Option, replacement: RetReplacement) { + match inner_span { + Some(inner_span) => { + if in_external_macro(cx.tcx.sess, inner_span) || inner_span.from_expansion() { + return; } - } - } - fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) { - if_chain! { - if let Some(ref stmt) = block.stmts.last(); - if let ast::StmtKind::Expr(ref expr) = stmt.kind; - if is_unit_expr(expr) && !stmt.span.from_expansion(); - then { - let sp = expr.span; + span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded `return` statement", |diag| { + if let Some(snippet) = snippet_opt(cx, inner_span) { + diag.span_suggestion(ret_span, "remove `return`", snippet, Applicability::MachineApplicable); + } + }) + }, + None => match replacement { + RetReplacement::Empty => { span_lint_and_sugg( cx, - UNUSED_UNIT, - sp, - "unneeded unit expression", - "remove the final `()`", + NEEDLESS_RETURN, + ret_span, + "unneeded `return` statement", + "remove `return`", String::new(), Applicability::MachineApplicable, ); - } - } - } - - fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) { - match e.kind { - ast::ExprKind::Ret(Some(ref expr)) | ast::ExprKind::Break(_, Some(ref expr)) => { - if is_unit_expr(expr) && !expr.span.from_expansion() { - span_lint_and_sugg( - cx, - UNUSED_UNIT, - expr.span, - "unneeded `()`", - "remove the `()`", - String::new(), - Applicability::MachineApplicable, - ); - } }, - _ => (), - } - } - - fn check_poly_trait_ref(&mut self, cx: &EarlyContext<'_>, poly: &ast::PolyTraitRef, _: &ast::TraitBoundModifier) { - let segments = &poly.trait_ref.path.segments; - - if_chain! { - if segments.len() == 1; - if ["Fn", "FnMut", "FnOnce"].contains(&&*segments[0].ident.name.as_str()); - if let Some(args) = &segments[0].args; - if let ast::GenericArgs::Parenthesized(generic_args) = &**args; - if let ast::FnRetTy::Ty(ty) = &generic_args.output; - if ty.kind.is_unit(); - then { - lint_unneeded_unit_return(cx, ty, generic_args.span); - } - } - } -} - -fn attr_is_cfg(attr: &ast::Attribute) -> bool { - attr.meta_item_list().is_some() && attr.has_name(sym!(cfg)) -} - -// get the def site -#[must_use] -fn get_def(span: Span) -> Option { - if span.from_expansion() { - Some(span.ctxt().outer_expn_data().def_site) - } else { - None - } -} - -// is this expr a `()` unit? -fn is_unit_expr(expr: &ast::Expr) -> bool { - if let ast::ExprKind::Tup(ref vals) = expr.kind { - vals.is_empty() - } else { - false - } -} - -fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { - let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - fn_source - .rfind("->") - .map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { - ( - #[allow(clippy::cast_possible_truncation)] - ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + RetReplacement::Block => { + span_lint_and_sugg( + cx, + NEEDLESS_RETURN, + ret_span, + "unneeded `return` statement", + "replace `return` with an empty block", + "{}".to_string(), Applicability::MachineApplicable, - ) - }) - } else { - (ty.span, Applicability::MaybeIncorrect) - }; - span_lint_and_sugg( - cx, - UNUSED_UNIT, - ret_span, - "unneeded unit return type", - "remove the `-> ()`", - String::new(), - appl, - ); + ); + }, + }, + } +} + +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<'tcx>, + borrows: bool, +} + +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) = fn_def_id(self.cx, 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/unused_unit.rs b/clippy_lints/src/unused_unit.rs new file mode 100644 index 000000000..7548c6afa --- /dev/null +++ b/clippy_lints/src/unused_unit.rs @@ -0,0 +1,144 @@ +use if_chain::if_chain; +use rustc_ast::ast; +use rustc_ast::visit::FnKind; +use rustc_errors::Applicability; +use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Span; +use rustc_span::BytePos; + +use crate::utils::span_lint_and_sugg; + +declare_clippy_lint! { + /// **What it does:** Checks for unit (`()`) expressions that can be removed. + /// + /// **Why is this bad?** Such expressions add no value, but can make the code + /// less readable. Depending on formatting they can make a `break` or `return` + /// statement look like a function call. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// fn return_unit() -> () { + /// () + /// } + /// ``` + pub UNUSED_UNIT, + style, + "needless unit expression" +} + +declare_lint_pass!(UnusedUnit => [UNUSED_UNIT]); + +impl EarlyLintPass for UnusedUnit { + fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, span: Span, _: ast::NodeId) { + if_chain! { + if let ast::FnRetTy::Ty(ref ty) = kind.decl().output; + if let ast::TyKind::Tup(ref vals) = ty.kind; + if vals.is_empty() && !ty.span.from_expansion() && get_def(span) == get_def(ty.span); + then { + lint_unneeded_unit_return(cx, ty, span); + } + } + } + + fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) { + if_chain! { + if let Some(ref stmt) = block.stmts.last(); + if let ast::StmtKind::Expr(ref expr) = stmt.kind; + if is_unit_expr(expr) && !stmt.span.from_expansion(); + then { + let sp = expr.span; + span_lint_and_sugg( + cx, + UNUSED_UNIT, + sp, + "unneeded unit expression", + "remove the final `()`", + String::new(), + Applicability::MachineApplicable, + ); + } + } + } + + fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) { + match e.kind { + ast::ExprKind::Ret(Some(ref expr)) | ast::ExprKind::Break(_, Some(ref expr)) => { + if is_unit_expr(expr) && !expr.span.from_expansion() { + span_lint_and_sugg( + cx, + UNUSED_UNIT, + expr.span, + "unneeded `()`", + "remove the `()`", + String::new(), + Applicability::MachineApplicable, + ); + } + }, + _ => (), + } + } + + fn check_poly_trait_ref(&mut self, cx: &EarlyContext<'_>, poly: &ast::PolyTraitRef, _: &ast::TraitBoundModifier) { + let segments = &poly.trait_ref.path.segments; + + if_chain! { + if segments.len() == 1; + if ["Fn", "FnMut", "FnOnce"].contains(&&*segments[0].ident.name.as_str()); + if let Some(args) = &segments[0].args; + if let ast::GenericArgs::Parenthesized(generic_args) = &**args; + if let ast::FnRetTy::Ty(ty) = &generic_args.output; + if ty.kind.is_unit(); + then { + lint_unneeded_unit_return(cx, ty, generic_args.span); + } + } + } +} + +// get the def site +#[must_use] +fn get_def(span: Span) -> Option { + if span.from_expansion() { + Some(span.ctxt().outer_expn_data().def_site) + } else { + None + } +} + +// is this expr a `()` unit? +fn is_unit_expr(expr: &ast::Expr) -> bool { + if let ast::ExprKind::Tup(ref vals) = expr.kind { + vals.is_empty() + } else { + false + } +} + +fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { + let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { + fn_source + .rfind("->") + .map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { + ( + #[allow(clippy::cast_possible_truncation)] + ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + Applicability::MachineApplicable, + ) + }) + } else { + (ty.span, Applicability::MaybeIncorrect) + }; + span_lint_and_sugg( + cx, + UNUSED_UNIT, + ret_span, + "unneeded unit return type", + "remove the `-> ()`", + String::new(), + appl, + ); +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f76a21500..233f95dee 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1037,7 +1037,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: "let_and_return", + module: "returns", }, Lint { name: "let_underscore_lock", @@ -2493,7 +2493,7 @@ pub static ref ALL_LINTS: Vec = vec![ group: "style", desc: "needless unit expression", deprecation: None, - module: "returns", + module: "unused_unit", }, Lint { name: "unwrap_used", diff --git a/tests/ui/needless_return.fixed b/tests/ui/needless_return.fixed index ad20e2381..d849e093d 100644 --- a/tests/ui/needless_return.fixed +++ b/tests/ui/needless_return.fixed @@ -69,6 +69,23 @@ fn test_void_match(x: u32) { } } +fn read_line() -> String { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + return stdin.lock().lines().next().unwrap().unwrap(); +} + +fn borrows_but_not_last(value: bool) -> String { + if value { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + let _a = stdin.lock().lines().next().unwrap().unwrap(); + String::from("test") + } else { + String::new() + } +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs index af0cdfb20..29f2bd185 100644 --- a/tests/ui/needless_return.rs +++ b/tests/ui/needless_return.rs @@ -69,6 +69,23 @@ fn test_void_match(x: u32) { } } +fn read_line() -> String { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + return stdin.lock().lines().next().unwrap().unwrap(); +} + +fn borrows_but_not_last(value: bool) -> String { + if value { + use std::io::BufRead; + let stdin = ::std::io::stdin(); + let _a = stdin.lock().lines().next().unwrap().unwrap(); + return String::from("test"); + } else { + return String::new(); + } +} + fn main() { let _ = test_end_of_fn(); let _ = test_no_semicolon(); diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr index c34eecbcb..f73c833a8 100644 --- a/tests/ui/needless_return.stderr +++ b/tests/ui/needless_return.stderr @@ -72,5 +72,17 @@ error: unneeded `return` statement LL | _ => return, | ^^^^^^ help: replace `return` with an empty block: `{}` -error: aborting due to 12 previous errors +error: unneeded `return` statement + --> $DIR/needless_return.rs:83:9 + | +LL | return String::from("test"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::from("test")` + +error: unneeded `return` statement + --> $DIR/needless_return.rs:85:9 + | +LL | return String::new(); + | ^^^^^^^^^^^^^^^^^^^^^ help: remove `return`: `String::new()` + +error: aborting due to 14 previous errors