empty_enum_variants_with_brackets: Do not lint reachable enums and enum variants used as functions in the same crate (#12971)

Fixes #12551

changelog: [`empty_enum_variants_with_brackets`]: Do not lint reachable
enums or enums which are used as functions within the same crate.

r? @xFrednet

Author: y21

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<Span> },
+    Used,
+    NoDefinition { redundant_use_sites: Vec<Span> },
+}
+
+#[derive(Default)]
+pub struct EmptyWithBrackets {
+    // Value holds `Usage::Used` if the empty tuple variant was used as a function
+    empty_tuple_enum_variants: FxIndexMap<LocalDefId, Usage>,
+}
+
+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<Span> {
+    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<LocalDefId> {
+    if let ExprKind::Path(QPath::Resolved(
+        _,
+        Path {
+            res: Def(Ctor(CtorOf::Variant, _), def_id),
+            ..
+        },
+    )) = expr.kind
+    {
+        def_id.as_local()
+    } else {
+        None
     }
 }

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));

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() {}