diff --git a/CHANGELOG.md b/CHANGELOG.md index c09310e55568..42361f54140d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3255,6 +3255,7 @@ Released 2018-09-13 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern [`single_component_path_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports [`single_element_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop +[`single_field_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_field_patterns [`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match [`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else [`size_of_in_element_count`]: https://rust-lang.github.io/rust-clippy/master/index.html#size_of_in_element_count diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 3d3999d4cc0d..17e939da9f21 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -253,6 +253,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(self_named_constructors::SELF_NAMED_CONSTRUCTORS), LintId::of(serde_api::SERDE_API_MISUSE), LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), + LintId::of(single_field_patterns::SINGLE_FIELD_PATTERNS), LintId::of(size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT), LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), LintId::of(stable_sort_primitive::STABLE_SORT_PRIMITIVE), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 766c5ba1bcb0..476f66f13a33 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -435,6 +435,7 @@ store.register_lints(&[ shadow::SHADOW_SAME, shadow::SHADOW_UNRELATED, single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS, + single_field_patterns::SINGLE_FIELD_PATTERNS, size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT, slow_vector_initialization::SLOW_VECTOR_INITIALIZATION, stable_sort_primitive::STABLE_SORT_PRIMITIVE, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index ea87e7e7a736..022bccbb9519 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -99,6 +99,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(returns::NEEDLESS_RETURN), LintId::of(self_named_constructors::SELF_NAMED_CONSTRUCTORS), LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), + LintId::of(single_field_patterns::SINGLE_FIELD_PATTERNS), LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS), LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME), LintId::of(try_err::TRY_ERR), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d1c7956a7a5c..8e5d4d2813d2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -349,6 +349,7 @@ mod semicolon_if_nothing_returned; mod serde_api; mod shadow; mod single_component_path_imports; +mod single_field_patterns; mod size_of_in_element_count; mod slow_vector_initialization; mod stable_sort_primitive; @@ -853,6 +854,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(trailing_empty_array::TrailingEmptyArray)); store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes)); store.register_late_pass(|| Box::new(needless_late_init::NeedlessLateInit)); + store.register_late_pass(|| Box::new(single_field_patterns::SingleFieldPatterns)); store.register_late_pass(|| Box::new(return_self_not_must_use::ReturnSelfNotMustUse)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/open_options.rs b/clippy_lints/src/open_options.rs index 2c77100bdcfc..3ec75abaaf21 100644 --- a/clippy_lints/src/open_options.rs +++ b/clippy_lints/src/open_options.rs @@ -5,7 +5,7 @@ use rustc_ast::ast::LitKind; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::source_map::{Span, Spanned}; +use rustc_span::source_map::Span; declare_clippy_lint! { /// ### What it does @@ -67,11 +67,7 @@ fn get_open_options(cx: &LateContext<'_>, argument: &Expr<'_>, options: &mut Vec if match_type(cx, obj_ty, &paths::OPEN_OPTIONS) && arguments.len() >= 2 { let argument_option = match arguments[1].kind { ExprKind::Lit(ref span) => { - if let Spanned { - node: LitKind::Bool(lit), - .. - } = *span - { + if let LitKind::Bool(lit) = span.node { if lit { Argument::True } else { Argument::False } } else { // The function is called with a literal which is not a boolean literal. diff --git a/clippy_lints/src/single_field_patterns.rs b/clippy_lints/src/single_field_patterns.rs new file mode 100644 index 000000000000..29a455b703a9 --- /dev/null +++ b/clippy_lints/src/single_field_patterns.rs @@ -0,0 +1,263 @@ +#![allow(rustc::usage_of_ty_tykind)] + +use clippy_utils::{ + diagnostics::{multispan_sugg_with_applicability, span_lint_and_then}, + higher::IfLetOrMatch, + higher::WhileLet, + source::snippet_opt, +}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Local, MatchSource, Pat, PatKind, Stmt, StmtKind, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{symbol::Ident, Span}; +use std::iter; + +declare_clippy_lint! { + /// ### What it does + /// Checks for patterns that only match a single field of a struct when they could directly access the field. + /// + /// ### Why is this bad? + /// It requires more text and more information than directly accessing the field. + /// + /// ### Example + /// ```rust + /// # struct Struct { + /// # field1: Option, + /// # field2: Option, + /// # } + /// # fn foo(struct1: Struct) { + /// // bad: + /// match struct1 { + /// Struct { field1: Some(n), .. } if n >= 50 => {}, + /// Struct { field1: None, .. } => {}, + /// _ => {}, + /// } + /// // better: + /// match struct1.field1 { + /// Some(n) if n >= 50 => {}, + /// None => {}, + /// _ => {}, + /// } + /// # } + /// ``` + #[clippy::version = "1.59.0"] + pub SINGLE_FIELD_PATTERNS, + style, + "single-field patterns" +} +declare_lint_pass!(SingleFieldPatterns => [SINGLE_FIELD_PATTERNS]); + +/// This represents 0 or 1 fields being used. Where more may be used, `Option` is used +/// where `None` represents the absence of a lint +#[derive(Debug, Clone, Copy)] +enum SingleField { + Id { id: Ident, pattern: Span }, + Unused, // The name "SingleField" is a lie but idk what's better. "AtMostOneField"? +} + +impl PartialEq for SingleField { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (SingleField::Id { id: id1, .. }, SingleField::Id { id: id2, .. }) => id1 == id2, + (SingleField::Unused, SingleField::Unused) => true, + _ => false, + } + } +} + +impl SingleField { + fn new<'a>(mut iter: impl Iterator)>) -> Option { + let one = iter.by_ref().find(|(_, pat)| !matches!(pat.kind, PatKind::Wild)); + match one { + Some((id, pat)) => { + if pat.span.from_expansion() { + return None; + } + if iter.all(|(_, other)| matches!(other.kind, PatKind::Wild)) { + Some(SingleField::Id { id, pattern: pat.span }) + } else { + None + } + }, + None => Some(SingleField::Unused), + } + } +} + +/// This handles recursive patterns and flattens them out lazily +/// e.g. 1 | (2 | 9) | 3..5 +struct FlatPatterns<'hir, I> +where + I: Iterator>, +{ + patterns: I, + stack: Vec<&'hir Pat<'hir>>, +} + +impl>> FlatPatterns<'hir, I> { + fn new(patterns: I) -> Self { + Self { + patterns, + stack: Vec::new(), + } + } +} + +impl>> Iterator for FlatPatterns<'hir, I> { + type Item = &'hir Pat<'hir>; + + fn next(&mut self) -> Option { + if self.stack.is_empty() { + if let Some(pat) = self.patterns.next() { + self.stack.push(pat); + } else { + return None; + } + } + while let Some(pat) = self.stack.pop() { + match pat.kind { + PatKind::Or(pats) => self.stack.extend(pats), + _ => return Some(pat), + } + } + unreachable!("Or always has 2 patterns, so one of the prior returns must return"); + } +} + +fn find_sf_lint<'hir>( + cx: &LateContext<'_>, + patterns: impl Iterator>, +) -> Option<(SingleField, Vec<(Span, String)>)> { + let fields = FlatPatterns::new(patterns).map(|p| { + ( + p.span, + match p.kind { + PatKind::Wild => Some(SingleField::Unused), + PatKind::Struct(_, pats, _) => SingleField::new(pats.iter().map(|field| (field.ident, field.pat))), + _ => None, + }, + ) + }); + let mut spans = Vec::<(Span, String)>::new(); + let mut the_one: Option = None; + for (target, sf) in fields { + if target.from_expansion() { + return None; + } + if let Some(sf) = sf { + match sf { + SingleField::Unused => { + spans.push((target, String::from("_"))); + }, + SingleField::Id { pattern, .. } => { + if let Some(str) = snippet_opt(cx, pattern) { + spans.push((target, str)); + } else { + return None; + } + if let Some(one) = the_one { + if sf != one { + return None; + } + } else { + the_one = Some(sf); + } + }, + } + } else { + return None; + } + } + the_one.map(|one| (one, spans)) +} + +fn apply_lint_sf(cx: &LateContext<'_>, span: Span, sugg: impl IntoIterator) { + span_lint_and_then( + cx, + SINGLE_FIELD_PATTERNS, + span, + "this single-variant pattern only matches one field", + |diag| { + multispan_sugg_with_applicability( + diag, + "try accessing this field directly", + Applicability::MaybeIncorrect, + sugg, + ); + }, + ); +} + +fn remove_deref<'a>(mut scrutinee: &'a Expr<'a>) -> &'a Expr<'a> { + // it would be wrong to convert something like `if let (x, _) = *a` into `if let x = *a.0` + while let ExprKind::Unary(UnOp::Deref, expr) = scrutinee.kind { + scrutinee = expr; + } + scrutinee +} + +fn lint_sf<'hir>( + cx: &LateContext<'_>, + overall_span: Span, + scrutinee: &Expr<'_>, + patterns: impl Iterator>, +) { + if scrutinee.span.from_expansion() { + return; + } + let scrutinee_name = if let Some(name) = snippet_opt(cx, remove_deref(scrutinee).span) { + name + } else { + return; + }; + match cx.typeck_results().expr_ty(scrutinee).kind() { + ty::Adt(def, ..) if def.is_struct() => { + if let Some((field, mut spans)) = find_sf_lint(cx, patterns) { + spans.push(( + scrutinee.span, + match field { + SingleField::Id { id, .. } => format!("{}.{}", scrutinee_name, id.as_str()), + SingleField::Unused => return, + }, + )); + apply_lint_sf(cx, overall_span, spans); + } + }, + _ => (), + }; +} + +impl LateLintPass<'_> for SingleFieldPatterns { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + match IfLetOrMatch::parse(cx, expr) { + Some(IfLetOrMatch::Match(scrutinee, arms, MatchSource::Normal)) => { + lint_sf(cx, expr.span, scrutinee, arms.iter().map(|arm| arm.pat)); + }, + Some(IfLetOrMatch::IfLet(scrutinee, pat, ..)) => lint_sf(cx, expr.span, scrutinee, iter::once(pat)), + _ => { + if let Some(WhileLet { let_pat, let_expr, .. }) = WhileLet::hir(expr) { + lint_sf(cx, expr.span, let_expr, iter::once(let_pat)); + } + }, + }; + } + + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &Stmt<'tcx>) { + if stmt.span.from_expansion() { + return; + } + if let StmtKind::Local(Local { + pat, + init: Some(scrutinee), + .. + }) = stmt.kind + { + lint_sf(cx, stmt.span, scrutinee, iter::once(*pat)); + } + } +} diff --git a/tests/ui/author/struct.rs b/tests/ui/author/struct.rs index 5fdf3433a370..dbe3cfcf37e6 100644 --- a/tests/ui/author/struct.rs +++ b/tests/ui/author/struct.rs @@ -1,4 +1,4 @@ -#[allow(clippy::unnecessary_operation, clippy::single_match)] +#[allow(clippy::unnecessary_operation, clippy::single_match, clippy::single_field_patterns)] fn main() { struct Test { field: u32, diff --git a/tests/ui/single_field_patterns.rs b/tests/ui/single_field_patterns.rs new file mode 100644 index 000000000000..06594593d711 --- /dev/null +++ b/tests/ui/single_field_patterns.rs @@ -0,0 +1,132 @@ +#![warn(clippy::single_field_patterns)] +struct Struct { + field1: Option, + field2: Option, +} + +fn lint_struct(struct1: Struct) { + let Struct { field1, .. } = struct1; + let Struct { field1, field2: _ } = struct1; + if let Struct { field1: None, .. } | Struct { field1: Some(1), .. } = struct1 {} + match struct1 { + Struct { field1: Some(n), .. } if n >= 50 => {}, + Struct { field1: None, .. } => {}, + _ => {}, + } + match struct1 { + Struct { field1: Some(n), .. } if n >= 50 => {}, + Struct { .. } | Struct { field1: None, .. } => {}, + _ => {}, + } + match struct1 { + Struct { field1: Some(1), .. } => {}, + Struct { field1: Some(2), .. } => {}, + Struct { field1: None, .. } => {}, + _ => {}, + } + match struct1 { + Struct { + field1: Some(_) | None, .. + } => {}, + } + while let Struct { field1: Some(5), .. } = struct1 {} +} + +fn lint_ref(struct1: &mut &mut Struct) { + // this should suggest struct1.field1, NOT **struct1.field1 + let Struct { field1, .. } = **struct1; + let Struct { ref field1, .. } = **struct1; + let Struct { ref mut field1, .. } = **struct1; +} + +macro_rules! mac { + () => { + Struct { + field1: Some(1), + field2: Some(2), + } + }; +} + +macro_rules! pat { + ($id:ident) => { + Struct { field1: $id, field2: _ } + }; +} + +fn ok_macro() { + let Struct { .. } = mac!(); + let pat!(a) = Struct { + field1: None, + field2: None, + }; + let (pat!(a), _) = (mac!(), mac!()); +} + +fn ok_struct(struct1: Struct) { + let _ = struct1; + let Struct { field1, field2, .. } = struct1; + let Struct { field1, field2 } = struct1; + match struct1 { + Struct { + field1: Some(1), + field2: _, + } => {}, + Struct { + field1: _, + field2: Some(1), + } => {}, + _ => {}, + } + match struct1 { + Struct { field1: Some(1), .. } => {}, + Struct { + field1: _, + field2: Some(1), + } => {}, + _ => {}, + } + let s @ Struct { field1, .. } = struct1; +} + +struct Tuple(Option, Option); + +fn ok_tuple_struct(tuple: Tuple) { + if let Tuple(Some(x), _) = tuple {} + if let Tuple(_, Some(y)) = tuple {} + match tuple { + Tuple(Some(1), ..) => {}, + Tuple(Some(2), ..) => {}, + Tuple(a, ..) => {}, + } + match tuple { + Tuple(Some(1) | Some(42) | Some(6082), ..) => {}, + Tuple(a, ..) => {}, + } +} + +fn ok_tuple(tuple: (Option, Option)) { + if let (Some(z), _) = tuple {} + if let (_, Some(n)) = tuple {} + match tuple { + (Some(1), ..) => {}, + (Some(2), ..) => {}, + (a, ..) => {}, + } + match tuple { + (Some(1) | Some(42) | Some(6082), ..) => {}, + (a, ..) => {}, + } +} + +fn ok_array(array: [i32; 3]) { + match array { + [1 | 2, ..] => {}, + [x @ 3, ..] => {}, + [r @ 20..=65, ..] => {}, + [e, ..] => {}, + } + if let [5, ..] = array {} +} + +fn main() {} diff --git a/tests/ui/single_field_patterns.stderr b/tests/ui/single_field_patterns.stderr new file mode 100644 index 000000000000..5664055e8899 --- /dev/null +++ b/tests/ui/single_field_patterns.stderr @@ -0,0 +1,152 @@ +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:8:5 + | +LL | let Struct { field1, .. } = struct1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::single-field-patterns` implied by `-D warnings` +help: try accessing this field directly + | +LL | let field1 = struct1.field1; + | ~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:9:5 + | +LL | let Struct { field1, field2: _ } = struct1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | let field1 = struct1.field1; + | ~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:10:5 + | +LL | if let Struct { field1: None, .. } | Struct { field1: Some(1), .. } = struct1 {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | if let None | Some(1) = struct1.field1 {} + | ~~~~ ~~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:11:5 + | +LL | / match struct1 { +LL | | Struct { field1: Some(n), .. } if n >= 50 => {}, +LL | | Struct { field1: None, .. } => {}, +LL | | _ => {}, +LL | | } + | |_____^ + | +help: try accessing this field directly + | +LL ~ match struct1.field1 { +LL ~ Some(n) if n >= 50 => {}, +LL ~ None => {}, +LL ~ _ => {}, + | + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:16:5 + | +LL | / match struct1 { +LL | | Struct { field1: Some(n), .. } if n >= 50 => {}, +LL | | Struct { .. } | Struct { field1: None, .. } => {}, +LL | | _ => {}, +LL | | } + | |_____^ + | +help: try accessing this field directly + | +LL ~ match struct1.field1 { +LL ~ Some(n) if n >= 50 => {}, +LL ~ _ | None => {}, +LL ~ _ => {}, + | + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:21:5 + | +LL | / match struct1 { +LL | | Struct { field1: Some(1), .. } => {}, +LL | | Struct { field1: Some(2), .. } => {}, +LL | | Struct { field1: None, .. } => {}, +LL | | _ => {}, +LL | | } + | |_____^ + | +help: try accessing this field directly + | +LL ~ match struct1.field1 { +LL ~ Some(1) => {}, +LL ~ Some(2) => {}, +LL ~ None => {}, +LL ~ _ => {}, + | + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:27:5 + | +LL | / match struct1 { +LL | | Struct { +LL | | field1: Some(_) | None, .. +LL | | } => {}, +LL | | } + | |_____^ + | +help: try accessing this field directly + | +LL ~ match struct1.field1 { +LL ~ Some(_) | None => {}, + | + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:32:5 + | +LL | while let Struct { field1: Some(5), .. } = struct1 {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | while let Some(5) = struct1.field1 {} + | ~~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:37:5 + | +LL | let Struct { field1, .. } = **struct1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | let field1 = struct1.field1; + | ~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:38:5 + | +LL | let Struct { ref field1, .. } = **struct1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | let ref field1 = struct1.field1; + | ~~~~~~~~~~ ~~~~~~~~~~~~~~ + +error: this single-variant pattern only matches one field + --> $DIR/single_field_patterns.rs:39:5 + | +LL | let Struct { ref mut field1, .. } = **struct1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try accessing this field directly + | +LL | let ref mut field1 = struct1.field1; + | ~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~ + +error: aborting due to 11 previous errors + diff --git a/tests/ui/unneeded_field_pattern.rs b/tests/ui/unneeded_field_pattern.rs index fa639aa70d61..6dc86fb11166 100644 --- a/tests/ui/unneeded_field_pattern.rs +++ b/tests/ui/unneeded_field_pattern.rs @@ -1,4 +1,5 @@ #![warn(clippy::unneeded_field_pattern)] +#![allow(clippy::single_field_patterns)] #[allow(dead_code, unused)] struct Foo { diff --git a/tests/ui/unneeded_field_pattern.stderr b/tests/ui/unneeded_field_pattern.stderr index b8d3c2945322..5614e82207a8 100644 --- a/tests/ui/unneeded_field_pattern.stderr +++ b/tests/ui/unneeded_field_pattern.stderr @@ -1,5 +1,5 @@ error: you matched a field with a wildcard pattern, consider using `..` instead - --> $DIR/unneeded_field_pattern.rs:14:15 + --> $DIR/unneeded_field_pattern.rs:15:15 | LL | Foo { a: _, b: 0, .. } => {}, | ^^^^ @@ -8,7 +8,7 @@ LL | Foo { a: _, b: 0, .. } => {}, = help: try with `Foo { b: 0, .. }` error: all the struct fields are matched to a wildcard pattern, consider using `..` - --> $DIR/unneeded_field_pattern.rs:16:9 + --> $DIR/unneeded_field_pattern.rs:17:9 | LL | Foo { a: _, b: _, c: _ } => {}, | ^^^^^^^^^^^^^^^^^^^^^^^^