fix: unused_unit suggests wrongly when unit never type fallback

profetia · profetia · commit d9a3982cc5af · 2025-04-15T21:22:49.000+08:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -770,7 +770,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_early_pass(|| Box::new(formatting::Formatting));
     store.register_early_pass(|| Box::new(misc_early::MiscEarlyLints));
     store.register_late_pass(|_| Box::new(redundant_closure_call::RedundantClosureCall));
-    store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
+    store.register_late_pass(move |_| Box::new(unused_unit::UnusedUnit::new(conf)));
     store.register_late_pass(|_| Box::new(returns::Return));
     store.register_late_pass(move |tcx| Box::new(collapsible_if::CollapsibleIf::new(tcx, conf)));
     store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements));
diff --git a/clippy_lints/src/unused_unit.rs b/clippy_lints/src/unused_unit.rs
@@ -1,10 +1,18 @@
+use clippy_config::Conf;
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::{SpanRangeExt, position_before_rarrow};
-use rustc_ast::visit::FnKind;
-use rustc_ast::{ClosureBinder, ast};
+use clippy_utils::{is_never_expr, is_unit_expr};
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
-use rustc_session::declare_lint_pass;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{
+    AssocItemConstraintKind, Block, Body, Expr, ExprKind, FnDecl, FnRetTy, GenericArgsParentheses, Node, PolyTraitRef,
+    Term, Ty, TyKind,
+};
+use rustc_lexer::{TokenKind, tokenize};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
 use rustc_span::{BytePos, Span};
 
 declare_clippy_lint! {
@@ -32,39 +40,66 @@ declare_clippy_lint! {
     "needless unit expression"
 }
 
-declare_lint_pass!(UnusedUnit => [UNUSED_UNIT]);
+pub struct UnusedUnit {
+    msrv: Msrv,
+}
+
+impl_lint_pass!(UnusedUnit => [UNUSED_UNIT]);
+
+impl UnusedUnit {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self { msrv: conf.msrv }
+    }
+}
 
-impl EarlyLintPass for UnusedUnit {
-    fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, span: Span, _: ast::NodeId) {
-        if let ast::FnRetTy::Ty(ref ty) = kind.decl().output
-            && let ast::TyKind::Tup(ref vals) = ty.kind
-            && vals.is_empty()
-            && !ty.span.from_expansion()
-            && get_def(span) == get_def(ty.span)
+impl<'tcx> LateLintPass<'tcx> for UnusedUnit {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        def_id: LocalDefId,
+    ) {
+        if let FnRetTy::Return(hir_ty) = decl.output
+            && is_unit_ty(hir_ty)
+            && !hir_ty.span.from_expansion()
+            && get_def(span) == get_def(hir_ty.span)
         {
             // implicit types in closure signatures are forbidden when `for<...>` is present
-            if let FnKind::Closure(&ClosureBinder::For { .. }, ..) = kind {
+            if let FnKind::Closure = kind
+                && let Node::Expr(expr) = cx.tcx.hir_node_by_def_id(def_id)
+                && let ExprKind::Closure(closure) = expr.kind
+                && !closure.bound_generic_params.is_empty()
+            {
                 return;
             }
 
-            lint_unneeded_unit_return(cx, ty, span);
+            // unit never type fallback is no longer supported since Rust 2024. For more information,
+            // see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/never-type-fallback.html>
+            if self.msrv.meets(cx, msrvs::UNIT_NEVER_TYPE_FALLBACK)
+                && let ExprKind::Block(block, _) = body.value.kind
+                && let Some(expr) = block.expr
+                && is_never_expr(cx, expr).is_some()
+            {
+                return;
+            }
+
+            lint_unneeded_unit_return(cx, hir_ty.span, span);
         }
     }
 
-    fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) {
-        if let Some(stmt) = block.stmts.last()
-            && let ast::StmtKind::Expr(ref expr) = stmt.kind
-            && is_unit_expr(expr)
-            && let ctxt = block.span.ctxt()
-            && stmt.span.ctxt() == ctxt
-            && expr.span.ctxt() == ctxt
-            && expr.attrs.is_empty()
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) {
+        if let Some(expr) = block.expr
+            && !expr.span.from_expansion()
+            && is_block_expr_unit(cx, block, expr)
+            && cx.tcx.hir_attrs(expr.hir_id).is_empty()
         {
-            let sp = expr.span;
             span_lint_and_sugg(
                 cx,
                 UNUSED_UNIT,
-                sp,
+                expr.span,
                 "unneeded unit expression",
                 "remove the final `()`",
                 String::new(),
@@ -73,40 +108,94 @@ impl EarlyLintPass for UnusedUnit {
         }
     }
 
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
-        match e.kind {
-            ast::ExprKind::Ret(Some(ref expr)) | ast::ExprKind::Break(_, Some(ref expr)) => {
-                if is_unit_expr(expr) && !expr.span.from_expansion() {
-                    span_lint_and_sugg(
-                        cx,
-                        UNUSED_UNIT,
-                        expr.span,
-                        "unneeded `()`",
-                        "remove the `()`",
-                        String::new(),
-                        Applicability::MachineApplicable,
-                    );
-                }
-            },
-            _ => (),
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Ret(Some(expr)) | ExprKind::Break(_, Some(expr)) = expr.kind
+            && is_unit_expr(expr)
+            && !expr.span.from_expansion()
+        {
+            span_lint_and_sugg(
+                cx,
+                UNUSED_UNIT,
+                expr.span,
+                "unneeded `()`",
+                "remove the `()`",
+                String::new(),
+                Applicability::MachineApplicable,
+            );
         }
     }
 
-    fn check_poly_trait_ref(&mut self, cx: &EarlyContext<'_>, poly: &ast::PolyTraitRef) {
+    fn check_poly_trait_ref(&mut self, cx: &LateContext<'tcx>, poly: &'tcx PolyTraitRef<'tcx>) {
         let segments = &poly.trait_ref.path.segments;
 
         if segments.len() == 1
             && ["Fn", "FnMut", "FnOnce"].contains(&segments[0].ident.name.as_str())
-            && let Some(args) = &segments[0].args
-            && let ast::GenericArgs::Parenthesized(generic_args) = &**args
-            && let ast::FnRetTy::Ty(ty) = &generic_args.output
-            && ty.kind.is_unit()
+            && let Some(args) = segments[0].args
+            && args.parenthesized == GenericArgsParentheses::ParenSugar
+            && let constraints = &args.constraints
+            && constraints.len() == 1
+            && constraints[0].ident.name.as_str() == "Output"
+            && let AssocItemConstraintKind::Equality { term: Term::Ty(hir_ty) } = constraints[0].kind
+            && !args.span_ext.between(poly.span.shrink_to_hi()).is_empty()
+            && !hir_ty.span.from_expansion()
+            && is_unit_ty(hir_ty)
         {
-            lint_unneeded_unit_return(cx, ty, generic_args.span);
+            lint_unneeded_unit_return(cx, hir_ty.span, poly.span);
         }
     }
 }
 
+fn is_unit_ty(ty: &Ty<'_>) -> bool {
+    matches!(ty.kind, TyKind::Tup([]))
+}
+
+/// Check if the block expression is a unit expression by itself and is not the result of any
+/// complex expression. Currently, it checks for:
+/// - Empty block
+/// - Unit expression with nested parens
+/// - Unit expression from a passthrough macro
+///
+/// A passthrough macro refers to a macro that expands its input as-is. Currently it is very hard to
+/// detect it because its expansion result is completely from user code, making its `SyntaxContext`
+/// root. For more information, see <https://github.com/rust-lang/rust-clippy/issues/4076>
+fn is_block_expr_unit(cx: &LateContext<'_>, block: &Block<'_>, expr: &Expr<'_>) -> bool {
+    // Check for empty block
+    if matches!(expr.kind, ExprKind::Tup([])) {
+        // Check for unit expression with nested parens
+        let Ok(snippet) = cx.tcx.sess.source_map().span_to_snippet(expr.span) else {
+            return false;
+        };
+        let mut has_first_paren = false;
+        for token in tokenize(&snippet) {
+            match token.kind {
+                TokenKind::OpenParen if has_first_paren => return false,
+                TokenKind::OpenParen => has_first_paren = true,
+                _ => {},
+            }
+        }
+
+        // Check for passthrough macro by tokenizing the source before the unit expression
+        let before = if let [.., stmt] = &block.stmts {
+            stmt.span.between(expr.span)
+        } else {
+            block.span.with_lo(BytePos(block.span.lo().0 + 1)).until(expr.span)
+        };
+        let Ok(snippet) = cx.tcx.sess.source_map().span_to_snippet(before) else {
+            return false;
+        };
+        for token in tokenize(&snippet) {
+            match token.kind {
+                TokenKind::LineComment { .. } | TokenKind::BlockComment { .. } | TokenKind::Whitespace => {},
+                _ => return false,
+            }
+        }
+
+        return true;
+    }
+
+    false
+}
+
 // get the def site
 #[must_use]
 fn get_def(span: Span) -> Option<Span> {
@@ -117,24 +206,15 @@ fn get_def(span: Span) -> Option<Span> {
     }
 }
 
-// is this expr a `()` unit?
-fn is_unit_expr(expr: &ast::Expr) -> bool {
-    if let ast::ExprKind::Tup(ref vals) = expr.kind {
-        vals.is_empty()
-    } else {
-        false
-    }
-}
-
-fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) {
+fn lint_unneeded_unit_return(cx: &LateContext<'_>, ty_span: Span, span: Span) {
     let (ret_span, appl) =
-        span.with_hi(ty.span.hi())
+        span.with_hi(ty_span.hi())
             .get_source_text(cx)
-            .map_or((ty.span, Applicability::MaybeIncorrect), |src| {
-                position_before_rarrow(&src).map_or((ty.span, Applicability::MaybeIncorrect), |rpos| {
+            .map_or((ty_span, Applicability::MaybeIncorrect), |src| {
+                position_before_rarrow(&src).map_or((ty_span, Applicability::MaybeIncorrect), |rpos| {
                     (
                         #[expect(clippy::cast_possible_truncation)]
-                        ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)),
+                        ty_span.with_lo(BytePos(span.lo().0 + rpos as u32)),
                         Applicability::MachineApplicable,
                     )
                 })
diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs
@@ -23,7 +23,7 @@ macro_rules! msrv_aliases {
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
     1,87,0 { OS_STR_DISPLAY, INT_MIDPOINT }
-    1,85,0 { UINT_FLOAT_MIDPOINT }
+    1,85,0 { UINT_FLOAT_MIDPOINT, UNIT_NEVER_TYPE_FALLBACK }
     1,84,0 { CONST_OPTION_AS_SLICE, MANUAL_DANGLING_PTR }
     1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY, CONST_MUT_REFS, CONST_UNWRAP }
     1,82,0 { IS_NONE_OR, REPEAT_N, RAW_REF_OP }
diff --git a/tests/ui/unused_unit.fixed b/tests/ui/unused_unit.fixed
@@ -120,3 +120,24 @@ mod issue9949 {
         ()
     }
 }
+
+#[clippy::msrv = "1.85"]
+mod issue14577 {
+    trait Unit {}
+    impl Unit for () {}
+
+    fn run<R: Unit>(f: impl FnOnce() -> R) {
+        f();
+    }
+
+    fn bar() {
+        run(|| -> () { todo!() });
+    }
+
+    struct UnitStruct;
+    impl UnitStruct {
+        fn apply<F: for<'c> Fn(&'c mut Self)>(&mut self, f: F) {
+            todo!()
+        }
+    }
+}
diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs
@@ -120,3 +120,24 @@ mod issue9949 {
         ()
     }
 }
+
+#[clippy::msrv = "1.85"]
+mod issue14577 {
+    trait Unit {}
+    impl Unit for () {}
+
+    fn run<R: Unit>(f: impl FnOnce() -> R) {
+        f();
+    }
+
+    fn bar() {
+        run(|| -> () { todo!() });
+    }
+
+    struct UnitStruct;
+    impl UnitStruct {
+        fn apply<F: for<'c> Fn(&'c mut Self)>(&mut self, f: F) {
+            todo!()
+        }
+    }
+}
diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr
@@ -1,27 +1,27 @@
 error: unneeded unit return type
-  --> tests/ui/unused_unit.rs:20:58
+  --> tests/ui/unused_unit.rs:20:28
    |
 LL |     pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
-   |                                                          ^^^^^^ help: remove the `-> ()`
+   |                            ^^^^^^ help: remove the `-> ()`
    |
 note: the lint level is defined here
   --> tests/ui/unused_unit.rs:13:9
    |
 LL | #![deny(clippy::unused_unit)]
    |         ^^^^^^^^^^^^^^^^^^^
 
-error: unneeded unit return type
-  --> tests/ui/unused_unit.rs:20:28
-   |
-LL |     pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
-   |                            ^^^^^^ help: remove the `-> ()`
-
 error: unneeded unit return type
   --> tests/ui/unused_unit.rs:23:18
    |
 LL |     where G: Fn() -> () {
    |                  ^^^^^^ help: remove the `-> ()`
 
+error: unneeded unit return type
+  --> tests/ui/unused_unit.rs:20:58
+   |
+LL |     pub fn get_unit<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
+   |                                                          ^^^^^^ help: remove the `-> ()`
+
 error: unneeded unit return type
   --> tests/ui/unused_unit.rs:25:26
    |
