X-Git-Url: https://git.lizzy.rs/?a=blobdiff_plain;f=clippy_lints%2Fsrc%2Fmisc_early.rs;h=7f70de01daaacbbd7970640e29a4cbf981c421b5;hb=e5a5b0a0774625eebbe7b29c67b49dc6431544d1;hp=c8ac26044ebc06cca21d01a1115da2e1eac22c94;hpb=835205b8da86a1eeee07efe13bedf7fec1aa7cbc;p=rust.git diff --git a/clippy_lints/src/misc_early.rs b/clippy_lints/src/misc_early.rs index c8ac26044eb..7f70de01daa 100644 --- a/clippy_lints/src/misc_early.rs +++ b/clippy_lints/src/misc_early.rs @@ -1,12 +1,13 @@ use crate::utils::{ - constants, snippet, snippet_opt, span_help_and_lint, span_lint, span_lint_and_sugg, span_lint_and_then, + constants, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg, + span_lint_and_then, }; use if_chain::if_chain; +use rustc::declare_lint_pass; use rustc::lint::{in_external_macro, EarlyContext, EarlyLintPass, LintArray, LintContext, LintPass}; -use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_data_structures::fx::FxHashMap; use rustc_errors::Applicability; -use std::char; +use rustc_session::declare_tool_lint; use syntax::ast::*; use syntax::source_map::Span; use syntax::visit::{walk_expr, FnKind, Visitor}; @@ -174,6 +175,66 @@ "shadowing a builtin type" } +declare_clippy_lint! { + /// **What it does:** Checks for patterns in the form `name @ _`. + /// + /// **Why is this bad?** It's almost always more readable to just use direct + /// bindings. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # let v = Some("abc"); + /// + /// match v { + /// Some(x) => (), + /// y @ _ => (), // easier written as `y`, + /// } + /// ``` + pub REDUNDANT_PATTERN, + style, + "using `name @ _` in a pattern" +} + +declare_clippy_lint! { + /// **What it does:** Checks for tuple patterns with a wildcard + /// pattern (`_`) is next to a rest pattern (`..`). + /// + /// _NOTE_: While `_, ..` means there is at least one element left, `..` + /// means there are 0 or more elements left. This can make a difference + /// when refactoring, but shouldn't result in errors in the refactored code, + /// since the wildcard pattern isn't used anyway. + /// **Why is this bad?** The wildcard pattern is unneeded as the rest pattern + /// can match that element as well. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # struct TupleStruct(u32, u32, u32); + /// # let t = TupleStruct(1, 2, 3); + /// + /// match t { + /// TupleStruct(0, .., _) => (), + /// _ => (), + /// } + /// ``` + /// can be written as + /// ```rust + /// # struct TupleStruct(u32, u32, u32); + /// # let t = TupleStruct(1, 2, 3); + /// + /// match t { + /// TupleStruct(0, ..) => (), + /// _ => (), + /// } + /// ``` + pub UNNEEDED_WILDCARD_PATTERN, + complexity, + "tuple patterns with a wildcard pattern (`_`) is next to a rest pattern (`..`)" +} + declare_lint_pass!(MiscEarlyLints => [ UNNEEDED_FIELD_PATTERN, DUPLICATE_UNDERSCORE_ARGUMENT, @@ -182,7 +243,9 @@ MIXED_CASE_HEX_LITERALS, UNSEPARATED_LITERAL_SUFFIX, ZERO_PREFIXED_LITERAL, - BUILTIN_TYPE_SHADOW + BUILTIN_TYPE_SHADOW, + REDUNDANT_PATTERN, + UNNEEDED_WILDCARD_PATTERN, ]); // Used to find `return` statements or equivalents e.g., `?` @@ -191,6 +254,7 @@ struct ReturnVisitor { } impl ReturnVisitor { + #[must_use] fn new() -> Self { Self { found_return: false } } @@ -198,9 +262,9 @@ fn new() -> Self { impl<'ast> Visitor<'ast> for ReturnVisitor { fn visit_expr(&mut self, ex: &'ast Expr) { - if let ExprKind::Ret(_) = ex.node { + if let ExprKind::Ret(_) = ex.kind { self.found_return = true; - } else if let ExprKind::Try(_) = ex.node { + } else if let ExprKind::Try(_) = ex.kind { self.found_return = true; } @@ -226,7 +290,7 @@ fn check_generics(&mut self, cx: &EarlyContext<'_>, gen: &Generics) { } fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) { - if let PatKind::Struct(ref npat, ref pfields, _) = pat.node { + if let PatKind::Struct(ref npat, ref pfields, _) = pat.kind { let mut wilds = 0; let type_name = npat .segments @@ -236,7 +300,7 @@ fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) { .name; for field in pfields { - if let PatKind::Wild = field.pat.node { + if let PatKind::Wild = field.pat.kind { wilds += 1; } } @@ -254,7 +318,7 @@ fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) { let mut normal = vec![]; for field in pfields { - match field.pat.node { + match field.pat.kind { PatKind::Wild => {}, _ => { if let Ok(n) = cx.sess().source_map().span_to_snippet(field.span) { @@ -264,7 +328,7 @@ fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) { } } for field in pfields { - if let PatKind::Wild = field.pat.node { + if let PatKind::Wild = field.pat.kind { wilds -= 1; if wilds > 0 { span_lint( @@ -287,13 +351,32 @@ fn check_pat(&mut self, cx: &EarlyContext<'_>, pat: &Pat) { } } } + + if let PatKind::Ident(_, ident, Some(ref right)) = pat.kind { + if let PatKind::Wild = right.kind { + span_lint_and_sugg( + cx, + REDUNDANT_PATTERN, + pat.span, + &format!( + "the `{} @ _` pattern can be written as just `{}`", + ident.name, ident.name, + ), + "try", + format!("{}", ident.name), + Applicability::MachineApplicable, + ); + } + } + + check_unneeded_wildcard_pattern(cx, pat); } fn check_fn(&mut self, cx: &EarlyContext<'_>, _: FnKind<'_>, decl: &FnDecl, _: Span, _: NodeId) { let mut registered_names: FxHashMap = FxHashMap::default(); for arg in &decl.inputs { - if let PatKind::Ident(_, ident, None) = arg.pat.node { + if let PatKind::Ident(_, ident, None) = arg.pat.kind { let arg_name = ident.to_string(); if arg_name.starts_with('_') { @@ -320,10 +403,10 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { if in_external_macro(cx.sess(), expr.span) { return; } - match expr.node { + match expr.kind { ExprKind::Call(ref paren, _) => { - if let ExprKind::Paren(ref closure) = paren.node { - if let ExprKind::Closure(_, _, _, ref decl, ref block, _) = closure.node { + if let ExprKind::Paren(ref closure) = paren.kind { + if let ExprKind::Closure(_, _, _, ref decl, ref block, _) = closure.kind { let mut visitor = ReturnVisitor::new(); visitor.visit_expr(block); if !visitor.found_return { @@ -334,13 +417,10 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { "Try not to call a closure in the expression where it is declared.", |db| { if decl.inputs.is_empty() { - let hint = snippet(cx, block.span, "..").into_owned(); - db.span_suggestion( - expr.span, - "Try doing something like: ", - hint, - Applicability::MachineApplicable, // snippet - ); + let mut app = Applicability::MachineApplicable; + let hint = + snippet_with_applicability(cx, block.span, "..", &mut app).into_owned(); + db.span_suggestion(expr.span, "Try doing something like: ", hint, app); } }, ); @@ -349,7 +429,7 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { } }, ExprKind::Unary(UnOp::Neg, ref inner) => { - if let ExprKind::Unary(UnOp::Neg, _) = inner.node { + if let ExprKind::Unary(UnOp::Neg, _) = inner.kind { span_lint( cx, DOUBLE_NEG, @@ -358,7 +438,7 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { ); } }, - ExprKind::Lit(ref lit) => self.check_lit(cx, lit), + ExprKind::Lit(ref lit) => Self::check_lit(cx, lit), _ => (), } } @@ -366,14 +446,14 @@ fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) { fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) { for w in block.stmts.windows(2) { if_chain! { - if let StmtKind::Local(ref local) = w[0].node; + if let StmtKind::Local(ref local) = w[0].kind; if let Option::Some(ref t) = local.init; - if let ExprKind::Closure(..) = t.node; - if let PatKind::Ident(_, ident, _) = local.pat.node; - if let StmtKind::Semi(ref second) = w[1].node; - if let ExprKind::Assign(_, ref call) = second.node; - if let ExprKind::Call(ref closure, _) = call.node; - if let ExprKind::Path(_, ref path) = closure.node; + if let ExprKind::Closure(..) = t.kind; + if let PatKind::Ident(_, ident, _) = local.pat.kind; + if let StmtKind::Semi(ref second) = w[1].kind; + if let ExprKind::Assign(_, ref call) = second.kind; + if let ExprKind::Call(ref closure, _) = call.kind; + if let ExprKind::Path(_, ref path) = closure.kind; then { if ident == path.segments[0].ident { span_lint( @@ -390,93 +470,144 @@ fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) { } impl MiscEarlyLints { - fn check_lit(self, cx: &EarlyContext<'_>, lit: &Lit) { - if_chain! { - if let LitKind::Int(value, ..) = lit.node; - if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::to_digit(firstch, 10).is_some(); - then { - let mut prev = '\0'; - for (idx, ch) in src.chars().enumerate() { - if ch == 'i' || ch == 'u' { - if prev != '_' { - span_lint_and_sugg( - cx, - UNSEPARATED_LITERAL_SUFFIX, - lit.span, - "integer type suffix should be separated by an underscore", - "add an underscore", - format!("{}_{}", &src[0..idx], &src[idx..]), - Applicability::MachineApplicable, - ); - } - break; - } - prev = ch; - } - if src.starts_with("0x") { - let mut seen = (false, false); - for ch in src.chars() { - match ch { - 'a' ..= 'f' => seen.0 = true, - 'A' ..= 'F' => seen.1 = true, - 'i' | 'u' => break, // start of suffix already - _ => () - } + fn check_lit(cx: &EarlyContext<'_>, lit: &Lit) { + // We test if first character in snippet is a number, because the snippet could be an expansion + // from a built-in macro like `line!()` or a proc-macro like `#[wasm_bindgen]`. + // Note that this check also covers special case that `line!()` is eagerly expanded by compiler. + // See for a regression. + // FIXME: Find a better way to detect those cases. + let lit_snip = match snippet_opt(cx, lit.span) { + Some(snip) if snip.chars().next().map_or(false, |c| c.is_digit(10)) => snip, + _ => return, + }; + + if let LitKind::Int(value, lit_int_type) = lit.kind { + let suffix = match lit_int_type { + LitIntType::Signed(ty) => ty.name_str(), + LitIntType::Unsigned(ty) => ty.name_str(), + LitIntType::Unsuffixed => "", + }; + + let maybe_last_sep_idx = lit_snip.len() - suffix.len() - 1; + // Do not lint when literal is unsuffixed. + if !suffix.is_empty() && lit_snip.as_bytes()[maybe_last_sep_idx] != b'_' { + span_lint_and_sugg( + cx, + UNSEPARATED_LITERAL_SUFFIX, + lit.span, + "integer type suffix should be separated by an underscore", + "add an underscore", + format!("{}_{}", &lit_snip[..=maybe_last_sep_idx], suffix), + Applicability::MachineApplicable, + ); + } + + if lit_snip.starts_with("0x") { + let mut seen = (false, false); + for ch in lit_snip.as_bytes()[2..=maybe_last_sep_idx].iter() { + match ch { + b'a'..=b'f' => seen.0 = true, + b'A'..=b'F' => seen.1 = true, + _ => {}, } if seen.0 && seen.1 { - span_lint(cx, MIXED_CASE_HEX_LITERALS, lit.span, - "inconsistent casing in hexadecimal literal"); + span_lint( + cx, + MIXED_CASE_HEX_LITERALS, + lit.span, + "inconsistent casing in hexadecimal literal", + ); + break; } - } else if src.starts_with("0b") || src.starts_with("0o") { - /* nothing to do */ - } else if value != 0 && src.starts_with('0') { - span_lint_and_then(cx, - ZERO_PREFIXED_LITERAL, - lit.span, - "this is a decimal constant", - |db| { + } + } else if lit_snip.starts_with("0b") || lit_snip.starts_with("0o") { + /* nothing to do */ + } else if value != 0 && lit_snip.starts_with('0') { + span_lint_and_then( + cx, + ZERO_PREFIXED_LITERAL, + lit.span, + "this is a decimal constant", + |db| { db.span_suggestion( lit.span, - "if you mean to use a decimal constant, remove the `0` to remove confusion", - src.trim_start_matches(|c| c == '_' || c == '0').to_string(), + "if you mean to use a decimal constant, remove the `0` to avoid confusion", + lit_snip.trim_start_matches(|c| c == '_' || c == '0').to_string(), Applicability::MaybeIncorrect, ); db.span_suggestion( lit.span, "if you mean to use an octal constant, use `0o`", - format!("0o{}", src.trim_start_matches(|c| c == '_' || c == '0')), + format!("0o{}", lit_snip.trim_start_matches(|c| c == '_' || c == '0')), Applicability::MaybeIncorrect, ); - }); - } + }, + ); + } + } else if let LitKind::Float(_, LitFloatType::Suffixed(float_ty)) = lit.kind { + let suffix = float_ty.name_str(); + let maybe_last_sep_idx = lit_snip.len() - suffix.len() - 1; + if lit_snip.as_bytes()[maybe_last_sep_idx] != b'_' { + span_lint_and_sugg( + cx, + UNSEPARATED_LITERAL_SUFFIX, + lit.span, + "float type suffix should be separated by an underscore", + "add an underscore", + format!("{}_{}", &lit_snip[..=maybe_last_sep_idx], suffix), + Applicability::MachineApplicable, + ); } } - if_chain! { - if let LitKind::Float(..) = lit.node; - if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::to_digit(firstch, 10).is_some(); - then { - let mut prev = '\0'; - for (idx, ch) in src.chars().enumerate() { - if ch == 'f' { - if prev != '_' { - span_lint_and_sugg( - cx, - UNSEPARATED_LITERAL_SUFFIX, - lit.span, - "float type suffix should be separated by an underscore", - "add an underscore", - format!("{}_{}", &src[0..idx], &src[idx..]), - Applicability::MachineApplicable, - ); - } - break; - } - prev = ch; - } + } +} + +fn check_unneeded_wildcard_pattern(cx: &EarlyContext<'_>, pat: &Pat) { + if let PatKind::TupleStruct(_, ref patterns) | PatKind::Tuple(ref patterns) = pat.kind { + fn span_lint(cx: &EarlyContext<'_>, span: Span, only_one: bool) { + span_lint_and_sugg( + cx, + UNNEEDED_WILDCARD_PATTERN, + span, + if only_one { + "this pattern is unneeded as the `..` pattern can match that element" + } else { + "these patterns are unneeded as the `..` pattern can match those elements" + }, + if only_one { "remove it" } else { "remove them" }, + "".to_string(), + Applicability::MachineApplicable, + ); + } + + #[allow(clippy::trivially_copy_pass_by_ref)] + fn is_wild>(pat: &&P) -> bool { + if let PatKind::Wild = pat.kind { + true + } else { + false + } + } + + if let Some(rest_index) = patterns.iter().position(|pat| pat.is_rest()) { + if let Some((left_index, left_pat)) = patterns[..rest_index] + .iter() + .rev() + .take_while(is_wild) + .enumerate() + .last() + { + span_lint(cx, left_pat.span.until(patterns[rest_index].span), left_index == 0); + } + + if let Some((right_index, right_pat)) = + patterns[rest_index + 1..].iter().take_while(is_wild).enumerate().last() + { + span_lint( + cx, + patterns[rest_index].span.shrink_to_hi().to(right_pat.span), + right_index == 0, + ); } } }