New lint: unit_as_impl_trait

CHANGELOG.md

@@ -6146,6 +6146,7 @@ Released 2018-09-13
 [`uninit_vec`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninit_vec
 [`uninlined_format_args`]: https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
 [`unit_arg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
+[`unit_as_impl_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_as_impl_trait
 [`unit_cmp`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_cmp
 [`unit_hash`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_hash
 [`unit_return_expecting_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#unit_return_expecting_ord

clippy_lints/src/declared_lints.rs

@@ -746,6 +746,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::unicode::UNICODE_NOT_NFC_INFO,
     crate::uninhabited_references::UNINHABITED_REFERENCES_INFO,
     crate::uninit_vec::UNINIT_VEC_INFO,
+    crate::unit_as_impl_trait::UNIT_AS_IMPL_TRAIT_INFO,
     crate::unit_return_expecting_ord::UNIT_RETURN_EXPECTING_ORD_INFO,
     crate::unit_types::LET_UNIT_VALUE_INFO,
     crate::unit_types::UNIT_ARG_INFO,

clippy_lints/src/lib.rs

@@ -365,6 +365,7 @@ mod undocumented_unsafe_blocks;
 mod unicode;
 mod uninhabited_references;
 mod uninit_vec;
+mod unit_as_impl_trait;
 mod unit_return_expecting_ord;
 mod unit_types;
 mod unnecessary_box_returns;
@@ -972,5 +973,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound));
     store.register_late_pass(move |_| Box::new(arbitrary_source_item_ordering::ArbitrarySourceItemOrdering::new(conf)));
     store.register_late_pass(|_| Box::new(unneeded_struct_pattern::UnneededStructPattern));
+    store.register_late_pass(|_| Box::new(unit_as_impl_trait::UnitAsImplTrait));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/unit_as_impl_trait.rs

@@ -0,0 +1,95 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, ExprKind, FnDecl, FnRetTy, TyKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::{BytePos, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for function whose return type is an `impl` trait
+    /// implemented by `()`, and whose `()` return value is implicit.
+    ///
+    /// ### Why is this bad?
+    /// Since the required trait is implemented by `()`, adding an extra `;`
+    /// at the end of the function last expression will compile with no
+    /// indication that the expected value may have been silently swallowed.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// fn key() -> impl Eq {
+    ///     [1, 10, 2, 0].sort_unstable()
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn key() -> impl Eq {
+    ///     let mut arr = [1, 10, 2, 0];
+    ///     arr.sort_unstable();
+    ///     arr
+    /// }
+    /// ```
+    /// or
+    /// ```no_run
+    /// fn key() -> impl Eq {
+    ///     [1, 10, 2, 0].sort_unstable();
+    ///     ()
+    /// }
+    /// ```
+    /// if returning `()` is intentional.
+    #[clippy::version = "1.86.0"]
+    pub UNIT_AS_IMPL_TRAIT,
+    suspicious,
+    "`()` which implements the required trait might be returned unexpectedly"
+}
+
+declare_lint_pass!(UnitAsImplTrait => [UNIT_AS_IMPL_TRAIT]);
+
+impl<'tcx> LateLintPass<'tcx> for UnitAsImplTrait {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        _: LocalDefId,
+    ) {
+        if !span.from_expansion()
+            && let FnRetTy::Return(ty) = decl.output
+            && let TyKind::OpaqueDef(_) = ty.kind
+            && cx.typeck_results().expr_ty(body.value).is_unit()
+            && let ExprKind::Block(block, None) = body.value.kind
+            && block.expr.is_none_or(|e| !matches!(e.kind, ExprKind::Tup([])))
+        {
+            let block_end_span = block.span.with_hi(block.span.hi() - BytePos(1)).shrink_to_hi();
+
+            span_lint_and_then(
+                cx,
+                UNIT_AS_IMPL_TRAIT,
+                ty.span,
+                "this function returns `()` which implements the required trait",
+                |diag| {
+                    if let Some(expr) = block.expr {
+                        diag.span_note(expr.span, "this expression evaluates to `()`")
+                            .span_help(
+                                expr.span.shrink_to_hi(),
+                                "consider being explicit, and terminate the body with `()`",
+                            );
+                    } else if let Some(last) = block.stmts.last() {
+                        diag.span_note(last.span, "this statement evaluates to `()` because it ends with `;`")
+                            .span_note(block.span, "therefore the function body evaluates to `()`")
+                            .span_help(
+                                block_end_span,
+                                "if this is intentional, consider being explicit, and terminate the body with `()`",
+                            );
+                    } else {
+                        diag.span_note(block.span, "the empty body evaluates to `()`")
+                            .span_help(block_end_span, "consider being explicit and use `()` in the body");
+                    }
+                },
+            );
+        }
+    }
+}

tests/ui/crashes/ice-11422.fixed

@@ -1,4 +1,5 @@
 #![warn(clippy::implied_bounds_in_impls)]
+#![allow(clippy::unit_as_impl_trait)]
 
 use std::fmt::Debug;
 use std::ops::*;

tests/ui/crashes/ice-11422.rs

@@ -1,4 +1,5 @@
 #![warn(clippy::implied_bounds_in_impls)]
+#![allow(clippy::unit_as_impl_trait)]
 
 use std::fmt::Debug;
 use std::ops::*;

tests/ui/crashes/ice-11422.stderr

@@ -1,5 +1,5 @@
 error: this bound is already specified as the supertrait of `PartialOrd`
-  --> tests/ui/crashes/ice-11422.rs:6:33
+  --> tests/ui/crashes/ice-11422.rs:7:33
    |
 LL | fn r#gen() -> impl PartialOrd + PartialEq + Debug {}
    |                                 ^^^^^^^^^

tests/ui/implied_bounds_in_impls.fixed

@@ -1,5 +1,5 @@
 #![warn(clippy::implied_bounds_in_impls)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::unit_as_impl_trait)]
 #![feature(impl_trait_in_assoc_type, type_alias_impl_trait)]
 
 use std::ops::{Deref, DerefMut};

tests/ui/implied_bounds_in_impls.rs

@@ -1,5 +1,5 @@
 #![warn(clippy::implied_bounds_in_impls)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::unit_as_impl_trait)]
 #![feature(impl_trait_in_assoc_type, type_alias_impl_trait)]
 
 use std::ops::{Deref, DerefMut};

tests/ui/new_ret_no_self.rs

@@ -1,6 +1,6 @@
 #![feature(type_alias_impl_trait)]
 #![warn(clippy::new_ret_no_self)]
-#![allow(dead_code)]
+#![allow(dead_code, clippy::unit_as_impl_trait)]
 
 fn main() {}
 

tests/ui/unit_as_impl_trait.rs

@@ -0,0 +1,45 @@
+#![warn(clippy::unit_as_impl_trait)]
+#![allow(clippy::unused_unit)]
+
+fn implicit_unit() -> impl Copy {
+    //~^ ERROR: function returns `()` which implements the required trait
+}
+
+fn explicit_unit() -> impl Copy {
+    ()
+}
+
+fn not_unit(x: u32) -> impl Copy {
+    x
+}
+
+fn never(x: u32) -> impl Copy {
+    panic!();
+}
+
+fn with_generic_param<T: Eq>(x: T) -> impl Eq {
+    //~^ ERROR: function returns `()` which implements the required trait
+    x;
+}
+
+fn non_empty_implicit_unit() -> impl Copy {
+    //~^ ERROR: function returns `()` which implements the required trait
+    println!("foobar");
+}
+
+fn last_expression_returning_unit() -> impl Eq {
+    //~^ ERROR: function returns `()` which implements the required trait
+    [1, 10, 2, 0].sort_unstable()
+}
+
+#[derive(Clone)]
+struct S;
+
+impl S {
+    fn clone(&self) -> impl Clone {
+        //~^ ERROR: function returns `()` which implements the required trait
+        S;
+    }
+}
+
+fn main() {}

tests/ui/unit_as_impl_trait.stderr

@@ -0,0 +1,120 @@
+error: this function returns `()` which implements the required trait
+  --> tests/ui/unit_as_impl_trait.rs:4:23
+   |
+LL | fn implicit_unit() -> impl Copy {
+   |                       ^^^^^^^^^
+   |
+note: the empty body evaluates to `()`
+  --> tests/ui/unit_as_impl_trait.rs:4:33
+   |
+LL |   fn implicit_unit() -> impl Copy {
+   |  _________________________________^
+LL | |
+LL | | }
+   | |_^
+help: consider being explicit and use `()` in the body
+  --> tests/ui/unit_as_impl_trait.rs:6:1
+   |
+LL | }
+   | ^
+   = note: `-D clippy::unit-as-impl-trait` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unit_as_impl_trait)]`
+
+error: this function returns `()` which implements the required trait
+  --> tests/ui/unit_as_impl_trait.rs:20:39
+   |
+LL | fn with_generic_param<T: Eq>(x: T) -> impl Eq {
+   |                                       ^^^^^^^
+   |
+note: this statement evaluates to `()` because it ends with `;`
+  --> tests/ui/unit_as_impl_trait.rs:22:5
+   |
+LL |     x;
+   |     ^^
+note: therefore the function body evaluates to `()`
+  --> tests/ui/unit_as_impl_trait.rs:20:47
+   |
+LL |   fn with_generic_param<T: Eq>(x: T) -> impl Eq {
+   |  _______________________________________________^
+LL | |
+LL | |     x;
+LL | | }
+   | |_^
+help: if this is intentional, consider being explicit, and terminate the body with `()`
+  --> tests/ui/unit_as_impl_trait.rs:23:1
+   |
+LL | }
+   | ^
+
+error: this function returns `()` which implements the required trait
+  --> tests/ui/unit_as_impl_trait.rs:25:33
+   |
+LL | fn non_empty_implicit_unit() -> impl Copy {
+   |                                 ^^^^^^^^^
+   |
+note: this statement evaluates to `()` because it ends with `;`
+  --> tests/ui/unit_as_impl_trait.rs:27:5
+   |
+LL |     println!("foobar");
+   |     ^^^^^^^^^^^^^^^^^^
+note: therefore the function body evaluates to `()`
+  --> tests/ui/unit_as_impl_trait.rs:25:43
+   |
+LL |   fn non_empty_implicit_unit() -> impl Copy {
+   |  ___________________________________________^
+LL | |
+LL | |     println!("foobar");
+LL | | }
+   | |_^
+help: if this is intentional, consider being explicit, and terminate the body with `()`
+  --> tests/ui/unit_as_impl_trait.rs:28:1
+   |
+LL | }
+   | ^
+   = note: this error originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: this function returns `()` which implements the required trait
+  --> tests/ui/unit_as_impl_trait.rs:30:40
+   |
+LL | fn last_expression_returning_unit() -> impl Eq {
+   |                                        ^^^^^^^
+   |
+note: this expression evaluates to `()`
+  --> tests/ui/unit_as_impl_trait.rs:32:5
+   |
+LL |     [1, 10, 2, 0].sort_unstable()
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: consider being explicit, and terminate the body with `()`
+  --> tests/ui/unit_as_impl_trait.rs:32:34
+   |
+LL |     [1, 10, 2, 0].sort_unstable()
+   |                                  ^
+
+error: this function returns `()` which implements the required trait
+  --> tests/ui/unit_as_impl_trait.rs:39:24
+   |
+LL |     fn clone(&self) -> impl Clone {
+   |                        ^^^^^^^^^^
+   |
+note: this statement evaluates to `()` because it ends with `;`
+  --> tests/ui/unit_as_impl_trait.rs:41:9
+   |
+LL |         S;
+   |         ^^
+note: therefore the function body evaluates to `()`
+  --> tests/ui/unit_as_impl_trait.rs:39:35
+   |
+LL |       fn clone(&self) -> impl Clone {
+   |  ___________________________________^
+LL | |
+LL | |         S;
+LL | |     }
+   | |_____^
+help: if this is intentional, consider being explicit, and terminate the body with `()`
+  --> tests/ui/unit_as_impl_trait.rs:42:5
+   |
+LL |     }
+   |     ^
+
+error: aborting due to 5 previous errors
+