diff --git a/clippy_lints/src/empty_with_brackets.rs b/clippy_lints/src/empty_with_brackets.rs index 7d87f04fef9d..a38d6df89f2b 100644 --- a/clippy_lints/src/empty_with_brackets.rs +++ b/clippy_lints/src/empty_with_brackets.rs @@ -1,10 +1,15 @@ -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::snippet_opt; -use rustc_ast::ast::{Item, ItemKind, Variant, VariantData}; +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_lexer::TokenKind; -use rustc_lint::{EarlyContext, EarlyLintPass}; -use rustc_session::declare_lint_pass; +use rustc_hir::def::CtorOf; +use rustc_hir::def::DefKind::Ctor; +use rustc_hir::def::Res::Def; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::{Expr, ExprKind, Item, ItemKind, Node, Path, QPath, Variant, VariantData}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::TyCtxt; +use rustc_session::impl_lint_pass; use rustc_span::Span; declare_clippy_lint! { @@ -70,10 +75,23 @@ declare_clippy_lint! { "finds enum variants with empty brackets" } -declare_lint_pass!(EmptyWithBrackets => [EMPTY_STRUCTS_WITH_BRACKETS, EMPTY_ENUM_VARIANTS_WITH_BRACKETS]); +#[derive(Debug)] +enum Usage { + Unused { redundant_use_sites: Vec }, + Used, + NoDefinition { redundant_use_sites: Vec }, +} + +#[derive(Default)] +pub struct EmptyWithBrackets { + // Value holds `Usage::Used` if the empty tuple variant was used as a function + empty_tuple_enum_variants: FxIndexMap, +} + +impl_lint_pass!(EmptyWithBrackets => [EMPTY_STRUCTS_WITH_BRACKETS, EMPTY_ENUM_VARIANTS_WITH_BRACKETS]); -impl EarlyLintPass for EmptyWithBrackets { - fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { +impl LateLintPass<'_> for EmptyWithBrackets { + fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { if let ItemKind::Struct(ident, var_data, _) = &item.kind && has_brackets(var_data) && let span_after_ident = item.span.with_lo(ident.span.hi()) @@ -96,70 +114,175 @@ impl EarlyLintPass for EmptyWithBrackets { } } - fn check_variant(&mut self, cx: &EarlyContext<'_>, variant: &Variant) { + fn check_variant(&mut self, cx: &LateContext<'_>, variant: &Variant<'_>) { + // the span of the parentheses/braces let span_after_ident = variant.span.with_lo(variant.ident.span.hi()); - if has_brackets(&variant.data) && has_no_fields(cx, &variant.data, span_after_ident) { - span_lint_and_then( + if has_no_fields(cx, &variant.data, span_after_ident) { + 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, + ); + }, + ); + }, + 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![], + }, + ); + } + }, + VariantData::Unit(..) => {}, + } + } + } + + 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) { + // Do not count expressions from macro expansion as a redundant use site. + 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], + }, + ); + }, + _ => {}, + } + } else { + // The parentheses are not redundant. + self.empty_tuple_enum_variants.insert(def_id, Usage::Used); + } + } + } + + 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 + let Usage::Unused { redundant_use_sites } = usage else { + continue; + }; + // Attempt to fetch the Variant from LocalDefId. + 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 { + continue; + }; + // Span of the parentheses in variant definition + let span = variant.span.with_lo(variant.ident.span.hi()); + span_lint_hir_and_then( cx, EMPTY_ENUM_VARIANTS_WITH_BRACKETS, - span_after_ident, + variant.hir_id, + span, "enum variant has empty brackets", |diagnostic| { - diagnostic.span_suggestion_hidden( - span_after_ident, - "remove the brackets", - "", - Applicability::MaybeIncorrect, - ); + if redundant_use_sites.is_empty() { + // If there's no redundant use sites, the definition is the only place to modify. + diagnostic.span_suggestion_hidden( + span, + "remove the brackets", + "", + Applicability::MaybeIncorrect, + ); + } else { + let mut parentheses_spans: Vec<_> = + redundant_use_sites.iter().map(|span| (*span, String::new())).collect(); + parentheses_spans.push((span, String::new())); + diagnostic.multipart_suggestion( + "remove the brackets", + parentheses_spans, + Applicability::MaybeIncorrect, + ); + } }, ); } } } -fn has_no_ident_token(braces_span_str: &str) -> bool { - !rustc_lexer::tokenize(braces_span_str).any(|t| t.kind == TokenKind::Ident) +fn has_brackets(var_data: &VariantData<'_>) -> bool { + !matches!(var_data, VariantData::Unit(..)) } -fn has_brackets(var_data: &VariantData) -> bool { - !matches!(var_data, VariantData::Unit(_)) -} - -fn has_no_fields(cx: &EarlyContext<'_>, var_data: &VariantData, braces_span: Span) -> bool { - if !var_data.fields().is_empty() { - return false; - } - +fn has_no_fields(cx: &LateContext<'_>, var_data: &VariantData<'_>, braces_span: Span) -> bool { + var_data.fields().is_empty() && // there might still be field declarations hidden from the AST // (conditionally compiled code using #[cfg(..)]) - - let Some(braces_span_str) = snippet_opt(cx, braces_span) else { - return false; - }; - - has_no_ident_token(braces_span_str.as_ref()) + !span_contains_cfg(cx, braces_span) } -#[cfg(test)] -mod unit_test { - use super::*; - - #[test] - fn test_has_no_ident_token() { - let input = "{ field: u8 }"; - assert!(!has_no_ident_token(input)); - - let input = "(u8, String);"; - assert!(!has_no_ident_token(input)); - - let input = " { - // test = 5 - } - "; - assert!(has_no_ident_token(input)); +// If expression HIR ID and callee HIR ID are same, returns the span of the parentheses, else, +// returns None. +fn call_parentheses_span(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> Option { + if let Node::Expr(parent) = tcx.parent_hir_node(expr.hir_id) + && let ExprKind::Call(callee, ..) = parent.kind + && callee.hir_id == expr.hir_id + { + Some(parent.span.with_lo(expr.span.hi())) + } else { + None + } +} - let input = " ();"; - assert!(has_no_ident_token(input)); +// 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), + .. + }, + )) = expr.kind + { + def_id.as_local() + } else { + None } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2be5e620f1d9..12aabb56aa1a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -858,7 +858,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(write::Write::new(conf, format_args.clone()))); store.register_late_pass(move |_| Box::new(cargo::Cargo::new(conf))); store.register_early_pass(|| Box::new(crate_in_macro_def::CrateInMacroDef)); - store.register_early_pass(|| Box::new(empty_with_brackets::EmptyWithBrackets)); + store.register_late_pass(|_| Box::new(empty_with_brackets::EmptyWithBrackets::default())); store.register_late_pass(|_| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings)); store.register_early_pass(|| Box::new(pub_use::PubUse)); store.register_late_pass(|_| Box::new(format_push_string::FormatPushString)); diff --git a/tests/ui/empty_enum_variants_with_brackets.fixed b/tests/ui/empty_enum_variants_with_brackets.fixed index 885f6a50025e..abdf6ca5cb61 100644 --- a/tests/ui/empty_enum_variants_with_brackets.fixed +++ b/tests/ui/empty_enum_variants_with_brackets.fixed @@ -6,8 +6,7 @@ pub enum PublicTestEnum { NonEmptyParentheses(i32, i32), // No error EmptyBraces, //~^ empty_enum_variants_with_brackets - EmptyParentheses, - //~^ empty_enum_variants_with_brackets + EmptyParentheses(), // No error as enum is pub } enum TestEnum { @@ -20,6 +19,67 @@ enum TestEnum { AnotherEnum, // No error } +mod issue12551 { + enum EvenOdd { + // Used as functions -> no error + Even(), + Odd(), + // Not used as a function + Unknown, + //~^ empty_enum_variants_with_brackets + } + + fn even_odd(x: i32) -> EvenOdd { + (x % 2 == 0).then(EvenOdd::Even).unwrap_or_else(EvenOdd::Odd) + } + + fn natural_number(x: i32) -> NaturalOrNot { + (x > 0) + .then(NaturalOrNot::Natural) + .unwrap_or_else(NaturalOrNot::NotNatural) + } + + enum NaturalOrNot { + // Used as functions -> no error + Natural(), + NotNatural(), + // Not used as a function + Unknown, + //~^ empty_enum_variants_with_brackets + } + + enum RedundantParenthesesFunctionCall { + // Used as a function call but with redundant parentheses + Parentheses, + //~^ empty_enum_variants_with_brackets + // Not used as a function + NoParentheses, + } + + #[allow(clippy::no_effect)] + fn redundant_parentheses_function_call() { + // The parentheses in the below line are redundant. + RedundantParenthesesFunctionCall::Parentheses; + RedundantParenthesesFunctionCall::NoParentheses; + } + + // Same test as above but with usage of the enum occurring before the definition. + #[allow(clippy::no_effect)] + fn redundant_parentheses_function_call_2() { + // The parentheses in the below line are redundant. + RedundantParenthesesFunctionCall2::Parentheses; + RedundantParenthesesFunctionCall2::NoParentheses; + } + + enum RedundantParenthesesFunctionCall2 { + // Used as a function call but with redundant parentheses + Parentheses, + //~^ empty_enum_variants_with_brackets + // Not used as a function + NoParentheses, + } +} + enum TestEnumWithFeatures { NonEmptyBraces { #[cfg(feature = "thisisneverenabled")] @@ -28,4 +88,18 @@ enum TestEnumWithFeatures { NonEmptyParentheses(#[cfg(feature = "thisisneverenabled")] i32), // No error } +#[derive(Clone)] +enum Foo { + Variant1(i32), + Variant2, + Variant3, //~ ERROR: enum variant has empty brackets +} + +#[derive(Clone)] +pub enum PubFoo { + Variant1(i32), + Variant2, + Variant3(), +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.rs b/tests/ui/empty_enum_variants_with_brackets.rs index 092712ee2ead..63a5a8e9143e 100644 --- a/tests/ui/empty_enum_variants_with_brackets.rs +++ b/tests/ui/empty_enum_variants_with_brackets.rs @@ -6,8 +6,7 @@ pub enum PublicTestEnum { NonEmptyParentheses(i32, i32), // No error EmptyBraces {}, //~^ empty_enum_variants_with_brackets - EmptyParentheses(), - //~^ empty_enum_variants_with_brackets + EmptyParentheses(), // No error as enum is pub } enum TestEnum { @@ -20,6 +19,67 @@ enum TestEnum { AnotherEnum, // No error } +mod issue12551 { + enum EvenOdd { + // Used as functions -> no error + Even(), + Odd(), + // Not used as a function + Unknown(), + //~^ empty_enum_variants_with_brackets + } + + fn even_odd(x: i32) -> EvenOdd { + (x % 2 == 0).then(EvenOdd::Even).unwrap_or_else(EvenOdd::Odd) + } + + fn natural_number(x: i32) -> NaturalOrNot { + (x > 0) + .then(NaturalOrNot::Natural) + .unwrap_or_else(NaturalOrNot::NotNatural) + } + + enum NaturalOrNot { + // Used as functions -> no error + Natural(), + NotNatural(), + // Not used as a function + Unknown(), + //~^ empty_enum_variants_with_brackets + } + + enum RedundantParenthesesFunctionCall { + // Used as a function call but with redundant parentheses + Parentheses(), + //~^ empty_enum_variants_with_brackets + // Not used as a function + NoParentheses, + } + + #[allow(clippy::no_effect)] + fn redundant_parentheses_function_call() { + // The parentheses in the below line are redundant. + RedundantParenthesesFunctionCall::Parentheses(); + RedundantParenthesesFunctionCall::NoParentheses; + } + + // Same test as above but with usage of the enum occurring before the definition. + #[allow(clippy::no_effect)] + fn redundant_parentheses_function_call_2() { + // The parentheses in the below line are redundant. + RedundantParenthesesFunctionCall2::Parentheses(); + RedundantParenthesesFunctionCall2::NoParentheses; + } + + enum RedundantParenthesesFunctionCall2 { + // Used as a function call but with redundant parentheses + Parentheses(), + //~^ empty_enum_variants_with_brackets + // Not used as a function + NoParentheses, + } +} + enum TestEnumWithFeatures { NonEmptyBraces { #[cfg(feature = "thisisneverenabled")] @@ -28,4 +88,18 @@ enum TestEnumWithFeatures { NonEmptyParentheses(#[cfg(feature = "thisisneverenabled")] i32), // No error } +#[derive(Clone)] +enum Foo { + Variant1(i32), + Variant2, + Variant3(), //~ ERROR: enum variant has empty brackets +} + +#[derive(Clone)] +pub enum PubFoo { + Variant1(i32), + Variant2, + Variant3(), +} + fn main() {} diff --git a/tests/ui/empty_enum_variants_with_brackets.stderr b/tests/ui/empty_enum_variants_with_brackets.stderr index a9ae3b476dd6..7fe85e829a35 100644 --- a/tests/ui/empty_enum_variants_with_brackets.stderr +++ b/tests/ui/empty_enum_variants_with_brackets.stderr @@ -9,7 +9,15 @@ LL | EmptyBraces {}, = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:9:21 + --> tests/ui/empty_enum_variants_with_brackets.rs:15:16 + | +LL | EmptyBraces {}, + | ^^^ + | + = help: remove the brackets + +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:17:21 | LL | EmptyParentheses(), | ^^ @@ -17,20 +25,58 @@ LL | EmptyParentheses(), = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:16:16 + --> tests/ui/empty_enum_variants_with_brackets.rs:28:16 | -LL | EmptyBraces {}, - | ^^^ +LL | Unknown(), + | ^^ | = help: remove the brackets error: enum variant has empty brackets - --> tests/ui/empty_enum_variants_with_brackets.rs:18:21 + --> tests/ui/empty_enum_variants_with_brackets.rs:47:16 | -LL | EmptyParentheses(), - | ^^ +LL | Unknown(), + | ^^ + | + = help: remove the brackets + +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:53:20 + | +LL | Parentheses(), + | ^^ + | +help: remove the brackets + | +LL ~ Parentheses, +LL | +... +LL | // The parentheses in the below line are redundant. +LL ~ RedundantParenthesesFunctionCall::Parentheses; + | + +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:76:20 + | +LL | Parentheses(), + | ^^ + | +help: remove the brackets + | +LL ~ RedundantParenthesesFunctionCall2::Parentheses; +LL | RedundantParenthesesFunctionCall2::NoParentheses; +... +LL | // Used as a function call but with redundant parentheses +LL ~ Parentheses, + | + +error: enum variant has empty brackets + --> tests/ui/empty_enum_variants_with_brackets.rs:95:13 + | +LL | Variant3(), + | ^^ | = help: remove the brackets -error: aborting due to 4 previous errors +error: aborting due to 8 previous errors