From bd0a5e4ef372d542b73c30e5fb5404f89aa18946 Mon Sep 17 00:00:00 2001 From: Linshu Yang Date: Sun, 30 Nov 2025 20:52:32 +0000 Subject: [PATCH 1/2] fix: `empty_enum_variants_with_brackets` misses removing brackets in patterns --- clippy_lints/src/empty_with_brackets.rs | 84 +++++++++++++------ .../empty_enum_variants_with_brackets.fixed | 13 +++ tests/ui/empty_enum_variants_with_brackets.rs | 13 +++ .../empty_enum_variants_with_brackets.stderr | 36 ++++++-- 4 files changed, 112 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/empty_with_brackets.rs b/clippy_lints/src/empty_with_brackets.rs index e7230ebf8cba..5dc1bb96266a 100644 --- a/clippy_lints/src/empty_with_brackets.rs +++ b/clippy_lints/src/empty_with_brackets.rs @@ -2,13 +2,13 @@ use clippy_utils::attrs::span_contains_cfg; use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; use rustc_data_structures::fx::FxIndexMap; use rustc_errors::Applicability; -use rustc_hir::def::CtorOf; use rustc_hir::def::DefKind::Ctor; use rustc_hir::def::Res::Def; +use rustc_hir::def::{CtorOf, DefKind}; use rustc_hir::def_id::LocalDefId; -use rustc_hir::{Expr, ExprKind, Item, ItemKind, Node, Path, QPath, Variant, VariantData}; +use rustc_hir::{Expr, ExprKind, Item, ItemKind, Node, Pat, PatKind, Path, QPath, Variant, VariantData}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::{self, TyCtxt}; use rustc_session::impl_lint_pass; use rustc_span::Span; @@ -177,28 +177,7 @@ impl LateLintPass<'_> for EmptyWithBrackets { if expr.span.from_expansion() { return; } - match self.empty_tuple_enum_variants.get_mut(&def_id) { - Some( - &mut (Usage::Unused { - ref mut redundant_use_sites, - } - | Usage::NoDefinition { - ref mut redundant_use_sites, - }), - ) => { - redundant_use_sites.push(parentheses_span); - }, - None => { - // The variant isn't in the IndexMap which means its definition wasn't encountered yet. - self.empty_tuple_enum_variants.insert( - def_id, - Usage::NoDefinition { - redundant_use_sites: vec![parentheses_span], - }, - ); - }, - _ => {}, - } + self.update_enum_variant_usage(def_id, parentheses_span); } else { // The parentheses are not redundant. self.empty_tuple_enum_variants.insert(def_id, Usage::Used); @@ -206,6 +185,16 @@ impl LateLintPass<'_> for EmptyWithBrackets { } } + fn check_pat(&mut self, cx: &LateContext<'_>, pat: &Pat<'_>) { + if let Some((def_id, parentheses_span)) = check_pat_for_enum_as_function(cx, pat) { + if pat.span.from_expansion() { + return; + } + + self.update_enum_variant_usage(def_id, parentheses_span); + } + } + fn check_crate_post(&mut self, cx: &LateContext<'_>) { for (local_def_id, usage) in &self.empty_tuple_enum_variants { // Ignore all variants with Usage::Used or Usage::NoDefinition @@ -252,6 +241,33 @@ impl LateLintPass<'_> for EmptyWithBrackets { } } +impl EmptyWithBrackets { + fn update_enum_variant_usage(&mut self, def_id: LocalDefId, parentheses_span: Span) { + match self.empty_tuple_enum_variants.get_mut(&def_id) { + Some( + &mut (Usage::Unused { + ref mut redundant_use_sites, + } + | Usage::NoDefinition { + ref mut redundant_use_sites, + }), + ) => { + redundant_use_sites.push(parentheses_span); + }, + None => { + // The variant isn't in the IndexMap which means its definition wasn't encountered yet. + self.empty_tuple_enum_variants.insert( + def_id, + Usage::NoDefinition { + redundant_use_sites: vec![parentheses_span], + }, + ); + }, + _ => {}, + } + } +} + fn has_brackets(var_data: &VariantData<'_>) -> bool { !matches!(var_data, VariantData::Unit(..)) } @@ -291,3 +307,21 @@ fn check_expr_for_enum_as_function(expr: &Expr<'_>) -> Option { None } } + +fn check_pat_for_enum_as_function(cx: &LateContext<'_>, pat: &Pat<'_>) -> Option<(LocalDefId, Span)> { + match pat.kind { + PatKind::TupleStruct(qpath, ..) + if let Def(Ctor(CtorOf::Variant, _), def_id) = cx.typeck_results().qpath_res(&qpath, pat.hir_id) => + { + def_id.as_local().map(|id| (id, qpath.span().with_lo(pat.span.hi()))) + }, + PatKind::Struct(qpath, ..) + if let Def(DefKind::Variant, def_id) = cx.typeck_results().qpath_res(&qpath, pat.hir_id) + && let ty = cx.tcx.type_of(def_id).instantiate_identity() + && let ty::FnDef(def_id, _) = ty.kind() => + { + def_id.as_local().map(|id| (id, qpath.span().with_lo(pat.span.hi()))) + }, + _ => None, + } +} diff --git a/tests/ui/empty_enum_variants_with_brackets.fixed b/tests/ui/empty_enum_variants_with_brackets.fixed index abdf6ca5cb61..515d5a975944 100644 --- a/tests/ui/empty_enum_variants_with_brackets.fixed +++ b/tests/ui/empty_enum_variants_with_brackets.fixed @@ -1,5 +1,6 @@ #![warn(clippy::empty_enum_variants_with_brackets)] #![allow(dead_code)] +#![feature(more_qualified_paths)] pub enum PublicTestEnum { NonEmptyBraces { x: i32, y: i32 }, // No error @@ -102,4 +103,16 @@ pub enum PubFoo { Variant3(), } +fn issue16157() { + enum E { + V, + //~^ empty_enum_variants_with_brackets + } + + let E::V = E::V; + + ::V = E::V; + ::V = E::V; +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.rs b/tests/ui/empty_enum_variants_with_brackets.rs index 63a5a8e9143e..2c29fc505d3b 100644 --- a/tests/ui/empty_enum_variants_with_brackets.rs +++ b/tests/ui/empty_enum_variants_with_brackets.rs @@ -1,5 +1,6 @@ #![warn(clippy::empty_enum_variants_with_brackets)] #![allow(dead_code)] +#![feature(more_qualified_paths)] pub enum PublicTestEnum { NonEmptyBraces { x: i32, y: i32 }, // No error @@ -102,4 +103,16 @@ pub enum PubFoo { Variant3(), } +fn issue16157() { + enum E { + V(), + //~^ empty_enum_variants_with_brackets + } + + let E::V() = E::V(); + + ::V() = E::V(); + ::V {} = E::V(); +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.stderr b/tests/ui/empty_enum_variants_with_brackets.stderr index 7fe85e829a35..c487cebd21ac 100644 --- a/tests/ui/empty_enum_variants_with_brackets.stderr +++ b/tests/ui/empty_enum_variants_with_brackets.stderr @@ -1,5 +1,5 @@ error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:7:16 + --> tests/ui/empty_enum_variants_with_brackets.rs:8:16 | LL | EmptyBraces {}, | ^^^ @@ -9,7 +9,7 @@ LL | EmptyBraces {}, = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:15:16 + --> tests/ui/empty_enum_variants_with_brackets.rs:16:16 | LL | EmptyBraces {}, | ^^^ @@ -17,7 +17,7 @@ LL | EmptyBraces {}, = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:17:21 + --> tests/ui/empty_enum_variants_with_brackets.rs:18:21 | LL | EmptyParentheses(), | ^^ @@ -25,7 +25,7 @@ LL | EmptyParentheses(), = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:28:16 + --> tests/ui/empty_enum_variants_with_brackets.rs:29:16 | LL | Unknown(), | ^^ @@ -33,7 +33,7 @@ LL | Unknown(), = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:47:16 + --> tests/ui/empty_enum_variants_with_brackets.rs:48:16 | LL | Unknown(), | ^^ @@ -41,7 +41,7 @@ LL | Unknown(), = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:53:20 + --> tests/ui/empty_enum_variants_with_brackets.rs:54:20 | LL | Parentheses(), | ^^ @@ -56,7 +56,7 @@ LL ~ RedundantParenthesesFunctionCall::Parentheses; | error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:76:20 + --> tests/ui/empty_enum_variants_with_brackets.rs:77:20 | LL | Parentheses(), | ^^ @@ -71,12 +71,30 @@ LL ~ Parentheses, | error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:95:13 + --> tests/ui/empty_enum_variants_with_brackets.rs:96:13 | LL | Variant3(), | ^^ | = help: remove the brackets -error: aborting due to 8 previous errors +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:108:10 + | +LL | V(), + | ^^ + | +help: remove the brackets + | +LL ~ V, +LL | +LL | } +LL | +LL ~ let E::V = E::V; +LL | +LL ~ ::V = E::V; +LL ~ ::V = E::V; + | + +error: aborting due to 9 previous errors From ddd328b65c54530cbd1cfe15edd9882831df192e Mon Sep 17 00:00:00 2001 From: Linshu Yang Date: Mon, 1 Dec 2025 22:08:08 +0000 Subject: [PATCH 2/2] Make `empty_enum_variants_with_brackets` to support empty variant with braces --- clippy_lints/src/empty_with_brackets.rs | 153 +++++++++--------- .../empty_enum_variants_with_brackets.fixed | 17 ++ tests/ui/empty_enum_variants_with_brackets.rs | 17 ++ .../empty_enum_variants_with_brackets.stderr | 33 +++- 4 files changed, 144 insertions(+), 76 deletions(-) diff --git a/clippy_lints/src/empty_with_brackets.rs b/clippy_lints/src/empty_with_brackets.rs index 5dc1bb96266a..7cef3183491d 100644 --- a/clippy_lints/src/empty_with_brackets.rs +++ b/clippy_lints/src/empty_with_brackets.rs @@ -1,6 +1,6 @@ use clippy_utils::attrs::span_contains_cfg; use clippy_utils::diagnostics::{span_lint_and_then, span_lint_hir_and_then}; -use rustc_data_structures::fx::FxIndexMap; +use rustc_data_structures::fx::{FxIndexMap, IndexEntry}; use rustc_errors::Applicability; use rustc_hir::def::DefKind::Ctor; use rustc_hir::def::Res::Def; @@ -118,7 +118,6 @@ impl LateLintPass<'_> for EmptyWithBrackets { } fn check_variant(&mut self, cx: &LateContext<'_>, variant: &Variant<'_>) { - // FIXME: handle `$name {}` if !variant.span.from_expansion() && !variant.ident.span.from_expansion() && let span_after_ident = variant.span.with_lo(variant.ident.span.hi()) @@ -126,44 +125,14 @@ impl LateLintPass<'_> for EmptyWithBrackets { { match variant.data { VariantData::Struct { .. } => { - // Empty struct variants can be linted immediately - span_lint_and_then( - cx, - EMPTY_ENUM_VARIANTS_WITH_BRACKETS, - span_after_ident, - "enum variant has empty brackets", - |diagnostic| { - diagnostic.span_suggestion_hidden( - span_after_ident, - "remove the brackets", - "", - Applicability::MaybeIncorrect, - ); - }, - ); + self.add_enum_variant(variant.def_id); }, VariantData::Tuple(.., local_def_id) => { // Don't lint reachable tuple enums if cx.effective_visibilities.is_reachable(variant.def_id) { return; } - if let Some(entry) = self.empty_tuple_enum_variants.get_mut(&local_def_id) { - // empty_tuple_enum_variants contains Usage::NoDefinition if the variant was called before the - // definition was encountered. Now that there's a definition, convert it - // to Usage::Unused. - if let Usage::NoDefinition { redundant_use_sites } = entry { - *entry = Usage::Unused { - redundant_use_sites: redundant_use_sites.clone(), - }; - } - } else { - self.empty_tuple_enum_variants.insert( - local_def_id, - Usage::Unused { - redundant_use_sites: vec![], - }, - ); - } + self.add_enum_variant(local_def_id); }, VariantData::Unit(..) => {}, } @@ -171,27 +140,33 @@ impl LateLintPass<'_> for EmptyWithBrackets { } fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { - if let Some(def_id) = check_expr_for_enum_as_function(expr) { - if let Some(parentheses_span) = call_parentheses_span(cx.tcx, expr) { + if let Some((def_id, mut span)) = check_expr_for_enum_as_function(cx, expr) { + if span.is_empty() + && let Some(parentheses_span) = call_parentheses_span(cx.tcx, expr) + { + span = parentheses_span; + } + + if span.is_empty() { + // The parentheses are not redundant. + self.empty_tuple_enum_variants.insert(def_id, Usage::Used); + } else { // Do not count expressions from macro expansion as a redundant use site. if expr.span.from_expansion() { return; } - self.update_enum_variant_usage(def_id, parentheses_span); - } else { - // The parentheses are not redundant. - self.empty_tuple_enum_variants.insert(def_id, Usage::Used); + self.update_enum_variant_usage(def_id, span); } } } fn check_pat(&mut self, cx: &LateContext<'_>, pat: &Pat<'_>) { - if let Some((def_id, parentheses_span)) = check_pat_for_enum_as_function(cx, pat) { + if let Some((def_id, span)) = check_pat_for_enum_as_function(cx, pat) { if pat.span.from_expansion() { return; } - self.update_enum_variant_usage(def_id, parentheses_span); + self.update_enum_variant_usage(def_id, span); } } @@ -201,13 +176,19 @@ impl LateLintPass<'_> for EmptyWithBrackets { let Usage::Unused { redundant_use_sites } = usage else { continue; }; + // Attempt to fetch the Variant from LocalDefId. - let Node::Variant(variant) = cx.tcx.hir_node( + let variant = if let Node::Variant(variant) = cx.tcx.hir_node(cx.tcx.local_def_id_to_hir_id(local_def_id)) { + variant + } else if let Node::Variant(variant) = cx.tcx.hir_node( cx.tcx .local_def_id_to_hir_id(cx.tcx.parent(local_def_id.to_def_id()).expect_local()), - ) else { + ) { + variant + } else { continue; }; + // Span of the parentheses in variant definition let span = variant.span.with_lo(variant.ident.span.hi()); span_lint_hir_and_then( @@ -242,28 +223,38 @@ impl LateLintPass<'_> for EmptyWithBrackets { } impl EmptyWithBrackets { + fn add_enum_variant(&mut self, local_def_id: LocalDefId) { + self.empty_tuple_enum_variants + .entry(local_def_id) + .and_modify(|entry| { + // empty_tuple_enum_variants contains Usage::NoDefinition if the variant was called before + // the definition was encountered. Now that there's a + // definition, convert it to Usage::Unused. + if let Usage::NoDefinition { redundant_use_sites } = entry { + *entry = Usage::Unused { + redundant_use_sites: redundant_use_sites.clone(), + }; + } + }) + .or_insert_with(|| Usage::Unused { + redundant_use_sites: vec![], + }); + } + fn update_enum_variant_usage(&mut self, def_id: LocalDefId, parentheses_span: Span) { - match self.empty_tuple_enum_variants.get_mut(&def_id) { - Some( - &mut (Usage::Unused { - ref mut redundant_use_sites, + match self.empty_tuple_enum_variants.entry(def_id) { + IndexEntry::Occupied(mut e) => { + if let Usage::Unused { redundant_use_sites } | Usage::NoDefinition { redundant_use_sites } = e.get_mut() + { + redundant_use_sites.push(parentheses_span); } - | Usage::NoDefinition { - ref mut redundant_use_sites, - }), - ) => { - redundant_use_sites.push(parentheses_span); }, - None => { + IndexEntry::Vacant(e) => { // The variant isn't in the IndexMap which means its definition wasn't encountered yet. - self.empty_tuple_enum_variants.insert( - def_id, - Usage::NoDefinition { - redundant_use_sites: vec![parentheses_span], - }, - ); + e.insert(Usage::NoDefinition { + redundant_use_sites: vec![parentheses_span], + }); }, - _ => {}, } } } @@ -293,18 +284,27 @@ fn call_parentheses_span(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> Option { } // Returns the LocalDefId of the variant being called as a function if it exists. -fn check_expr_for_enum_as_function(expr: &Expr<'_>) -> Option { - if let ExprKind::Path(QPath::Resolved( - _, - Path { - res: Def(Ctor(CtorOf::Variant, _), def_id), - .. +fn check_expr_for_enum_as_function(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<(LocalDefId, Span)> { + match expr.kind { + ExprKind::Path(QPath::Resolved( + _, + Path { + res: Def(Ctor(CtorOf::Variant, _), def_id), + span, + .. + }, + )) => def_id.as_local().map(|id| (id, span.with_lo(expr.span.hi()))), + ExprKind::Struct(qpath, ..) + if let Def(DefKind::Variant, mut def_id) = cx.typeck_results().qpath_res(qpath, expr.hir_id) => + { + let ty = cx.tcx.type_of(def_id).instantiate_identity(); + if let ty::FnDef(ctor_def_id, _) = ty.kind() { + def_id = *ctor_def_id; + } + + def_id.as_local().map(|id| (id, qpath.span().with_lo(expr.span.hi()))) }, - )) = expr.kind - { - def_id.as_local() - } else { - None + _ => None, } } @@ -316,10 +316,13 @@ fn check_pat_for_enum_as_function(cx: &LateContext<'_>, pat: &Pat<'_>) -> Option def_id.as_local().map(|id| (id, qpath.span().with_lo(pat.span.hi()))) }, PatKind::Struct(qpath, ..) - if let Def(DefKind::Variant, def_id) = cx.typeck_results().qpath_res(&qpath, pat.hir_id) - && let ty = cx.tcx.type_of(def_id).instantiate_identity() - && let ty::FnDef(def_id, _) = ty.kind() => + if let Def(DefKind::Variant, mut def_id) = cx.typeck_results().qpath_res(&qpath, pat.hir_id) => { + let ty = cx.tcx.type_of(def_id).instantiate_identity(); + if let ty::FnDef(ctor_def_id, _) = ty.kind() { + def_id = *ctor_def_id; + } + def_id.as_local().map(|id| (id, qpath.span().with_lo(pat.span.hi()))) }, _ => None, diff --git a/tests/ui/empty_enum_variants_with_brackets.fixed b/tests/ui/empty_enum_variants_with_brackets.fixed index 515d5a975944..215a78bbb7d8 100644 --- a/tests/ui/empty_enum_variants_with_brackets.fixed +++ b/tests/ui/empty_enum_variants_with_brackets.fixed @@ -115,4 +115,21 @@ fn issue16157() { ::V = E::V; } +fn variant_with_braces() { + enum E { + V, + //~^ empty_enum_variants_with_brackets + } + E::V = E::V; + E::V = E::V; + ::V = ::V; + + enum F { + U, + //~^ empty_enum_variants_with_brackets + } + F::U = F::U; + ::U = F::U; +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.rs b/tests/ui/empty_enum_variants_with_brackets.rs index 2c29fc505d3b..18d502ac72ea 100644 --- a/tests/ui/empty_enum_variants_with_brackets.rs +++ b/tests/ui/empty_enum_variants_with_brackets.rs @@ -115,4 +115,21 @@ fn issue16157() { ::V {} = E::V(); } +fn variant_with_braces() { + enum E { + V(), + //~^ empty_enum_variants_with_brackets + } + E::V() = E::V(); + E::V() = E::V {}; + ::V {} = ::V {}; + + enum F { + U {}, + //~^ empty_enum_variants_with_brackets + } + F::U {} = F::U {}; + ::U {} = F::U {}; +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.stderr b/tests/ui/empty_enum_variants_with_brackets.stderr index c487cebd21ac..d50b07036a94 100644 --- a/tests/ui/empty_enum_variants_with_brackets.stderr +++ b/tests/ui/empty_enum_variants_with_brackets.stderr @@ -96,5 +96,36 @@ LL ~ ::V = E::V; LL ~ ::V = E::V; | -error: aborting due to 9 previous errors +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:120:10 + | +LL | V(), + | ^^ + | +help: remove the brackets + | +LL ~ V, +LL | +LL | } +LL ~ E::V = E::V; +LL ~ E::V = E::V; +LL ~ ::V = ::V; + | + +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:128:10 + | +LL | U {}, + | ^^^ + | +help: remove the brackets + | +LL ~ U, +LL | +LL | } +LL ~ F::U = F::U; +LL ~ ::U = F::U; + | + +error: aborting due to 11 previous errors