diff --git a/CHANGELOG.md b/CHANGELOG.md index 89e3bd336c5a..f96c83860429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6736,6 +6736,7 @@ Released 2018-09-13 [`repr_packed_without_abi`]: https://rust-lang.github.io/rust-clippy/master/index.html#repr_packed_without_abi [`reserve_after_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#reserve_after_initialization [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs +[`rest_when_destructuring_struct`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_when_destructuring_struct [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used [`result_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_filter_map [`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 2a9c6efb52ee..23825fe8a138 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -657,6 +657,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::repeat_vec_with_capacity::REPEAT_VEC_WITH_CAPACITY_INFO, crate::replace_box::REPLACE_BOX_INFO, crate::reserve_after_initialization::RESERVE_AFTER_INITIALIZATION_INFO, + crate::rest_when_destructuring_struct::REST_WHEN_DESTRUCTURING_STRUCT_INFO, crate::return_self_not_must_use::RETURN_SELF_NOT_MUST_USE_INFO, crate::returns::LET_AND_RETURN_INFO, crate::returns::NEEDLESS_RETURN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 149785c59447..ecd30431f300 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -325,6 +325,7 @@ mod regex; mod repeat_vec_with_capacity; mod replace_box; mod reserve_after_initialization; +mod rest_when_destructuring_struct; mod return_self_not_must_use; mod returns; mod same_name_method; @@ -834,5 +835,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg)); store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites)); store.register_late_pass(|_| Box::new(replace_box::ReplaceBox)); + store.register_late_pass(|_| Box::new(rest_when_destructuring_struct::RestWhenDestructuringStruct)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/rest_when_destructuring_struct.rs b/clippy_lints/src/rest_when_destructuring_struct.rs new file mode 100644 index 000000000000..648a3805f02d --- /dev/null +++ b/clippy_lints/src/rest_when_destructuring_struct.rs @@ -0,0 +1,99 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::is_from_proc_macro; +use itertools::Itertools; +use rustc_abi::VariantIdx; +use rustc_lint::LateLintPass; +use rustc_middle::ty; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Disallows the use of rest patterns when destructuring structs. + /// + /// ### Why is this bad? + /// It might lead to unhandled fields when the struct changes. + /// + /// ### Example + /// ```no_run + /// struct S { + /// a: u8, + /// b: u8, + /// c: u8, + /// } + /// + /// let s = S { a: 1, b: 2, c: 3 }; + /// + /// let S { a, b, .. } = s; + /// ``` + /// Use instead: + /// ```no_run + /// struct S { + /// a: u8, + /// b: u8, + /// c: u8, + /// } + /// + /// let s = S { a: 1, b: 2, c: 3 }; + /// + /// let S { a, b, c: _ } = s; + /// ``` + #[clippy::version = "1.89.0"] + pub REST_WHEN_DESTRUCTURING_STRUCT, + restriction, + "rest (..) in destructuring expression" +} +declare_lint_pass!(RestWhenDestructuringStruct => [REST_WHEN_DESTRUCTURING_STRUCT]); + +impl<'tcx> LateLintPass<'tcx> for RestWhenDestructuringStruct { + fn check_pat(&mut self, cx: &rustc_lint::LateContext<'tcx>, pat: &'tcx rustc_hir::Pat<'tcx>) { + if let rustc_hir::PatKind::Struct(path, fields, Some(dotdot)) = pat.kind + && !pat.span.in_external_macro(cx.tcx.sess.source_map()) + && !is_from_proc_macro(cx, pat) + && let qty = cx.typeck_results().qpath_res(&path, pat.hir_id) + && let ty = cx.typeck_results().pat_ty(pat) + && let ty::Adt(a, _) = ty.kind() + { + let vid = qty + .opt_def_id() + .map_or(VariantIdx::ZERO, |x| a.variant_index_with_id(x)); + + let leave_dotdot = a.variants()[vid] + .fields + .iter() + .any(|f| !f.vis.is_accessible_from(cx.tcx.parent_module(pat.hir_id), cx.tcx)); + + let mut rest_fields = a.variants()[vid] + .fields + .iter() + .filter(|f| f.vis.is_accessible_from(cx.tcx.parent_module(pat.hir_id), cx.tcx)) + .filter(|pf| !fields.iter().any(|x| x.ident.name == pf.name)) + .map(|x| format!("{}: _", x.ident(cx.tcx))); + + let mut fmt_fields = rest_fields.join(", "); + + if fmt_fields.is_empty() && leave_dotdot { + // The struct is non_exhaustive, from a non-local crate and all public fields are explicitly named. + return; + } + + if leave_dotdot { + fmt_fields.push_str(", .."); + } + + span_lint_and_then( + cx, + REST_WHEN_DESTRUCTURING_STRUCT, + pat.span, + "struct destructuring with rest (..)", + |diag| { + diag.span_suggestion_verbose( + dotdot, + "consider explicitly ignoring remaining fields with wildcard patterns (x: _)", + fmt_fields, + rustc_errors::Applicability::MachineApplicable, + ); + }, + ); + } + } +} diff --git a/clippy_utils/src/check_proc_macro.rs b/clippy_utils/src/check_proc_macro.rs index ff3e7b94f03b..7da41a9cfdf4 100644 --- a/clippy_utils/src/check_proc_macro.rs +++ b/clippy_utils/src/check_proc_macro.rs @@ -16,15 +16,15 @@ use rustc_abi::ExternAbi; use rustc_ast as ast; use rustc_ast::AttrStyle; use rustc_ast::ast::{ - AttrKind, Attribute, GenericArgs, IntTy, LitIntType, LitKind, StrStyle, TraitObjectSyntax, UintTy, + AttrKind, Attribute, BindingMode, GenericArgs, IntTy, LitIntType, LitKind, StrStyle, TraitObjectSyntax, UintTy, }; use rustc_ast::token::CommentKind; use rustc_hir::intravisit::FnKind; use rustc_hir::{ Block, BlockCheckMode, Body, Closure, Destination, Expr, ExprKind, FieldDef, FnHeader, FnRetTy, HirId, Impl, - ImplItem, ImplItemImplKind, ImplItemKind, IsAuto, Item, ItemKind, Lit, LoopSource, MatchSource, MutTy, Node, Path, - QPath, Safety, TraitImplHeader, TraitItem, TraitItemKind, Ty, TyKind, UnOp, UnsafeSource, Variant, VariantData, - YieldSource, + ImplItem, ImplItemImplKind, ImplItemKind, IsAuto, Item, ItemKind, Lit, LoopSource, MatchSource, MutTy, Node, + PatExpr, PatExprKind, PatKind, Path, QPath, Safety, TraitImplHeader, TraitItem, TraitItemKind, Ty, TyKind, UnOp, + UnsafeSource, Variant, VariantData, YieldSource, }; use rustc_lint::{EarlyContext, LateContext, LintContext}; use rustc_middle::ty::TyCtxt; @@ -541,6 +541,99 @@ fn ident_search_pat(ident: Ident) -> (Pat, Pat) { (Pat::Sym(ident.name), Pat::Sym(ident.name)) } +fn pat_search_pat(tcx: TyCtxt<'_>, pat: &rustc_hir::Pat<'_>) -> (Pat, Pat) { + match pat.kind { + PatKind::Missing | PatKind::Err(_) => (Pat::Str(""), Pat::Str("")), + PatKind::Wild => (Pat::Sym(kw::Underscore), Pat::Sym(kw::Underscore)), + PatKind::Binding(binding_mode, _, ident, Some(end_pat)) => { + let start = if binding_mode == BindingMode::NONE { + ident_search_pat(ident).0 + } else { + Pat::Str(binding_mode.prefix_str()) + }; + + let (_, end) = pat_search_pat(tcx, end_pat); + (start, end) + }, + PatKind::Binding(binding_mode, _, ident, None) => { + let (s, end) = ident_search_pat(ident); + let start = if binding_mode == BindingMode::NONE { + s + } else { + Pat::Str(binding_mode.prefix_str()) + }; + + (start, end) + }, + PatKind::Struct(path, _, _) => { + let (start, _) = qpath_search_pat(&path); + (start, Pat::Str("}")) + }, + PatKind::TupleStruct(path, _, _) => { + let (start, _) = qpath_search_pat(&path); + (start, Pat::Str(")")) + }, + PatKind::Or(plist) => { + // documented invariant + debug_assert!(plist.len() >= 2); + let (start, _) = pat_search_pat(tcx, plist.first().unwrap()); + let (_, end) = pat_search_pat(tcx, plist.last().unwrap()); + (start, end) + }, + PatKind::Never => (Pat::Str("!"), Pat::Str("")), + PatKind::Tuple(_, _) => (Pat::Str("("), Pat::Str(")")), + PatKind::Box(p) => { + let (_, end) = pat_search_pat(tcx, p); + (Pat::Str("box"), end) + }, + PatKind::Deref(_) => (Pat::Str("deref!("), Pat::Str(")")), + PatKind::Ref(p, _) => { + let (_, end) = pat_search_pat(tcx, p); + (Pat::Str("&"), end) + }, + PatKind::Expr(expr) => pat_search_pat_expr_kind(expr), + PatKind::Guard(pat, guard) => { + let (start, _) = pat_search_pat(tcx, pat); + let (_, end) = expr_search_pat(tcx, guard); + (start, end) + }, + PatKind::Range(None, None, range) => match range { + rustc_hir::RangeEnd::Included => (Pat::Str("..="), Pat::Str("")), + rustc_hir::RangeEnd::Excluded => (Pat::Str(".."), Pat::Str("")), + }, + PatKind::Range(r_start, r_end, range) => { + let start = match r_start { + Some(e) => pat_search_pat_expr_kind(e).0, + None => match range { + rustc_hir::RangeEnd::Included => Pat::Str("..="), + rustc_hir::RangeEnd::Excluded => Pat::Str(".."), + }, + }; + + let end = match r_end { + Some(e) => pat_search_pat_expr_kind(e).1, + None => match range { + rustc_hir::RangeEnd::Included => Pat::Str("..="), + rustc_hir::RangeEnd::Excluded => Pat::Str(".."), + }, + }; + (start, end) + }, + PatKind::Slice(_, _, _) => (Pat::Str("["), Pat::Str("]")), + } +} + +fn pat_search_pat_expr_kind(expr: &PatExpr<'_>) -> (Pat, Pat) { + match expr.kind { + PatExprKind::Lit { lit, negated } => { + let (start, end) = lit_search_pat(&lit.node); + if negated { (Pat::Str("!"), end) } else { (start, end) } + }, + PatExprKind::ConstBlock(_block) => (Pat::Str("const {"), Pat::Str("}")), + PatExprKind::Path(path) => qpath_search_pat(&path), + } +} + pub trait WithSearchPat<'cx> { type Context: LintContext; fn search_pat(&self, cx: &Self::Context) -> (Pat, Pat); @@ -569,6 +662,7 @@ impl_with_search_pat!((_cx: LateContext<'tcx>, self: Ty<'_>) => ty_search_pat(se impl_with_search_pat!((_cx: LateContext<'tcx>, self: Ident) => ident_search_pat(*self)); impl_with_search_pat!((_cx: LateContext<'tcx>, self: Lit) => lit_search_pat(&self.node)); impl_with_search_pat!((_cx: LateContext<'tcx>, self: Path<'_>) => path_search_pat(self)); +impl_with_search_pat!((cx: LateContext<'tcx>, self: rustc_hir::Pat<'_>) => pat_search_pat(cx.tcx, self)); impl_with_search_pat!((_cx: EarlyContext<'tcx>, self: Attribute) => attr_search_pat(self)); impl_with_search_pat!((_cx: EarlyContext<'tcx>, self: ast::Ty) => ast_ty_search_pat(self)); diff --git a/tests/ui/auxiliary/non-exhaustive-struct.rs b/tests/ui/auxiliary/non-exhaustive-struct.rs new file mode 100644 index 000000000000..3d944d9775a2 --- /dev/null +++ b/tests/ui/auxiliary/non-exhaustive-struct.rs @@ -0,0 +1,7 @@ +#[non_exhaustive] +#[derive(Default)] +pub struct NonExhaustiveStruct { + pub field1: i32, + pub field2: i32, + _private: i32, +} diff --git a/tests/ui/rest_when_destructuring_struct.fixed b/tests/ui/rest_when_destructuring_struct.fixed new file mode 100644 index 000000000000..31d04558b594 --- /dev/null +++ b/tests/ui/rest_when_destructuring_struct.fixed @@ -0,0 +1,84 @@ +//@aux-build:proc_macros.rs +//@aux-build:non-exhaustive-struct.rs +#![warn(clippy::rest_when_destructuring_struct)] +#![allow(dead_code)] +#![allow(unused_variables)] + +extern crate proc_macros; + +extern crate non_exhaustive_struct; + +use non_exhaustive_struct::NonExhaustiveStruct; + +struct S { + a: u8, + b: u8, + c: u8, +} + +enum E { + A { a1: u8, a2: u8 }, + B { b1: u8, b2: u8 }, + C {}, +} + +mod m { + #[derive(Default)] + pub struct Sm { + pub a: u8, + pub(crate) b: u8, + c: u8, + } +} + +fn main() { + let s = S { a: 1, b: 2, c: 3 }; + + let S { a, b, c: _ } = s; + //~^ rest_when_destructuring_struct + + let S { a, b, c, } = s; + //~^ rest_when_destructuring_struct + + let e = E::A { a1: 1, a2: 2 }; + + match e { + E::A { a1, a2 } => (), + E::B { b1: _, b2: _ } => (), + //~^ rest_when_destructuring_struct + E::C { } => (), + //~^ rest_when_destructuring_struct + } + + match e { + E::A { a1: _, a2: _ } => (), + E::B { b1: _, b2: _ } => (), + //~^ rest_when_destructuring_struct + E::C {} => (), + } + + proc_macros::external! { + let s1 = S { a: 1, b: 2, c: 3 }; + let S { a, b, .. } = s1; + } + + proc_macros::with_span! { + span + let s2 = S { a: 1, b: 2, c: 3 }; + let S { a, b, .. } = s2; + } + + let ne = NonExhaustiveStruct::default(); + let NonExhaustiveStruct { field1: _, field2: _, .. } = ne; + //~^ rest_when_destructuring_struct + + let ne = NonExhaustiveStruct::default(); + let NonExhaustiveStruct { + field1: _, field2: _, .. + } = ne; + + use m::Sm; + + let Sm { a: _, b: _, .. } = Sm::default(); + //~^ rest_when_destructuring_struct +} diff --git a/tests/ui/rest_when_destructuring_struct.rs b/tests/ui/rest_when_destructuring_struct.rs new file mode 100644 index 000000000000..218dd5c7f555 --- /dev/null +++ b/tests/ui/rest_when_destructuring_struct.rs @@ -0,0 +1,84 @@ +//@aux-build:proc_macros.rs +//@aux-build:non-exhaustive-struct.rs +#![warn(clippy::rest_when_destructuring_struct)] +#![allow(dead_code)] +#![allow(unused_variables)] + +extern crate proc_macros; + +extern crate non_exhaustive_struct; + +use non_exhaustive_struct::NonExhaustiveStruct; + +struct S { + a: u8, + b: u8, + c: u8, +} + +enum E { + A { a1: u8, a2: u8 }, + B { b1: u8, b2: u8 }, + C {}, +} + +mod m { + #[derive(Default)] + pub struct Sm { + pub a: u8, + pub(crate) b: u8, + c: u8, + } +} + +fn main() { + let s = S { a: 1, b: 2, c: 3 }; + + let S { a, b, .. } = s; + //~^ rest_when_destructuring_struct + + let S { a, b, c, .. } = s; + //~^ rest_when_destructuring_struct + + let e = E::A { a1: 1, a2: 2 }; + + match e { + E::A { a1, a2 } => (), + E::B { .. } => (), + //~^ rest_when_destructuring_struct + E::C { .. } => (), + //~^ rest_when_destructuring_struct + } + + match e { + E::A { a1: _, a2: _ } => (), + E::B { b1: _, .. } => (), + //~^ rest_when_destructuring_struct + E::C {} => (), + } + + proc_macros::external! { + let s1 = S { a: 1, b: 2, c: 3 }; + let S { a, b, .. } = s1; + } + + proc_macros::with_span! { + span + let s2 = S { a: 1, b: 2, c: 3 }; + let S { a, b, .. } = s2; + } + + let ne = NonExhaustiveStruct::default(); + let NonExhaustiveStruct { field1: _, .. } = ne; + //~^ rest_when_destructuring_struct + + let ne = NonExhaustiveStruct::default(); + let NonExhaustiveStruct { + field1: _, field2: _, .. + } = ne; + + use m::Sm; + + let Sm { .. } = Sm::default(); + //~^ rest_when_destructuring_struct +} diff --git a/tests/ui/rest_when_destructuring_struct.stderr b/tests/ui/rest_when_destructuring_struct.stderr new file mode 100644 index 000000000000..3e6febddf162 --- /dev/null +++ b/tests/ui/rest_when_destructuring_struct.stderr @@ -0,0 +1,86 @@ +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:37:9 + | +LL | let S { a, b, .. } = s; + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::rest-when-destructuring-struct` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::rest_when_destructuring_struct)]` +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL - let S { a, b, .. } = s; +LL + let S { a, b, c: _ } = s; + | + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:40:9 + | +LL | let S { a, b, c, .. } = s; + | ^^^^^^^^^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL - let S { a, b, c, .. } = s; +LL + let S { a, b, c, } = s; + | + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:47:9 + | +LL | E::B { .. } => (), + | ^^^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL - E::B { .. } => (), +LL + E::B { b1: _, b2: _ } => (), + | + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:49:9 + | +LL | E::C { .. } => (), + | ^^^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL - E::C { .. } => (), +LL + E::C { } => (), + | + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:55:9 + | +LL | E::B { b1: _, .. } => (), + | ^^^^^^^^^^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL - E::B { b1: _, .. } => (), +LL + E::B { b1: _, b2: _ } => (), + | + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:72:9 + | +LL | let NonExhaustiveStruct { field1: _, .. } = ne; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL | let NonExhaustiveStruct { field1: _, field2: _, .. } = ne; + | ++++++++++ + +error: struct destructuring with rest (..) + --> tests/ui/rest_when_destructuring_struct.rs:82:9 + | +LL | let Sm { .. } = Sm::default(); + | ^^^^^^^^^ + | +help: consider explicitly ignoring remaining fields with wildcard patterns (x: _) + | +LL | let Sm { a: _, b: _, .. } = Sm::default(); + | +++++++++++ + +error: aborting due to 7 previous errors +