Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}
107 changes: 107 additions & 0 deletions clippy_lints/src/rest_when_destructuring_struct.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
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(", ..");
}

let message = if a.variants()[vid].fields.is_empty() {
"consider remove rest pattern (`..`)"
} else if fields.is_empty() {
"consider explicitly ignoring fields with wildcard patterns (`x: _`)"
} else {
"consider explicitly ignoring remaining fields with wildcard patterns (`x: _`)"
};

span_lint_and_then(
cx,
REST_WHEN_DESTRUCTURING_STRUCT,
pat.span,
"struct destructuring with rest (`..`)",
|diag| {
diag.span_suggestion_verbose(
dotdot,
message,
fmt_fields,
rustc_errors::Applicability::MachineApplicable,
);
},
);
}
}
}
102 changes: 98 additions & 4 deletions clippy_utils/src/check_proc_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(")")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jarcho these parens should probably be removed, right?

PatKind::Ref(p, _) => {
let (_, end) = pat_search_pat(tcx, p);
(Pat::Str("&"), end)
},
PatKind::Expr(expr) => pat_expr_search_pat(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_expr_search_pat(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_expr_search_pat(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_expr_search_pat(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);
Expand Down Expand Up @@ -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));
Expand Down
7 changes: 7 additions & 0 deletions tests/ui/auxiliary/non-exhaustive-struct.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#[non_exhaustive]
#[derive(Default)]
pub struct NonExhaustiveStruct {
pub field1: i32,
pub field2: i32,
_private: i32,
}
78 changes: 78 additions & 0 deletions tests/ui/rest_when_destructuring_struct.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
//@aux-build:proc_macros.rs
//@aux-build:non-exhaustive-struct.rs
#![warn(clippy::rest_when_destructuring_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
}
Loading