Skip to content

Commit

Permalink
Auto merge of #13345 - Alexendoo:manual-non-exhaustive-visibility, r=y21
Browse files Browse the repository at this point in the history 
Only lint `manual_non_exhaustive` for exported types

For types that are not exported the attribute doesn't make a difference, but the manual pattern can still be used to achieve module level non exhaustiveness

Fixes rust-lang/rust-clippy#10301
Fixes rust-lang/rust-clippy#12106

changelog: none
  • Loading branch information
@bors
bors committed Sep 5, 2024
2 parents a95afe2 + f7f5505 commit c41be9e
Show file tree
Hide file tree
Showing 6 changed files with 166 additions and 179 deletions.
3 changes: 1 addition & 2 deletions clippy_lints/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
let format_args = format_args_storage.clone();
store.register_late_pass(move |_| Box::new(methods::Methods::new(conf, format_args.clone())));
store.register_late_pass(move |_| Box::new(matches::Matches::new(conf)));
store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(conf)));
store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(conf)));
store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustive::new(conf)));
store.register_late_pass(move |_| Box::new(manual_strip::ManualStrip::new(conf)));
store.register_early_pass(move || Box::new(redundant_static_lifetimes::RedundantStaticLifetimes::new(conf)));
store.register_early_pass(move || Box::new(redundant_field_names::RedundantFieldNames::new(conf)));
Expand Down
182 changes: 73 additions & 109 deletions clippy_lints/src/manual_non_exhaustive.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@ use clippy_config::msrvs::{self, Msrv};
use clippy_config::Conf;
use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then};
use clippy_utils::is_doc_hidden;
use clippy_utils::source::SpanRangeExt;
use rustc_ast::ast::{self, VisibilityKind};
use clippy_utils::source::snippet_indent;
use itertools::Itertools;
use rustc_ast::attr;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
use rustc_hir::{self as hir, Expr, ExprKind, QPath};
use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
use rustc_hir::{Expr, ExprKind, Item, ItemKind, QPath, TyKind, VariantData};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::def_id::LocalDefId;
use rustc_span::{sym, Span};

declare_clippy_lint! {
Expand Down Expand Up @@ -62,29 +62,13 @@ declare_clippy_lint! {
"manual implementations of the non-exhaustive pattern can be simplified using #[non_exhaustive]"
}

#[expect(clippy::module_name_repetitions)]
pub struct ManualNonExhaustiveStruct {
pub struct ManualNonExhaustive {
msrv: Msrv,
}

impl ManualNonExhaustiveStruct {
pub fn new(conf: &'static Conf) -> Self {
Self {
msrv: conf.msrv.clone(),
}
}
}

impl_lint_pass!(ManualNonExhaustiveStruct => [MANUAL_NON_EXHAUSTIVE]);

#[expect(clippy::module_name_repetitions)]
pub struct ManualNonExhaustiveEnum {
msrv: Msrv,
constructed_enum_variants: FxHashSet<(DefId, DefId)>,
constructed_enum_variants: FxHashSet<LocalDefId>,
potential_enums: Vec<(LocalDefId, LocalDefId, Span, Span)>,
}

impl ManualNonExhaustiveEnum {
impl ManualNonExhaustive {
pub fn new(conf: &'static Conf) -> Self {
Self {
msrv: conf.msrv.clone(),
Expand All @@ -94,96 +78,78 @@ impl ManualNonExhaustiveEnum {
}
}

impl_lint_pass!(ManualNonExhaustiveEnum => [MANUAL_NON_EXHAUSTIVE]);

impl EarlyLintPass for ManualNonExhaustiveStruct {
fn check_item(&mut self, cx: &EarlyContext<'_>, item: &ast::Item) {
if let ast::ItemKind::Struct(variant_data, _) = &item.kind
&& let (fields, delimiter) = match variant_data {
ast::VariantData::Struct { fields, .. } => (&**fields, '{'),
ast::VariantData::Tuple(fields, _) => (&**fields, '('),
ast::VariantData::Unit(_) => return,
}
&& fields.len() > 1
&& self.msrv.meets(msrvs::NON_EXHAUSTIVE)
{
let mut iter = fields.iter().filter_map(|f| match f.vis.kind {
VisibilityKind::Public => None,
VisibilityKind::Inherited => Some(Ok(f)),
VisibilityKind::Restricted { .. } => Some(Err(())),
});
if let Some(Ok(field)) = iter.next()
&& iter.next().is_none()
&& field.ty.kind.is_unit()
{
span_lint_and_then(
cx,
MANUAL_NON_EXHAUSTIVE,
item.span,
"this seems like a manual implementation of the non-exhaustive pattern",
|diag| {
if !item.attrs.iter().any(|attr| attr.has_name(sym::non_exhaustive))
&& let header_span = cx.sess().source_map().span_until_char(item.span, delimiter)
&& let Some(snippet) = header_span.get_source_text(cx)
{
diag.span_suggestion(
header_span,
"add the attribute",
format!("#[non_exhaustive] {snippet}"),
Applicability::Unspecified,
);
}
diag.span_help(field.span, "remove this field");
},
);
}
}
}
impl_lint_pass!(ManualNonExhaustive => [MANUAL_NON_EXHAUSTIVE]);

extract_msrv_attr!(EarlyContext);
}

impl<'tcx> LateLintPass<'tcx> for ManualNonExhaustiveEnum {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
if !self.msrv.meets(msrvs::NON_EXHAUSTIVE) {
impl<'tcx> LateLintPass<'tcx> for ManualNonExhaustive {
fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) {
if !self.msrv.meets(msrvs::NON_EXHAUSTIVE) || !cx.effective_visibilities.is_exported(item.owner_id.def_id) {
return;
}

if let hir::ItemKind::Enum(def, _) = &item.kind
&& def.variants.len() > 1
{
let mut iter = def.variants.iter().filter_map(|v| {
(matches!(v.data, hir::VariantData::Unit(_, _))
&& is_doc_hidden(cx.tcx.hir().attrs(v.hir_id))
&& !attr::contains_name(cx.tcx.hir().attrs(item.hir_id()), sym::non_exhaustive))
.then_some((v.def_id, v.span))
});
if let Some((id, span)) = iter.next()
&& iter.next().is_none()
{
self.potential_enums.push((item.owner_id.def_id, id, item.span, span));
}
match item.kind {
ItemKind::Enum(def, _) if def.variants.len() > 1 => {
let iter = def.variants.iter().filter_map(|v| {
(matches!(v.data, VariantData::Unit(_, _)) && is_doc_hidden(cx.tcx.hir().attrs(v.hir_id)))
.then_some((v.def_id, v.span))
});
if let Ok((id, span)) = iter.exactly_one()
&& !attr::contains_name(cx.tcx.hir().attrs(item.hir_id()), sym::non_exhaustive)
{
self.potential_enums.push((item.owner_id.def_id, id, item.span, span));
}
},
ItemKind::Struct(variant_data, _) => {
let fields = variant_data.fields();
let private_fields = fields
.iter()
.filter(|field| !cx.effective_visibilities.is_exported(field.def_id));
if fields.len() > 1
&& let Ok(field) = private_fields.exactly_one()
&& let TyKind::Tup([]) = field.ty.kind
{
span_lint_and_then(
cx,
MANUAL_NON_EXHAUSTIVE,
item.span,
"this seems like a manual implementation of the non-exhaustive pattern",
|diag| {
if let Some(non_exhaustive) =
attr::find_by_name(cx.tcx.hir().attrs(item.hir_id()), sym::non_exhaustive)
{
diag.span_note(non_exhaustive.span, "the struct is already non-exhaustive");
} else {
let indent = snippet_indent(cx, item.span).unwrap_or_default();
diag.span_suggestion_verbose(
item.span.shrink_to_lo(),
"use the `#[non_exhaustive]` attribute instead",
format!("#[non_exhaustive]\n{indent}"),
Applicability::MaybeIncorrect,
);
}
diag.span_help(field.span, "remove this field");
},
);
}
},
_ => {},
}
}

fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
if let ExprKind::Path(QPath::Resolved(None, p)) = &e.kind
&& let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), id) = p.res
&& let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Const), ctor_id) = p.res
&& let Some(local_ctor) = ctor_id.as_local()
{
let variant_id = cx.tcx.parent(id);
let enum_id = cx.tcx.parent(variant_id);

self.constructed_enum_variants.insert((enum_id, variant_id));
let variant_id = cx.tcx.local_parent(local_ctor);
self.constructed_enum_variants.insert(variant_id);
}
}

fn check_crate_post(&mut self, cx: &LateContext<'tcx>) {
for &(enum_id, _, enum_span, variant_span) in
self.potential_enums.iter().filter(|&&(enum_id, variant_id, _, _)| {
!self
.constructed_enum_variants
.contains(&(enum_id.to_def_id(), variant_id.to_def_id()))
})
for &(enum_id, _, enum_span, variant_span) in self
.potential_enums
.iter()
.filter(|(_, variant_id, _, _)| !self.constructed_enum_variants.contains(variant_id))
{
let hir_id = cx.tcx.local_def_id_to_hir_id(enum_id);
span_lint_hir_and_then(
Expand All @@ -193,15 +159,13 @@ impl<'tcx> LateLintPass<'tcx> for ManualNonExhaustiveEnum {
enum_span,
"this seems like a manual implementation of the non-exhaustive pattern",
|diag| {
let header_span = cx.sess().source_map().span_until_char(enum_span, '{');
if let Some(snippet) = header_span.get_source_text(cx) {
diag.span_suggestion(
header_span,
"add the attribute",
format!("#[non_exhaustive] {snippet}"),
Applicability::Unspecified,
);
}
let indent = snippet_indent(cx, enum_span).unwrap_or_default();
diag.span_suggestion_verbose(
enum_span.shrink_to_lo(),
"use the `#[non_exhaustive]` attribute instead",
format!("#[non_exhaustive]\n{indent}"),
Applicability::MaybeIncorrect,
);
diag.span_help(variant_span, "remove this variant");
},
);
Expand Down
24 changes: 11 additions & 13 deletions tests/ui/manual_non_exhaustive_enum.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#![warn(clippy::manual_non_exhaustive)]
#![allow(unused)]
//@no-rustfix
enum E {
//~^ ERROR: this seems like a manual implementation of the non-exhaustive pattern
pub enum E {
//~^ manual_non_exhaustive
A,
B,
#[doc(hidden)]
Expand All @@ -11,44 +11,44 @@ enum E {

// if the user explicitly marks as nonexhaustive we shouldn't warn them
#[non_exhaustive]
enum Ep {
pub enum Ep {
A,
B,
#[doc(hidden)]
_C,
}

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

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

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

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

// variant with multiple markers, should be ignored
enum MultipleMarkers {
pub enum MultipleMarkers {
A,
#[doc(hidden)]
_B,
Expand All @@ -58,13 +58,13 @@ enum MultipleMarkers {

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

// marked is used, don't lint
enum UsedHidden {
pub enum UsedHidden {
#[doc(hidden)]
_A,
B,
Expand All @@ -77,11 +77,9 @@ fn foo(x: &mut UsedHidden) {
}

#[expect(clippy::manual_non_exhaustive)]
enum ExpectLint {
pub enum ExpectLint {
A,
B,
#[doc(hidden)]
_C,
}

fn main() {}
22 changes: 12 additions & 10 deletions tests/ui/manual_non_exhaustive_enum.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
error: this seems like a manual implementation of the non-exhaustive pattern
--> tests/ui/manual_non_exhaustive_enum.rs:4:1
|
LL | enum E {
| ^-----
| |
| _help: add the attribute: `#[non_exhaustive] enum E`
| |
LL | / pub enum E {
LL | |
LL | | A,
LL | | B,
Expand All @@ -21,15 +17,16 @@ LL | _C,
| ^^
= note: `-D clippy::manual-non-exhaustive` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_non_exhaustive)]`
help: use the `#[non_exhaustive]` attribute instead
|
LL + #[non_exhaustive]
LL | pub enum E {
|

error: this seems like a manual implementation of the non-exhaustive pattern
--> tests/ui/manual_non_exhaustive_enum.rs:29:1
|
LL | enum NoUnderscore {
| ^----------------
| |
| _help: add the attribute: `#[non_exhaustive] enum NoUnderscore`
| |
LL | / pub enum NoUnderscore {
LL | | A,
LL | | B,
LL | | #[doc(hidden)]
Expand All @@ -42,6 +39,11 @@ help: remove this variant
|
LL | C,
| ^
help: use the `#[non_exhaustive]` attribute instead
|
LL + #[non_exhaustive]
LL | pub enum NoUnderscore {
|

error: aborting due to 2 previous errors

Loading

0 comments on commit c41be9e

Please sign in to comment.