fix: unused_unit suggests wrongly when unit never type fallback

Author: profetia

clippy_lints/src/lib.rs

@@ -729,6 +729,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     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(|_| Box::new(unused_unit::UnusedUnit));
     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));

clippy_lints/src/unused_unit.rs

@@ -1,11 +1,18 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 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_ast::{Block, StmtKind};
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{
+    AssocItemConstraintKind, Body, Expr, ExprKind, FnDecl, FnRetTy, GenericArgsParentheses, Node, PolyTraitRef, Term,
+    Ty, TyKind,
+};
+use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::{BytePos, Span};
+use rustc_span::edition::Edition;
+use rustc_span::{BytePos, Span, sym};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -34,27 +41,89 @@ declare_clippy_lint! {
 
 declare_lint_pass!(UnusedUnit => [UNUSED_UNIT]);
 
-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;
+            }
+
+            // 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 cx.tcx.sess.edition() >= Edition::Edition2024
+                && 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, ty, span);
+            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
+    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: &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
+            && args.parenthesized == GenericArgsParentheses::ParenSugar
+            && let constraints = &args.constraints
+            && constraints.len() == 1
+            && constraints[0].ident.name == sym::Output
+            && let AssocItemConstraintKind::Equality { term: Term::Ty(hir_ty) } = constraints[0].kind
+            && args.span_ext.hi() != poly.span.hi()
+            && !hir_ty.span.from_expansion()
+            && is_unit_ty(hir_ty)
+        {
+            lint_unneeded_unit_return(cx, hir_ty.span, poly.span);
+        }
+    }
+}
+
+impl EarlyLintPass for UnusedUnit {
+    /// Check for unit expressions in blocks. This is left in the early pass because some macros
+    /// expand its inputs as-is, making it invisible to the late pass. See #4076.
+    fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) {
+        if let Some(stmt) = block.stmts.last()
+            && let StmtKind::Expr(expr) = &stmt.kind
+            && let rustc_ast::ExprKind::Tup(inner) = &expr.kind
+            && inner.is_empty()
             && let ctxt = block.span.ctxt()
             && stmt.span.ctxt() == ctxt
             && expr.span.ctxt() == ctxt
@@ -72,39 +141,10 @@ 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_poly_trait_ref(&mut self, cx: &EarlyContext<'_>, poly: &ast::PolyTraitRef) {
-        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()
-        {
-            lint_unneeded_unit_return(cx, ty, generic_args.span);
-        }
-    }
+fn is_unit_ty(ty: &Ty<'_>) -> bool {
+    matches!(ty.kind, TyKind::Tup([]))
 }
 
 // get the def site
@@ -117,24 +157,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,
                     )
                 })

tests/ui/unused_unit.edition2021.fixed

@@ -0,0 +1,146 @@
+//@revisions: edition2021 edition2024
+//@[edition2021] edition:2021
+//@[edition2024] edition:2024
+
+// The output for humans should just highlight the whole span without showing
+// the suggested replacement, but we also want to test that suggested
+// replacement only removes one set of parentheses, rather than naïvely
+// stripping away any starting or ending parenthesis characters—hence this
+// test of the JSON error format.
+
+#![feature(custom_inner_attributes)]
+#![feature(closure_lifetime_binder)]
+#![rustfmt::skip]
+
+#![deny(clippy::unused_unit)]
+#![allow(dead_code)]
+#![allow(clippy::from_over_into)]
+
+struct Unitter;
+impl Unitter {
+    #[allow(clippy::no_effect)]
+    pub fn get_unit<F: Fn(), G>(&self, f: F, _g: G)
+    //~^ unused_unit
+    //~| unused_unit
+    where G: Fn() {
+    //~^ unused_unit
+        let _y: &dyn Fn() = &f;
+        //~^ unused_unit
+        (); // this should not lint, as it's not in return type position
+    }
+}
+
+impl Into<()> for Unitter {
+    #[rustfmt::skip]
+    fn into(self) {
+    //~^ unused_unit
+        
+        //~^ unused_unit
+    }
+}
+
+trait Trait {
+    fn redundant<F: FnOnce(), G, H>(&self, _f: F, _g: G, _h: H)
+    //~^ unused_unit
+    where
+        G: FnMut(),
+        //~^ unused_unit
+        H: Fn();
+        //~^ unused_unit
+}
+
+impl Trait for Unitter {
+    fn redundant<F: FnOnce(), G, H>(&self, _f: F, _g: G, _h: H)
+    //~^ unused_unit
+    where
+        G: FnMut(),
+        //~^ unused_unit
+        H: Fn() {}
+        //~^ unused_unit
+}
+
+fn return_unit() {  }
+//~^ unused_unit
+//~| unused_unit
+
+#[allow(clippy::needless_return)]
+#[allow(clippy::never_loop)]
+#[allow(clippy::unit_cmp)]
+fn main() {
+    let u = Unitter;
+    assert_eq!(u.get_unit(|| {}, return_unit), u.into());
+    return_unit();
+    loop {
+        break;
+        //~^ unused_unit
+    }
+    return;
+    //~^ unused_unit
+}
+
+// https://github.com/rust-lang/rust-clippy/issues/4076
+fn foo() {
+    macro_rules! foo {
+        (recv($r:expr) -> $res:pat => $body:expr) => {
+            $body
+        }
+    }
+
+    foo! {
+        recv(rx) -> _x => ()
+    }
+}
+
+#[rustfmt::skip]
+fn test(){}
+//~^ unused_unit
+
+#[rustfmt::skip]
+fn test2(){}
+//~^ unused_unit
+
+#[rustfmt::skip]
+fn test3(){}
+//~^ unused_unit
+
+fn macro_expr() {
+    macro_rules! e {
+        () => (());
+    }
+    e!()
+}
+
+mod issue9748 {
+    fn main() {
+        let _ = for<'a> |_: &'a u32| -> () {};
+    }
+}
+
+mod issue9949 {
+    fn main() {
+        #[doc = "documentation"]
+        ()
+    }
+}
+
+mod issue14577 {
+    trait Unit {}
+    impl Unit for () {}
+
+    fn run<R: Unit>(f: impl FnOnce() -> R) {
+        f();
+    }
+
+    #[allow(dependency_on_unit_never_type_fallback)]
+    fn bar() {
+        run(|| { todo!() }); 
+        //~[edition2021]^ unused_unit
+    }
+
+    struct UnitStruct;
+    impl UnitStruct {
+        fn apply<F: for<'c> Fn(&'c mut Self)>(&mut self, f: F) {
+            todo!()
+        }
+    }
+}