From ecbef77bd71c2a4b4ad8dbb0b0ffa67affe29cc3 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 12 Mar 2021 12:04:44 +0900 Subject: [PATCH 1/6] Extract lints of unit_types group from types group --- clippy_lints/src/lib.rs | 23 +- clippy_lints/src/types/mod.rs | 396 +--------------------------- clippy_lints/src/unit_types/mod.rs | 398 +++++++++++++++++++++++++++++ 3 files changed, 415 insertions(+), 402 deletions(-) create mode 100644 clippy_lints/src/unit_types/mod.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 52c342d580e24..f80aee182e22e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -350,6 +350,7 @@ mod types; mod undropped_manually_drops; mod unicode; mod unit_return_expecting_ord; +mod unit_types; mod unnamed_address; mod unnecessary_sort_by; mod unnecessary_wraps; @@ -960,20 +961,20 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &types::BOX_VEC, &types::IMPLICIT_HASHER, &types::INVALID_UPCAST_COMPARISONS, - &types::LET_UNIT_VALUE, &types::LINKEDLIST, &types::OPTION_OPTION, &types::RC_BUFFER, &types::REDUNDANT_ALLOCATION, &types::TYPE_COMPLEXITY, - &types::UNIT_ARG, - &types::UNIT_CMP, &types::VEC_BOX, &undropped_manually_drops::UNDROPPED_MANUALLY_DROPS, &unicode::INVISIBLE_CHARACTERS, &unicode::NON_ASCII_LITERAL, &unicode::UNICODE_NOT_NFC, &unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD, + &unit_types::LET_UNIT_VALUE, + &unit_types::UNIT_ARG, + &unit_types::UNIT_CMP, &unnamed_address::FN_ADDRESS_COMPARISONS, &unnamed_address::VTABLE_ADDRESS_COMPARISONS, &unnecessary_sort_by::UNNECESSARY_SORT_BY, @@ -1084,8 +1085,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box map_clone::MapClone); store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); - store.register_late_pass(|| box types::LetUnitValue); - store.register_late_pass(|| box types::UnitCmp); + store.register_late_pass(|| box unit_types::LetUnitValue); + store.register_late_pass(|| box unit_types::UnitCmp); store.register_late_pass(|| box loops::Loops); store.register_late_pass(|| box main_recursion::MainRecursion::default()); store.register_late_pass(|| box lifetimes::Lifetimes); @@ -1160,7 +1161,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box useless_conversion::UselessConversion::default()); store.register_late_pass(|| box types::ImplicitHasher); store.register_late_pass(|| box fallible_impl_from::FallibleImplFrom); - store.register_late_pass(|| box types::UnitArg); + store.register_late_pass(|| box unit_types::UnitArg); store.register_late_pass(|| box double_comparison::DoubleComparisons); store.register_late_pass(|| box question_mark::QuestionMark); store.register_early_pass(|| box suspicious_operation_groupings::SuspiciousOperationGroupings); @@ -1415,11 +1416,11 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS), LintId::of(&types::IMPLICIT_HASHER), LintId::of(&types::INVALID_UPCAST_COMPARISONS), - LintId::of(&types::LET_UNIT_VALUE), LintId::of(&types::LINKEDLIST), LintId::of(&types::OPTION_OPTION), LintId::of(&unicode::NON_ASCII_LITERAL), LintId::of(&unicode::UNICODE_NOT_NFC), + LintId::of(&unit_types::LET_UNIT_VALUE), LintId::of(&unnecessary_wraps::UNNECESSARY_WRAPS), LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS), LintId::of(&unused_self::UNUSED_SELF), @@ -1708,12 +1709,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&types::BOX_VEC), LintId::of(&types::REDUNDANT_ALLOCATION), LintId::of(&types::TYPE_COMPLEXITY), - LintId::of(&types::UNIT_ARG), - LintId::of(&types::UNIT_CMP), LintId::of(&types::VEC_BOX), LintId::of(&undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(&unicode::INVISIBLE_CHARACTERS), LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), + LintId::of(&unit_types::UNIT_ARG), + LintId::of(&unit_types::UNIT_CMP), LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), @@ -1934,8 +1935,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::TRANSMUTE_PTR_TO_REF), LintId::of(&types::BORROWED_BOX), LintId::of(&types::TYPE_COMPLEXITY), - LintId::of(&types::UNIT_ARG), LintId::of(&types::VEC_BOX), + LintId::of(&unit_types::UNIT_ARG), LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY), LintId::of(&unwrap::UNNECESSARY_UNWRAP), LintId::of(&useless_conversion::USELESS_CONVERSION), @@ -2005,10 +2006,10 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&transmute::WRONG_TRANSMUTE), LintId::of(&transmuting_null::TRANSMUTING_NULL), LintId::of(&types::ABSURD_EXTREME_COMPARISONS), - LintId::of(&types::UNIT_CMP), LintId::of(&undropped_manually_drops::UNDROPPED_MANUALLY_DROPS), LintId::of(&unicode::INVISIBLE_CHARACTERS), LintId::of(&unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD), + LintId::of(&unit_types::UNIT_CMP), LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS), LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS), LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT), diff --git a/clippy_lints/src/types/mod.rs b/clippy_lints/src/types/mod.rs index ac4fe502a7adf..5103a259559b0 100644 --- a/clippy_lints/src/types/mod.rs +++ b/clippy_lints/src/types/mod.rs @@ -14,23 +14,21 @@ use std::cmp::Ordering; use std::collections::BTreeMap; use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_help, span_lint_and_then}; -use clippy_utils::source::{indent_of, reindent_multiline, snippet, snippet_opt, snippet_with_macro_callsite}; +use clippy_utils::source::{snippet, snippet_opt}; use clippy_utils::ty::{is_isize_or_usize, is_type_diagnostic_item}; use if_chain::if_chain; -use rustc_errors::{Applicability, DiagnosticBuilder}; +use rustc_errors::DiagnosticBuilder; use rustc_hir as hir; use rustc_hir::intravisit::{walk_body, walk_expr, walk_ty, FnKind, NestedVisitorMap, Visitor}; use rustc_hir::{ - BinOpKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, - ImplItemKind, Item, ItemKind, Local, MatchSource, MutTy, Node, QPath, Stmt, StmtKind, TraitFn, TraitItem, - TraitItemKind, TyKind, + BinOpKind, Body, Expr, ExprKind, FnDecl, FnRetTy, FnSig, GenericArg, GenericParamKind, HirId, ImplItem, + ImplItemKind, Item, ItemKind, Local, MutTy, QPath, TraitFn, TraitItem, TraitItemKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::hir::map::Map; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::{self, IntTy, Ty, TyS, TypeckResults, UintTy}; use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass}; -use rustc_span::hygiene::{ExpnKind, MacroKind}; use rustc_span::source_map::Span; use rustc_span::symbol::sym; use rustc_target::abi::LayoutOf; @@ -39,7 +37,7 @@ use rustc_typeck::hir_ty_to_ty; use crate::consts::{constant, Constant}; use crate::utils::paths; -use crate::utils::{clip, comparisons, differing_macro_contexts, higher, int_bits, match_path, sext, unsext}; +use crate::utils::{clip, comparisons, differing_macro_contexts, int_bits, match_path, sext, unsext}; declare_clippy_lint! { /// **What it does:** Checks for use of `Box>` anywhere in the code. @@ -389,390 +387,6 @@ impl Types { } } -declare_clippy_lint! { - /// **What it does:** Checks for binding a unit value. - /// - /// **Why is this bad?** A unit value cannot usefully be used anywhere. So - /// binding one is kind of pointless. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// let x = { - /// 1; - /// }; - /// ``` - pub LET_UNIT_VALUE, - pedantic, - "creating a `let` binding to a value of unit type, which usually can't be used afterwards" -} - -declare_lint_pass!(LetUnitValue => [LET_UNIT_VALUE]); - -impl<'tcx> LateLintPass<'tcx> for LetUnitValue { - fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { - if let StmtKind::Local(ref local) = stmt.kind { - if is_unit(cx.typeck_results().pat_ty(&local.pat)) { - if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() { - return; - } - if higher::is_from_for_desugar(local) { - return; - } - span_lint_and_then( - cx, - LET_UNIT_VALUE, - stmt.span, - "this let-binding has unit value", - |diag| { - if let Some(expr) = &local.init { - let snip = snippet_with_macro_callsite(cx, expr.span, "()"); - diag.span_suggestion( - stmt.span, - "omit the `let` binding", - format!("{};", snip), - Applicability::MachineApplicable, // snippet - ); - } - }, - ); - } - } - } -} - -declare_clippy_lint! { - /// **What it does:** Checks for comparisons to unit. This includes all binary - /// comparisons (like `==` and `<`) and asserts. - /// - /// **Why is this bad?** Unit is always equal to itself, and thus is just a - /// clumsily written constant. Mostly this happens when someone accidentally - /// adds semicolons at the end of the operands. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// # fn foo() {}; - /// # fn bar() {}; - /// # fn baz() {}; - /// if { - /// foo(); - /// } == { - /// bar(); - /// } { - /// baz(); - /// } - /// ``` - /// is equal to - /// ```rust - /// # fn foo() {}; - /// # fn bar() {}; - /// # fn baz() {}; - /// { - /// foo(); - /// bar(); - /// baz(); - /// } - /// ``` - /// - /// For asserts: - /// ```rust - /// # fn foo() {}; - /// # fn bar() {}; - /// assert_eq!({ foo(); }, { bar(); }); - /// ``` - /// will always succeed - pub UNIT_CMP, - correctness, - "comparing unit values" -} - -declare_lint_pass!(UnitCmp => [UNIT_CMP]); - -impl<'tcx> LateLintPass<'tcx> for UnitCmp { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if expr.span.from_expansion() { - if let Some(callee) = expr.span.source_callee() { - if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { - if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { - let op = cmp.node; - if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { - let result = match &*symbol.as_str() { - "assert_eq" | "debug_assert_eq" => "succeed", - "assert_ne" | "debug_assert_ne" => "fail", - _ => return, - }; - span_lint( - cx, - UNIT_CMP, - expr.span, - &format!( - "`{}` of unit values detected. This will always {}", - symbol.as_str(), - result - ), - ); - } - } - } - } - return; - } - if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { - let op = cmp.node; - if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { - let result = match op { - BinOpKind::Eq | BinOpKind::Le | BinOpKind::Ge => "true", - _ => "false", - }; - span_lint( - cx, - UNIT_CMP, - expr.span, - &format!( - "{}-comparison of unit values detected. This will always be {}", - op.as_str(), - result - ), - ); - } - } - } -} - -declare_clippy_lint! { - /// **What it does:** Checks for passing a unit value as an argument to a function without using a - /// unit literal (`()`). - /// - /// **Why is this bad?** This is likely the result of an accidental semicolon. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust,ignore - /// foo({ - /// let a = bar(); - /// baz(a); - /// }) - /// ``` - pub UNIT_ARG, - complexity, - "passing unit to a function" -} - -declare_lint_pass!(UnitArg => [UNIT_ARG]); - -impl<'tcx> LateLintPass<'tcx> for UnitArg { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if expr.span.from_expansion() { - return; - } - - // apparently stuff in the desugaring of `?` can trigger this - // so check for that here - // only the calls to `Try::from_error` is marked as desugared, - // so we need to check both the current Expr and its parent. - if is_questionmark_desugar_marked_call(expr) { - return; - } - if_chain! { - let map = &cx.tcx.hir(); - let opt_parent_node = map.find(map.get_parent_node(expr.hir_id)); - if let Some(hir::Node::Expr(parent_expr)) = opt_parent_node; - if is_questionmark_desugar_marked_call(parent_expr); - then { - return; - } - } - - match expr.kind { - ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => { - let args_to_recover = args - .iter() - .filter(|arg| { - if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { - !matches!( - &arg.kind, - ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) - ) - } else { - false - } - }) - .collect::>(); - if !args_to_recover.is_empty() { - lint_unit_args(cx, expr, &args_to_recover); - } - }, - _ => (), - } - } -} - -fn fmt_stmts_and_call( - cx: &LateContext<'_>, - call_expr: &Expr<'_>, - call_snippet: &str, - args_snippets: &[impl AsRef], - non_empty_block_args_snippets: &[impl AsRef], -) -> String { - let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0); - let call_snippet_with_replacements = args_snippets - .iter() - .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1)); - - let mut stmts_and_call = non_empty_block_args_snippets - .iter() - .map(|it| it.as_ref().to_owned()) - .collect::>(); - stmts_and_call.push(call_snippet_with_replacements); - stmts_and_call = stmts_and_call - .into_iter() - .map(|v| reindent_multiline(v.into(), true, Some(call_expr_indent)).into_owned()) - .collect(); - - let mut stmts_and_call_snippet = stmts_and_call.join(&format!("{}{}", ";\n", " ".repeat(call_expr_indent))); - // expr is not in a block statement or result expression position, wrap in a block - let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(call_expr.hir_id)); - if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) { - let block_indent = call_expr_indent + 4; - stmts_and_call_snippet = - reindent_multiline(stmts_and_call_snippet.into(), true, Some(block_indent)).into_owned(); - stmts_and_call_snippet = format!( - "{{\n{}{}\n{}}}", - " ".repeat(block_indent), - &stmts_and_call_snippet, - " ".repeat(call_expr_indent) - ); - } - stmts_and_call_snippet -} - -fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { - let mut applicability = Applicability::MachineApplicable; - let (singular, plural) = if args_to_recover.len() > 1 { - ("", "s") - } else { - ("a ", "") - }; - span_lint_and_then( - cx, - UNIT_ARG, - expr.span, - &format!("passing {}unit value{} to a function", singular, plural), - |db| { - let mut or = ""; - args_to_recover - .iter() - .filter_map(|arg| { - if_chain! { - if let ExprKind::Block(block, _) = arg.kind; - if block.expr.is_none(); - if let Some(last_stmt) = block.stmts.iter().last(); - if let StmtKind::Semi(last_expr) = last_stmt.kind; - if let Some(snip) = snippet_opt(cx, last_expr.span); - then { - Some(( - last_stmt.span, - snip, - )) - } - else { - None - } - } - }) - .for_each(|(span, sugg)| { - db.span_suggestion( - span, - "remove the semicolon from the last statement in the block", - sugg, - Applicability::MaybeIncorrect, - ); - or = "or "; - applicability = Applicability::MaybeIncorrect; - }); - - let arg_snippets: Vec = args_to_recover - .iter() - .filter_map(|arg| snippet_opt(cx, arg.span)) - .collect(); - let arg_snippets_without_empty_blocks: Vec = args_to_recover - .iter() - .filter(|arg| !is_empty_block(arg)) - .filter_map(|arg| snippet_opt(cx, arg.span)) - .collect(); - - if let Some(call_snippet) = snippet_opt(cx, expr.span) { - let sugg = fmt_stmts_and_call( - cx, - expr, - &call_snippet, - &arg_snippets, - &arg_snippets_without_empty_blocks, - ); - - if arg_snippets_without_empty_blocks.is_empty() { - db.multipart_suggestion( - &format!("use {}unit literal{} instead", singular, plural), - args_to_recover - .iter() - .map(|arg| (arg.span, "()".to_string())) - .collect::>(), - applicability, - ); - } else { - let plural = arg_snippets_without_empty_blocks.len() > 1; - let empty_or_s = if plural { "s" } else { "" }; - let it_or_them = if plural { "them" } else { "it" }; - db.span_suggestion( - expr.span, - &format!( - "{}move the expression{} in front of the call and replace {} with the unit literal `()`", - or, empty_or_s, it_or_them - ), - sugg, - applicability, - ); - } - } - }, - ); -} - -fn is_empty_block(expr: &Expr<'_>) -> bool { - matches!( - expr.kind, - ExprKind::Block( - Block { - stmts: &[], - expr: None, - .. - }, - _, - ) - ) -} - -fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { - use rustc_span::hygiene::DesugaringKind; - if let ExprKind::Call(ref callee, _) = expr.kind { - callee.span.is_desugaring(DesugaringKind::QuestionMark) - } else { - false - } -} - -fn is_unit(ty: Ty<'_>) -> bool { - matches!(ty.kind(), ty::Tuple(slice) if slice.is_empty()) -} - -fn is_unit_literal(expr: &Expr<'_>) -> bool { - matches!(expr.kind, ExprKind::Tup(ref slice) if slice.is_empty()) -} - declare_clippy_lint! { /// **What it does:** Checks for types used in structs, parameters and `let` /// declarations above a certain complexity threshold. diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs new file mode 100644 index 0000000000000..6450ef600ca2b --- /dev/null +++ b/clippy_lints/src/unit_types/mod.rs @@ -0,0 +1,398 @@ +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty::{self, Ty}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::hygiene::{ExpnKind, MacroKind}; + +use if_chain::if_chain; + +use crate::utils::diagnostics::{span_lint, span_lint_and_then}; +use crate::utils::higher; +use crate::utils::source::{indent_of, reindent_multiline, snippet_opt, snippet_with_macro_callsite}; + +declare_clippy_lint! { + /// **What it does:** Checks for binding a unit value. + /// + /// **Why is this bad?** A unit value cannot usefully be used anywhere. So + /// binding one is kind of pointless. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// let x = { + /// 1; + /// }; + /// ``` + pub LET_UNIT_VALUE, + pedantic, + "creating a `let` binding to a value of unit type, which usually can't be used afterwards" +} + +declare_lint_pass!(LetUnitValue => [LET_UNIT_VALUE]); + +impl<'tcx> LateLintPass<'tcx> for LetUnitValue { + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { + if let StmtKind::Local(ref local) = stmt.kind { + if is_unit(cx.typeck_results().pat_ty(&local.pat)) { + if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() { + return; + } + if higher::is_from_for_desugar(local) { + return; + } + span_lint_and_then( + cx, + LET_UNIT_VALUE, + stmt.span, + "this let-binding has unit value", + |diag| { + if let Some(expr) = &local.init { + let snip = snippet_with_macro_callsite(cx, expr.span, "()"); + diag.span_suggestion( + stmt.span, + "omit the `let` binding", + format!("{};", snip), + Applicability::MachineApplicable, // snippet + ); + } + }, + ); + } + } + } +} + +declare_clippy_lint! { + /// **What it does:** Checks for comparisons to unit. This includes all binary + /// comparisons (like `==` and `<`) and asserts. + /// + /// **Why is this bad?** Unit is always equal to itself, and thus is just a + /// clumsily written constant. Mostly this happens when someone accidentally + /// adds semicolons at the end of the operands. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// # fn baz() {}; + /// if { + /// foo(); + /// } == { + /// bar(); + /// } { + /// baz(); + /// } + /// ``` + /// is equal to + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// # fn baz() {}; + /// { + /// foo(); + /// bar(); + /// baz(); + /// } + /// ``` + /// + /// For asserts: + /// ```rust + /// # fn foo() {}; + /// # fn bar() {}; + /// assert_eq!({ foo(); }, { bar(); }); + /// ``` + /// will always succeed + pub UNIT_CMP, + correctness, + "comparing unit values" +} + +declare_lint_pass!(UnitCmp => [UNIT_CMP]); + +impl<'tcx> LateLintPass<'tcx> for UnitCmp { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if expr.span.from_expansion() { + if let Some(callee) = expr.span.source_callee() { + if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { + if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { + let op = cmp.node; + if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { + let result = match &*symbol.as_str() { + "assert_eq" | "debug_assert_eq" => "succeed", + "assert_ne" | "debug_assert_ne" => "fail", + _ => return, + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "`{}` of unit values detected. This will always {}", + symbol.as_str(), + result + ), + ); + } + } + } + } + return; + } + if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { + let op = cmp.node; + if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { + let result = match op { + BinOpKind::Eq | BinOpKind::Le | BinOpKind::Ge => "true", + _ => "false", + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "{}-comparison of unit values detected. This will always be {}", + op.as_str(), + result + ), + ); + } + } + } +} + +declare_clippy_lint! { + /// **What it does:** Checks for passing a unit value as an argument to a function without using a + /// unit literal (`()`). + /// + /// **Why is this bad?** This is likely the result of an accidental semicolon. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,ignore + /// foo({ + /// let a = bar(); + /// baz(a); + /// }) + /// ``` + pub UNIT_ARG, + complexity, + "passing unit to a function" +} + +declare_lint_pass!(UnitArg => [UNIT_ARG]); + +impl<'tcx> LateLintPass<'tcx> for UnitArg { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if expr.span.from_expansion() { + return; + } + + // apparently stuff in the desugaring of `?` can trigger this + // so check for that here + // only the calls to `Try::from_error` is marked as desugared, + // so we need to check both the current Expr and its parent. + if is_questionmark_desugar_marked_call(expr) { + return; + } + if_chain! { + let map = &cx.tcx.hir(); + let opt_parent_node = map.find(map.get_parent_node(expr.hir_id)); + if let Some(hir::Node::Expr(parent_expr)) = opt_parent_node; + if is_questionmark_desugar_marked_call(parent_expr); + then { + return; + } + } + + match expr.kind { + ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => { + let args_to_recover = args + .iter() + .filter(|arg| { + if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { + !matches!( + &arg.kind, + ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) + ) + } else { + false + } + }) + .collect::>(); + if !args_to_recover.is_empty() { + lint_unit_args(cx, expr, &args_to_recover); + } + }, + _ => (), + } + } +} + +fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { + use rustc_span::hygiene::DesugaringKind; + if let ExprKind::Call(ref callee, _) = expr.kind { + callee.span.is_desugaring(DesugaringKind::QuestionMark) + } else { + false + } +} + +fn is_unit(ty: Ty<'_>) -> bool { + matches!(ty.kind(), ty::Tuple(slice) if slice.is_empty()) +} + +fn is_unit_literal(expr: &Expr<'_>) -> bool { + matches!(expr.kind, ExprKind::Tup(ref slice) if slice.is_empty()) +} + +fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { + let mut applicability = Applicability::MachineApplicable; + let (singular, plural) = if args_to_recover.len() > 1 { + ("", "s") + } else { + ("a ", "") + }; + span_lint_and_then( + cx, + UNIT_ARG, + expr.span, + &format!("passing {}unit value{} to a function", singular, plural), + |db| { + let mut or = ""; + args_to_recover + .iter() + .filter_map(|arg| { + if_chain! { + if let ExprKind::Block(block, _) = arg.kind; + if block.expr.is_none(); + if let Some(last_stmt) = block.stmts.iter().last(); + if let StmtKind::Semi(last_expr) = last_stmt.kind; + if let Some(snip) = snippet_opt(cx, last_expr.span); + then { + Some(( + last_stmt.span, + snip, + )) + } + else { + None + } + } + }) + .for_each(|(span, sugg)| { + db.span_suggestion( + span, + "remove the semicolon from the last statement in the block", + sugg, + Applicability::MaybeIncorrect, + ); + or = "or "; + applicability = Applicability::MaybeIncorrect; + }); + + let arg_snippets: Vec = args_to_recover + .iter() + .filter_map(|arg| snippet_opt(cx, arg.span)) + .collect(); + let arg_snippets_without_empty_blocks: Vec = args_to_recover + .iter() + .filter(|arg| !is_empty_block(arg)) + .filter_map(|arg| snippet_opt(cx, arg.span)) + .collect(); + + if let Some(call_snippet) = snippet_opt(cx, expr.span) { + let sugg = fmt_stmts_and_call( + cx, + expr, + &call_snippet, + &arg_snippets, + &arg_snippets_without_empty_blocks, + ); + + if arg_snippets_without_empty_blocks.is_empty() { + db.multipart_suggestion( + &format!("use {}unit literal{} instead", singular, plural), + args_to_recover + .iter() + .map(|arg| (arg.span, "()".to_string())) + .collect::>(), + applicability, + ); + } else { + let plural = arg_snippets_without_empty_blocks.len() > 1; + let empty_or_s = if plural { "s" } else { "" }; + let it_or_them = if plural { "them" } else { "it" }; + db.span_suggestion( + expr.span, + &format!( + "{}move the expression{} in front of the call and replace {} with the unit literal `()`", + or, empty_or_s, it_or_them + ), + sugg, + applicability, + ); + } + } + }, + ); +} + +fn is_empty_block(expr: &Expr<'_>) -> bool { + matches!( + expr.kind, + ExprKind::Block( + Block { + stmts: &[], + expr: None, + .. + }, + _, + ) + ) +} + +fn fmt_stmts_and_call( + cx: &LateContext<'_>, + call_expr: &Expr<'_>, + call_snippet: &str, + args_snippets: &[impl AsRef], + non_empty_block_args_snippets: &[impl AsRef], +) -> String { + let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0); + let call_snippet_with_replacements = args_snippets + .iter() + .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1)); + + let mut stmts_and_call = non_empty_block_args_snippets + .iter() + .map(|it| it.as_ref().to_owned()) + .collect::>(); + stmts_and_call.push(call_snippet_with_replacements); + stmts_and_call = stmts_and_call + .into_iter() + .map(|v| reindent_multiline(v.into(), true, Some(call_expr_indent)).into_owned()) + .collect(); + + let mut stmts_and_call_snippet = stmts_and_call.join(&format!("{}{}", ";\n", " ".repeat(call_expr_indent))); + // expr is not in a block statement or result expression position, wrap in a block + let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(call_expr.hir_id)); + if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) { + let block_indent = call_expr_indent + 4; + stmts_and_call_snippet = + reindent_multiline(stmts_and_call_snippet.into(), true, Some(block_indent)).into_owned(); + stmts_and_call_snippet = format!( + "{{\n{}{}\n{}}}", + " ".repeat(block_indent), + &stmts_and_call_snippet, + " ".repeat(call_expr_indent) + ); + } + stmts_and_call_snippet +} From 37bffb7797ff7ffa65eeda3f936a1d45670c157b Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 12 Mar 2021 12:15:45 +0900 Subject: [PATCH 2/6] Extract utility functions to utils.rs --- clippy_lints/src/unit_types/mod.rs | 13 ++++--------- clippy_lints/src/unit_types/utils.rs | 10 ++++++++++ 2 files changed, 14 insertions(+), 9 deletions(-) create mode 100644 clippy_lints/src/unit_types/utils.rs diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs index 6450ef600ca2b..d71f9d7d24b06 100644 --- a/clippy_lints/src/unit_types/mod.rs +++ b/clippy_lints/src/unit_types/mod.rs @@ -1,9 +1,10 @@ +mod utils; + use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::{ExpnKind, MacroKind}; @@ -13,6 +14,8 @@ use crate::utils::diagnostics::{span_lint, span_lint_and_then}; use crate::utils::higher; use crate::utils::source::{indent_of, reindent_multiline, snippet_opt, snippet_with_macro_callsite}; +use utils::{is_unit, is_unit_literal}; + declare_clippy_lint! { /// **What it does:** Checks for binding a unit value. /// @@ -244,14 +247,6 @@ fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { } } -fn is_unit(ty: Ty<'_>) -> bool { - matches!(ty.kind(), ty::Tuple(slice) if slice.is_empty()) -} - -fn is_unit_literal(expr: &Expr<'_>) -> bool { - matches!(expr.kind, ExprKind::Tup(ref slice) if slice.is_empty()) -} - fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { let mut applicability = Applicability::MachineApplicable; let (singular, plural) = if args_to_recover.len() > 1 { diff --git a/clippy_lints/src/unit_types/utils.rs b/clippy_lints/src/unit_types/utils.rs new file mode 100644 index 0000000000000..a15a7b812c590 --- /dev/null +++ b/clippy_lints/src/unit_types/utils.rs @@ -0,0 +1,10 @@ +use rustc_hir::{Expr, ExprKind}; +use rustc_middle::ty::{self, Ty}; + +pub(super) fn is_unit(ty: Ty<'_>) -> bool { + matches!(ty.kind(), ty::Tuple(slice) if slice.is_empty()) +} + +pub(super) fn is_unit_literal(expr: &Expr<'_>) -> bool { + matches!(expr.kind, ExprKind::Tup(ref slice) if slice.is_empty()) +} From d17f54538f602480a10357f7677020190b9bac8e Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 12 Mar 2021 12:28:31 +0900 Subject: [PATCH 3/6] Move let_unit_value to its own module --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/unit_types/let_unit_value.rs | 40 +++++++++++++++++++ clippy_lints/src/unit_types/mod.rs | 39 +++--------------- 3 files changed, 47 insertions(+), 34 deletions(-) create mode 100644 clippy_lints/src/unit_types/let_unit_value.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f80aee182e22e..a8c4eb61a8539 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1085,7 +1085,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box map_clone::MapClone); store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); - store.register_late_pass(|| box unit_types::LetUnitValue); + store.register_late_pass(|| box unit_types::UnitTypes); store.register_late_pass(|| box unit_types::UnitCmp); store.register_late_pass(|| box loops::Loops); store.register_late_pass(|| box main_recursion::MainRecursion::default()); diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs new file mode 100644 index 0000000000000..126f6aa746adc --- /dev/null +++ b/clippy_lints/src/unit_types/let_unit_value.rs @@ -0,0 +1,40 @@ +use rustc_errors::Applicability; +use rustc_hir::{Stmt, StmtKind}; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; + +use crate::utils::diagnostics::span_lint_and_then; +use crate::utils::higher; +use crate::utils::source::snippet_with_macro_callsite; + +use super::{utils, LET_UNIT_VALUE}; + +pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) { + if let StmtKind::Local(ref local) = stmt.kind { + if utils::is_unit(cx.typeck_results().pat_ty(&local.pat)) { + if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() { + return; + } + if higher::is_from_for_desugar(local) { + return; + } + span_lint_and_then( + cx, + LET_UNIT_VALUE, + stmt.span, + "this let-binding has unit value", + |diag| { + if let Some(expr) = &local.init { + let snip = snippet_with_macro_callsite(cx, expr.span, "()"); + diag.span_suggestion( + stmt.span, + "omit the `let` binding", + format!("{};", snip), + Applicability::MachineApplicable, // snippet + ); + } + }, + ); + } + } +} diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs index d71f9d7d24b06..530c0fcf53dd7 100644 --- a/clippy_lints/src/unit_types/mod.rs +++ b/clippy_lints/src/unit_types/mod.rs @@ -1,18 +1,17 @@ +mod let_unit_value; mod utils; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_middle::lint::in_external_macro; +use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::hygiene::{ExpnKind, MacroKind}; use if_chain::if_chain; use crate::utils::diagnostics::{span_lint, span_lint_and_then}; -use crate::utils::higher; -use crate::utils::source::{indent_of, reindent_multiline, snippet_opt, snippet_with_macro_callsite}; +use crate::utils::source::{indent_of, reindent_multiline, snippet_opt}; use utils::{is_unit, is_unit_literal}; @@ -35,37 +34,11 @@ declare_clippy_lint! { "creating a `let` binding to a value of unit type, which usually can't be used afterwards" } -declare_lint_pass!(LetUnitValue => [LET_UNIT_VALUE]); +declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE]); -impl<'tcx> LateLintPass<'tcx> for LetUnitValue { +impl<'tcx> LateLintPass<'tcx> for UnitTypes { fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { - if let StmtKind::Local(ref local) = stmt.kind { - if is_unit(cx.typeck_results().pat_ty(&local.pat)) { - if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() { - return; - } - if higher::is_from_for_desugar(local) { - return; - } - span_lint_and_then( - cx, - LET_UNIT_VALUE, - stmt.span, - "this let-binding has unit value", - |diag| { - if let Some(expr) = &local.init { - let snip = snippet_with_macro_callsite(cx, expr.span, "()"); - diag.span_suggestion( - stmt.span, - "omit the `let` binding", - format!("{};", snip), - Applicability::MachineApplicable, // snippet - ); - } - }, - ); - } - } + let_unit_value::check(cx, stmt); } } From 1bb221243b853856596241ff3c4210e80a114d34 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 12 Mar 2021 12:38:31 +0900 Subject: [PATCH 4/6] Move unit_cmp to its own module --- clippy_lints/src/lib.rs | 1 - clippy_lints/src/unit_types/mod.rs | 71 ++++--------------------- clippy_lints/src/unit_types/unit_cmp.rs | 57 ++++++++++++++++++++ 3 files changed, 68 insertions(+), 61 deletions(-) create mode 100644 clippy_lints/src/unit_types/unit_cmp.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a8c4eb61a8539..c52c2ea80c5f0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1086,7 +1086,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box map_err_ignore::MapErrIgnore); store.register_late_pass(|| box shadow::Shadow); store.register_late_pass(|| box unit_types::UnitTypes); - store.register_late_pass(|| box unit_types::UnitCmp); store.register_late_pass(|| box loops::Loops); store.register_late_pass(|| box main_recursion::MainRecursion::default()); store.register_late_pass(|| box lifetimes::Lifetimes); diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs index 530c0fcf53dd7..40cc9d9dbfba2 100644 --- a/clippy_lints/src/unit_types/mod.rs +++ b/clippy_lints/src/unit_types/mod.rs @@ -1,16 +1,16 @@ mod let_unit_value; +mod unit_cmp; mod utils; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; +use rustc_hir::{Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::hygiene::{ExpnKind, MacroKind}; use if_chain::if_chain; -use crate::utils::diagnostics::{span_lint, span_lint_and_then}; +use crate::utils::diagnostics::span_lint_and_then; use crate::utils::source::{indent_of, reindent_multiline, snippet_opt}; use utils::{is_unit, is_unit_literal}; @@ -34,14 +34,6 @@ declare_clippy_lint! { "creating a `let` binding to a value of unit type, which usually can't be used afterwards" } -declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE]); - -impl<'tcx> LateLintPass<'tcx> for UnitTypes { - fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { - let_unit_value::check(cx, stmt); - } -} - declare_clippy_lint! { /// **What it does:** Checks for comparisons to unit. This includes all binary /// comparisons (like `==` and `<`) and asserts. @@ -89,56 +81,15 @@ declare_clippy_lint! { "comparing unit values" } -declare_lint_pass!(UnitCmp => [UNIT_CMP]); +declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP]); -impl<'tcx> LateLintPass<'tcx> for UnitCmp { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if expr.span.from_expansion() { - if let Some(callee) = expr.span.source_callee() { - if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { - if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { - let op = cmp.node; - if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { - let result = match &*symbol.as_str() { - "assert_eq" | "debug_assert_eq" => "succeed", - "assert_ne" | "debug_assert_ne" => "fail", - _ => return, - }; - span_lint( - cx, - UNIT_CMP, - expr.span, - &format!( - "`{}` of unit values detected. This will always {}", - symbol.as_str(), - result - ), - ); - } - } - } - } - return; - } - if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { - let op = cmp.node; - if op.is_comparison() && is_unit(cx.typeck_results().expr_ty(left)) { - let result = match op { - BinOpKind::Eq | BinOpKind::Le | BinOpKind::Ge => "true", - _ => "false", - }; - span_lint( - cx, - UNIT_CMP, - expr.span, - &format!( - "{}-comparison of unit values detected. This will always be {}", - op.as_str(), - result - ), - ); - } - } +impl LateLintPass<'_> for UnitTypes { + fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { + let_unit_value::check(cx, stmt); + } + + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + unit_cmp::check(cx, expr); } } diff --git a/clippy_lints/src/unit_types/unit_cmp.rs b/clippy_lints/src/unit_types/unit_cmp.rs new file mode 100644 index 0000000000000..e19fc1ec9efc7 --- /dev/null +++ b/clippy_lints/src/unit_types/unit_cmp.rs @@ -0,0 +1,57 @@ +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_span::hygiene::{ExpnKind, MacroKind}; + +use crate::utils::diagnostics::span_lint; + +use super::{utils, UNIT_CMP}; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { + if expr.span.from_expansion() { + if let Some(callee) = expr.span.source_callee() { + if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { + if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { + let op = cmp.node; + if op.is_comparison() && utils::is_unit(cx.typeck_results().expr_ty(left)) { + let result = match &*symbol.as_str() { + "assert_eq" | "debug_assert_eq" => "succeed", + "assert_ne" | "debug_assert_ne" => "fail", + _ => return, + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "`{}` of unit values detected. This will always {}", + symbol.as_str(), + result + ), + ); + } + } + } + } + return; + } + + if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { + let op = cmp.node; + if op.is_comparison() && utils::is_unit(cx.typeck_results().expr_ty(left)) { + let result = match op { + BinOpKind::Eq | BinOpKind::Le | BinOpKind::Ge => "true", + _ => "false", + }; + span_lint( + cx, + UNIT_CMP, + expr.span, + &format!( + "{}-comparison of unit values detected. This will always be {}", + op.as_str(), + result + ), + ); + } + } +} From 6211b49ac1c9a013869943a003efbd5a71a73c93 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Fri, 12 Mar 2021 12:47:21 +0900 Subject: [PATCH 5/6] Move unit_arg to its own module --- clippy_lints/src/lib.rs | 1 - clippy_lints/src/unit_types/mod.rs | 228 +----------------------- clippy_lints/src/unit_types/unit_arg.rs | 209 ++++++++++++++++++++++ 3 files changed, 218 insertions(+), 220 deletions(-) create mode 100644 clippy_lints/src/unit_types/unit_arg.rs diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c52c2ea80c5f0..f479d756a332b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1160,7 +1160,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box useless_conversion::UselessConversion::default()); store.register_late_pass(|| box types::ImplicitHasher); store.register_late_pass(|| box fallible_impl_from::FallibleImplFrom); - store.register_late_pass(|| box unit_types::UnitArg); store.register_late_pass(|| box double_comparison::DoubleComparisons); store.register_late_pass(|| box question_mark::QuestionMark); store.register_early_pass(|| box suspicious_operation_groupings::SuspiciousOperationGroupings); diff --git a/clippy_lints/src/unit_types/mod.rs b/clippy_lints/src/unit_types/mod.rs index 40cc9d9dbfba2..64420a0393349 100644 --- a/clippy_lints/src/unit_types/mod.rs +++ b/clippy_lints/src/unit_types/mod.rs @@ -1,20 +1,12 @@ mod let_unit_value; +mod unit_arg; mod unit_cmp; mod utils; -use rustc_errors::Applicability; -use rustc_hir as hir; -use rustc_hir::{Block, Expr, ExprKind, MatchSource, Node, Stmt, StmtKind}; +use rustc_hir::{Expr, Stmt}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use if_chain::if_chain; - -use crate::utils::diagnostics::span_lint_and_then; -use crate::utils::source::{indent_of, reindent_multiline, snippet_opt}; - -use utils::{is_unit, is_unit_literal}; - declare_clippy_lint! { /// **What it does:** Checks for binding a unit value. /// @@ -81,18 +73,6 @@ declare_clippy_lint! { "comparing unit values" } -declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP]); - -impl LateLintPass<'_> for UnitTypes { - fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { - let_unit_value::check(cx, stmt); - } - - fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - unit_cmp::check(cx, expr); - } -} - declare_clippy_lint! { /// **What it does:** Checks for passing a unit value as an argument to a function without using a /// unit literal (`()`). @@ -113,205 +93,15 @@ declare_clippy_lint! { "passing unit to a function" } -declare_lint_pass!(UnitArg => [UNIT_ARG]); - -impl<'tcx> LateLintPass<'tcx> for UnitArg { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if expr.span.from_expansion() { - return; - } - - // apparently stuff in the desugaring of `?` can trigger this - // so check for that here - // only the calls to `Try::from_error` is marked as desugared, - // so we need to check both the current Expr and its parent. - if is_questionmark_desugar_marked_call(expr) { - return; - } - if_chain! { - let map = &cx.tcx.hir(); - let opt_parent_node = map.find(map.get_parent_node(expr.hir_id)); - if let Some(hir::Node::Expr(parent_expr)) = opt_parent_node; - if is_questionmark_desugar_marked_call(parent_expr); - then { - return; - } - } - - match expr.kind { - ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => { - let args_to_recover = args - .iter() - .filter(|arg| { - if is_unit(cx.typeck_results().expr_ty(arg)) && !is_unit_literal(arg) { - !matches!( - &arg.kind, - ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) - ) - } else { - false - } - }) - .collect::>(); - if !args_to_recover.is_empty() { - lint_unit_args(cx, expr, &args_to_recover); - } - }, - _ => (), - } - } -} +declare_lint_pass!(UnitTypes => [LET_UNIT_VALUE, UNIT_CMP, UNIT_ARG]); -fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { - use rustc_span::hygiene::DesugaringKind; - if let ExprKind::Call(ref callee, _) = expr.kind { - callee.span.is_desugaring(DesugaringKind::QuestionMark) - } else { - false +impl LateLintPass<'_> for UnitTypes { + fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &Stmt<'_>) { + let_unit_value::check(cx, stmt); } -} - -fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { - let mut applicability = Applicability::MachineApplicable; - let (singular, plural) = if args_to_recover.len() > 1 { - ("", "s") - } else { - ("a ", "") - }; - span_lint_and_then( - cx, - UNIT_ARG, - expr.span, - &format!("passing {}unit value{} to a function", singular, plural), - |db| { - let mut or = ""; - args_to_recover - .iter() - .filter_map(|arg| { - if_chain! { - if let ExprKind::Block(block, _) = arg.kind; - if block.expr.is_none(); - if let Some(last_stmt) = block.stmts.iter().last(); - if let StmtKind::Semi(last_expr) = last_stmt.kind; - if let Some(snip) = snippet_opt(cx, last_expr.span); - then { - Some(( - last_stmt.span, - snip, - )) - } - else { - None - } - } - }) - .for_each(|(span, sugg)| { - db.span_suggestion( - span, - "remove the semicolon from the last statement in the block", - sugg, - Applicability::MaybeIncorrect, - ); - or = "or "; - applicability = Applicability::MaybeIncorrect; - }); - - let arg_snippets: Vec = args_to_recover - .iter() - .filter_map(|arg| snippet_opt(cx, arg.span)) - .collect(); - let arg_snippets_without_empty_blocks: Vec = args_to_recover - .iter() - .filter(|arg| !is_empty_block(arg)) - .filter_map(|arg| snippet_opt(cx, arg.span)) - .collect(); - - if let Some(call_snippet) = snippet_opt(cx, expr.span) { - let sugg = fmt_stmts_and_call( - cx, - expr, - &call_snippet, - &arg_snippets, - &arg_snippets_without_empty_blocks, - ); - if arg_snippets_without_empty_blocks.is_empty() { - db.multipart_suggestion( - &format!("use {}unit literal{} instead", singular, plural), - args_to_recover - .iter() - .map(|arg| (arg.span, "()".to_string())) - .collect::>(), - applicability, - ); - } else { - let plural = arg_snippets_without_empty_blocks.len() > 1; - let empty_or_s = if plural { "s" } else { "" }; - let it_or_them = if plural { "them" } else { "it" }; - db.span_suggestion( - expr.span, - &format!( - "{}move the expression{} in front of the call and replace {} with the unit literal `()`", - or, empty_or_s, it_or_them - ), - sugg, - applicability, - ); - } - } - }, - ); -} - -fn is_empty_block(expr: &Expr<'_>) -> bool { - matches!( - expr.kind, - ExprKind::Block( - Block { - stmts: &[], - expr: None, - .. - }, - _, - ) - ) -} - -fn fmt_stmts_and_call( - cx: &LateContext<'_>, - call_expr: &Expr<'_>, - call_snippet: &str, - args_snippets: &[impl AsRef], - non_empty_block_args_snippets: &[impl AsRef], -) -> String { - let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0); - let call_snippet_with_replacements = args_snippets - .iter() - .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1)); - - let mut stmts_and_call = non_empty_block_args_snippets - .iter() - .map(|it| it.as_ref().to_owned()) - .collect::>(); - stmts_and_call.push(call_snippet_with_replacements); - stmts_and_call = stmts_and_call - .into_iter() - .map(|v| reindent_multiline(v.into(), true, Some(call_expr_indent)).into_owned()) - .collect(); - - let mut stmts_and_call_snippet = stmts_and_call.join(&format!("{}{}", ";\n", " ".repeat(call_expr_indent))); - // expr is not in a block statement or result expression position, wrap in a block - let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(call_expr.hir_id)); - if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) { - let block_indent = call_expr_indent + 4; - stmts_and_call_snippet = - reindent_multiline(stmts_and_call_snippet.into(), true, Some(block_indent)).into_owned(); - stmts_and_call_snippet = format!( - "{{\n{}{}\n{}}}", - " ".repeat(block_indent), - &stmts_and_call_snippet, - " ".repeat(call_expr_indent) - ); + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + unit_cmp::check(cx, expr); + unit_arg::check(cx, expr); } - stmts_and_call_snippet } diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs new file mode 100644 index 0000000000000..49b5725f528dd --- /dev/null +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -0,0 +1,209 @@ +use rustc_errors::Applicability; +use rustc_hir::{self as hir, Block, Expr, ExprKind, MatchSource, Node, StmtKind}; +use rustc_lint::LateContext; + +use if_chain::if_chain; + +use crate::utils::diagnostics::span_lint_and_then; +use crate::utils::source::{indent_of, reindent_multiline, snippet_opt}; + +use super::{utils, UNIT_ARG}; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { + if expr.span.from_expansion() { + return; + } + + // apparently stuff in the desugaring of `?` can trigger this + // so check for that here + // only the calls to `Try::from_error` is marked as desugared, + // so we need to check both the current Expr and its parent. + if is_questionmark_desugar_marked_call(expr) { + return; + } + if_chain! { + let map = &cx.tcx.hir(); + let opt_parent_node = map.find(map.get_parent_node(expr.hir_id)); + if let Some(hir::Node::Expr(parent_expr)) = opt_parent_node; + if is_questionmark_desugar_marked_call(parent_expr); + then { + return; + } + } + + match expr.kind { + ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) => { + let args_to_recover = args + .iter() + .filter(|arg| { + if utils::is_unit(cx.typeck_results().expr_ty(arg)) && !utils::is_unit_literal(arg) { + !matches!( + &arg.kind, + ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) + ) + } else { + false + } + }) + .collect::>(); + if !args_to_recover.is_empty() { + lint_unit_args(cx, expr, &args_to_recover); + } + }, + _ => (), + } +} + +fn is_questionmark_desugar_marked_call(expr: &Expr<'_>) -> bool { + use rustc_span::hygiene::DesugaringKind; + if let ExprKind::Call(ref callee, _) = expr.kind { + callee.span.is_desugaring(DesugaringKind::QuestionMark) + } else { + false + } +} + +fn lint_unit_args(cx: &LateContext<'_>, expr: &Expr<'_>, args_to_recover: &[&Expr<'_>]) { + let mut applicability = Applicability::MachineApplicable; + let (singular, plural) = if args_to_recover.len() > 1 { + ("", "s") + } else { + ("a ", "") + }; + span_lint_and_then( + cx, + UNIT_ARG, + expr.span, + &format!("passing {}unit value{} to a function", singular, plural), + |db| { + let mut or = ""; + args_to_recover + .iter() + .filter_map(|arg| { + if_chain! { + if let ExprKind::Block(block, _) = arg.kind; + if block.expr.is_none(); + if let Some(last_stmt) = block.stmts.iter().last(); + if let StmtKind::Semi(last_expr) = last_stmt.kind; + if let Some(snip) = snippet_opt(cx, last_expr.span); + then { + Some(( + last_stmt.span, + snip, + )) + } + else { + None + } + } + }) + .for_each(|(span, sugg)| { + db.span_suggestion( + span, + "remove the semicolon from the last statement in the block", + sugg, + Applicability::MaybeIncorrect, + ); + or = "or "; + applicability = Applicability::MaybeIncorrect; + }); + + let arg_snippets: Vec = args_to_recover + .iter() + .filter_map(|arg| snippet_opt(cx, arg.span)) + .collect(); + let arg_snippets_without_empty_blocks: Vec = args_to_recover + .iter() + .filter(|arg| !is_empty_block(arg)) + .filter_map(|arg| snippet_opt(cx, arg.span)) + .collect(); + + if let Some(call_snippet) = snippet_opt(cx, expr.span) { + let sugg = fmt_stmts_and_call( + cx, + expr, + &call_snippet, + &arg_snippets, + &arg_snippets_without_empty_blocks, + ); + + if arg_snippets_without_empty_blocks.is_empty() { + db.multipart_suggestion( + &format!("use {}unit literal{} instead", singular, plural), + args_to_recover + .iter() + .map(|arg| (arg.span, "()".to_string())) + .collect::>(), + applicability, + ); + } else { + let plural = arg_snippets_without_empty_blocks.len() > 1; + let empty_or_s = if plural { "s" } else { "" }; + let it_or_them = if plural { "them" } else { "it" }; + db.span_suggestion( + expr.span, + &format!( + "{}move the expression{} in front of the call and replace {} with the unit literal `()`", + or, empty_or_s, it_or_them + ), + sugg, + applicability, + ); + } + } + }, + ); +} + +fn is_empty_block(expr: &Expr<'_>) -> bool { + matches!( + expr.kind, + ExprKind::Block( + Block { + stmts: &[], + expr: None, + .. + }, + _, + ) + ) +} + +fn fmt_stmts_and_call( + cx: &LateContext<'_>, + call_expr: &Expr<'_>, + call_snippet: &str, + args_snippets: &[impl AsRef], + non_empty_block_args_snippets: &[impl AsRef], +) -> String { + let call_expr_indent = indent_of(cx, call_expr.span).unwrap_or(0); + let call_snippet_with_replacements = args_snippets + .iter() + .fold(call_snippet.to_owned(), |acc, arg| acc.replacen(arg.as_ref(), "()", 1)); + + let mut stmts_and_call = non_empty_block_args_snippets + .iter() + .map(|it| it.as_ref().to_owned()) + .collect::>(); + stmts_and_call.push(call_snippet_with_replacements); + stmts_and_call = stmts_and_call + .into_iter() + .map(|v| reindent_multiline(v.into(), true, Some(call_expr_indent)).into_owned()) + .collect(); + + let mut stmts_and_call_snippet = stmts_and_call.join(&format!("{}{}", ";\n", " ".repeat(call_expr_indent))); + // expr is not in a block statement or result expression position, wrap in a block + let parent_node = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(call_expr.hir_id)); + if !matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_))) { + let block_indent = call_expr_indent + 4; + stmts_and_call_snippet = + reindent_multiline(stmts_and_call_snippet.into(), true, Some(block_indent)).into_owned(); + stmts_and_call_snippet = format!( + "{{\n{}{}\n{}}}", + " ".repeat(block_indent), + &stmts_and_call_snippet, + " ".repeat(call_expr_indent) + ); + } + stmts_and_call_snippet +} From 5a439f5a82815c029c8b2bfa28d9525aaac1c320 Mon Sep 17 00:00:00 2001 From: Yoshitomo Nakanishi Date: Tue, 16 Mar 2021 10:37:05 +0900 Subject: [PATCH 6/6] Remove unit_types::utils::is_unit --- clippy_lints/src/unit_types/let_unit_value.rs | 4 ++-- clippy_lints/src/unit_types/unit_arg.rs | 2 +- clippy_lints/src/unit_types/unit_cmp.rs | 6 +++--- clippy_lints/src/unit_types/utils.rs | 5 ----- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/unit_types/let_unit_value.rs b/clippy_lints/src/unit_types/let_unit_value.rs index 126f6aa746adc..55715cc6bf39c 100644 --- a/clippy_lints/src/unit_types/let_unit_value.rs +++ b/clippy_lints/src/unit_types/let_unit_value.rs @@ -7,11 +7,11 @@ use crate::utils::diagnostics::span_lint_and_then; use crate::utils::higher; use crate::utils::source::snippet_with_macro_callsite; -use super::{utils, LET_UNIT_VALUE}; +use super::LET_UNIT_VALUE; pub(super) fn check(cx: &LateContext<'_>, stmt: &Stmt<'_>) { if let StmtKind::Local(ref local) = stmt.kind { - if utils::is_unit(cx.typeck_results().pat_ty(&local.pat)) { + if cx.typeck_results().pat_ty(&local.pat).is_unit() { if in_external_macro(cx.sess(), stmt.span) || local.pat.span.from_expansion() { return; } diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index 49b5725f528dd..d4ef645fc8d9e 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -36,7 +36,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { let args_to_recover = args .iter() .filter(|arg| { - if utils::is_unit(cx.typeck_results().expr_ty(arg)) && !utils::is_unit_literal(arg) { + if cx.typeck_results().expr_ty(arg).is_unit() && !utils::is_unit_literal(arg) { !matches!( &arg.kind, ExprKind::Match(.., MatchSource::TryDesugar) | ExprKind::Path(..) diff --git a/clippy_lints/src/unit_types/unit_cmp.rs b/clippy_lints/src/unit_types/unit_cmp.rs index e19fc1ec9efc7..f9c0374c86dc1 100644 --- a/clippy_lints/src/unit_types/unit_cmp.rs +++ b/clippy_lints/src/unit_types/unit_cmp.rs @@ -4,7 +4,7 @@ use rustc_span::hygiene::{ExpnKind, MacroKind}; use crate::utils::diagnostics::span_lint; -use super::{utils, UNIT_CMP}; +use super::UNIT_CMP; pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { if expr.span.from_expansion() { @@ -12,7 +12,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { if let ExpnKind::Macro(MacroKind::Bang, symbol) = callee.kind { if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { let op = cmp.node; - if op.is_comparison() && utils::is_unit(cx.typeck_results().expr_ty(left)) { + if op.is_comparison() && cx.typeck_results().expr_ty(left).is_unit() { let result = match &*symbol.as_str() { "assert_eq" | "debug_assert_eq" => "succeed", "assert_ne" | "debug_assert_ne" => "fail", @@ -37,7 +37,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>) { if let ExprKind::Binary(ref cmp, ref left, _) = expr.kind { let op = cmp.node; - if op.is_comparison() && utils::is_unit(cx.typeck_results().expr_ty(left)) { + if op.is_comparison() && cx.typeck_results().expr_ty(left).is_unit() { let result = match op { BinOpKind::Eq | BinOpKind::Le | BinOpKind::Ge => "true", _ => "false", diff --git a/clippy_lints/src/unit_types/utils.rs b/clippy_lints/src/unit_types/utils.rs index a15a7b812c590..4e194a05e8dec 100644 --- a/clippy_lints/src/unit_types/utils.rs +++ b/clippy_lints/src/unit_types/utils.rs @@ -1,9 +1,4 @@ use rustc_hir::{Expr, ExprKind}; -use rustc_middle::ty::{self, Ty}; - -pub(super) fn is_unit(ty: Ty<'_>) -> bool { - matches!(ty.kind(), ty::Tuple(slice) if slice.is_empty()) -} pub(super) fn is_unit_literal(expr: &Expr<'_>) -> bool { matches!(expr.kind, ExprKind::Tup(ref slice) if slice.is_empty())