Skip to content

Commit

Permalink
Auto merge of #5550 - ebroto:manual_non_exhaustive, r=flip1995
Browse files Browse the repository at this point in the history
Implement the manual_non_exhaustive lint

Some implementation notes:
* Not providing automatic fixups because additional changes may be needed in other parts of the code, e.g. when constructing a struct.
* Even though the attribute is valid on enum variants, it's not possible to use the manual implementation of the pattern because the visibility is always public, so the lint ignores enum variants.
* Unit structs are also ignored, it's not possible to implement the pattern manually without fields.
* The attribute is not accepted in unions, so those are ignored too.
* Even though the original issue did not mention it, tuple structs are also linted because it's possible to apply the pattern manually.

changelog: Added the manual non-exhaustive implementation lint

Closes #2017
  • Loading branch information
bors committed May 1, 2020
2 parents d270887 + 350c17d commit d9894c1
Show file tree
Hide file tree
Showing 6 changed files with 426 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,7 @@ Released 2018-09-13
[`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports
[`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
[`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap
[`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ mod literal_representation;
mod loops;
mod macro_use;
mod main_recursion;
mod manual_non_exhaustive;
mod map_clone;
mod map_unit_fn;
mod match_on_vec_items;
Expand Down Expand Up @@ -628,6 +629,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&loops::WHILE_LET_ON_ITERATOR,
&macro_use::MACRO_USE_IMPORTS,
&main_recursion::MAIN_RECURSION,
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
&map_clone::MAP_CLONE,
&map_unit_fn::OPTION_MAP_UNIT_FN,
&map_unit_fn::RESULT_MAP_UNIT_FN,
Expand Down Expand Up @@ -1064,6 +1066,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls);
store.register_late_pass(|| box if_let_mutex::IfLetMutex);
store.register_late_pass(|| box match_on_vec_items::MatchOnVecItems);
store.register_early_pass(|| box manual_non_exhaustive::ManualNonExhaustive);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand Down Expand Up @@ -1280,6 +1283,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::WHILE_LET_LOOP),
LintId::of(&loops::WHILE_LET_ON_ITERATOR),
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
LintId::of(&map_clone::MAP_CLONE),
LintId::of(&map_unit_fn::OPTION_MAP_UNIT_FN),
LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
Expand Down Expand Up @@ -1474,6 +1478,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::NEEDLESS_RANGE_LOOP),
LintId::of(&loops::WHILE_LET_ON_ITERATOR),
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
LintId::of(&map_clone::MAP_CLONE),
LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
LintId::of(&matches::MATCH_OVERLAPPING_ARM),
Expand Down
173 changes: 173 additions & 0 deletions clippy_lints/src/manual_non_exhaustive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
use crate::utils::{snippet_opt, span_lint_and_then};
use if_chain::if_chain;
use rustc_ast::ast::{Attribute, Item, ItemKind, StructField, Variant, VariantData, VisibilityKind};
use rustc_attr as attr;
use rustc_errors::Applicability;
use rustc_lint::{EarlyContext, EarlyLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;

declare_clippy_lint! {
/// **What it does:** Checks for manual implementations of the non-exhaustive pattern.
///
/// **Why is this bad?** Using the #[non_exhaustive] attribute expresses better the intent
/// and allows possible optimizations when applied to enums.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// struct S {
/// pub a: i32,
/// pub b: i32,
/// _c: (),
/// }
///
/// enum E {
/// A,
/// B,
/// #[doc(hidden)]
/// _C,
/// }
///
/// struct T(pub i32, pub i32, ());
/// ```
/// Use instead:
/// ```rust
/// #[non_exhaustive]
/// struct S {
/// pub a: i32,
/// pub b: i32,
/// }
///
/// #[non_exhaustive]
/// enum E {
/// A,
/// B,
/// }
///
/// #[non_exhaustive]
/// struct T(pub i32, pub i32);
/// ```
pub MANUAL_NON_EXHAUSTIVE,
style,
"manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]"
}

declare_lint_pass!(ManualNonExhaustive => [MANUAL_NON_EXHAUSTIVE]);

impl EarlyLintPass for ManualNonExhaustive {
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) {
match &item.kind {
ItemKind::Enum(def, _) => {
check_manual_non_exhaustive_enum(cx, item, &def.variants);
},
ItemKind::Struct(variant_data, _) => {
if let VariantData::Unit(..) = variant_data {
return;
}

check_manual_non_exhaustive_struct(cx, item, variant_data);
},
_ => {},
}
}
}

fn check_manual_non_exhaustive_enum(cx: &EarlyContext<'_>, item: &Item, variants: &[Variant]) {
fn is_non_exhaustive_marker(variant: &Variant) -> bool {
matches!(variant.data, VariantData::Unit(_))
&& variant.ident.as_str().starts_with('_')
&& variant.attrs.iter().any(|a| is_doc_hidden(a))
}

fn is_doc_hidden(attr: &Attribute) -> bool {
attr.check_name(sym!(doc))
&& match attr.meta_item_list() {
Some(l) => attr::list_contains_name(&l, sym!(hidden)),
None => false,
}
}

if_chain! {
let mut markers = variants.iter().filter(|v| is_non_exhaustive_marker(v));
if let Some(marker) = markers.next();
if markers.count() == 0 && variants.len() > 1;
then {
span_lint_and_then(
cx,
MANUAL_NON_EXHAUSTIVE,
item.span,
"this seems like a manual implementation of the non-exhaustive pattern",
|diag| {
if_chain! {
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
let header_span = cx.sess.source_map().span_until_char(item.span, '{');
if let Some(snippet) = snippet_opt(cx, header_span);
then {
diag.span_suggestion(
header_span,
"add the attribute",
format!("#[non_exhaustive] {}", snippet),
Applicability::Unspecified,
);
}
}
diag.span_help(marker.span, "remove this variant");
});
}
}
}

fn check_manual_non_exhaustive_struct(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) {
fn is_private(field: &StructField) -> bool {
matches!(field.vis.node, VisibilityKind::Inherited)
}

fn is_non_exhaustive_marker(field: &StructField) -> bool {
is_private(field) && field.ty.kind.is_unit() && field.ident.map_or(true, |n| n.as_str().starts_with('_'))
}

fn find_header_span(cx: &EarlyContext<'_>, item: &Item, data: &VariantData) -> Span {
let delimiter = match data {
VariantData::Struct(..) => '{',
VariantData::Tuple(..) => '(',
VariantData::Unit(_) => unreachable!("`VariantData::Unit` is already handled above"),
};

cx.sess.source_map().span_until_char(item.span, delimiter)
}

let fields = data.fields();
let private_fields = fields.iter().filter(|f| is_private(f)).count();
let public_fields = fields.iter().filter(|f| f.vis.node.is_pub()).count();

if_chain! {
if private_fields == 1 && public_fields >= 1 && public_fields == fields.len() - 1;
if let Some(marker) = fields.iter().find(|f| is_non_exhaustive_marker(f));
then {
span_lint_and_then(
cx,
MANUAL_NON_EXHAUSTIVE,
item.span,
"this seems like a manual implementation of the non-exhaustive pattern",
|diag| {
if_chain! {
if !attr::contains_name(&item.attrs, sym!(non_exhaustive));
let header_span = find_header_span(cx, item, data);
if let Some(snippet) = snippet_opt(cx, header_span);
then {
diag.span_suggestion(
header_span,
"add the attribute",
format!("#[non_exhaustive] {}", snippet),
Applicability::Unspecified,
);
}
}
diag.span_help(marker.span, "remove this field");
});
}
}
}
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "loops",
},
Lint {
name: "manual_non_exhaustive",
group: "style",
desc: "manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]",
deprecation: None,
module: "manual_non_exhaustive",
},
Lint {
name: "manual_saturating_arithmetic",
group: "style",
Expand Down
137 changes: 137 additions & 0 deletions tests/ui/manual_non_exhaustive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
#![warn(clippy::manual_non_exhaustive)]
#![allow(unused)]

mod enums {
enum E {
A,
B,
#[doc(hidden)]
_C,
}

// user forgot to remove the marker
#[non_exhaustive]
enum Ep {
A,
B,
#[doc(hidden)]
_C,
}

// marker variant does not have doc hidden attribute, should be ignored
enum NoDocHidden {
A,
B,
_C,
}

// name of variant with doc hidden does not start with underscore, should be ignored
enum NoUnderscore {
A,
B,
#[doc(hidden)]
C,
}

// variant with doc hidden is not unit, should be ignored
enum NotUnit {
A,
B,
#[doc(hidden)]
_C(bool),
}

// variant with doc hidden is the only one, should be ignored
enum OnlyMarker {
#[doc(hidden)]
_A,
}

// variant with multiple markers, should be ignored
enum MultipleMarkers {
A,
#[doc(hidden)]
_B,
#[doc(hidden)]
_C,
}

// already non_exhaustive and no markers, should be ignored
#[non_exhaustive]
enum NonExhaustive {
A,
B,
}
}

mod structs {
struct S {
pub a: i32,
pub b: i32,
_c: (),
}

// user forgot to remove the private field
#[non_exhaustive]
struct Sp {
pub a: i32,
pub b: i32,
_c: (),
}

// some other fields are private, should be ignored
struct PrivateFields {
a: i32,
pub b: i32,
_c: (),
}

// private field name does not start with underscore, should be ignored
struct NoUnderscore {
pub a: i32,
pub b: i32,
c: (),
}

// private field is not unit type, should be ignored
struct NotUnit {
pub a: i32,
pub b: i32,
_c: i32,
}

// private field is the only field, should be ignored
struct OnlyMarker {
_a: (),
}

// already non exhaustive and no private fields, should be ignored
#[non_exhaustive]
struct NonExhaustive {
pub a: i32,
pub b: i32,
}
}

mod tuple_structs {
struct T(pub i32, pub i32, ());

// user forgot to remove the private field
#[non_exhaustive]
struct Tp(pub i32, pub i32, ());

// some other fields are private, should be ignored
struct PrivateFields(pub i32, i32, ());

// private field is not unit type, should be ignored
struct NotUnit(pub i32, pub i32, i32);

// private field is the only field, should be ignored
struct OnlyMarker(());

// already non exhaustive and no private fields, should be ignored
#[non_exhaustive]
struct NonExhaustive(pub i32, pub i32);
}

fn main() {}
Loading

0 comments on commit d9894c1

Please sign in to comment.