From caf59304caf60091022e5c1e796dfb79fd37d998 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Wed, 1 Jan 2025 21:06:18 +0100 Subject: [PATCH] New lint: unit_as_impl_trait --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unit_as_impl_trait.rs | 95 ++++++++++++++++++++ tests/ui/crashes/ice-11422.fixed | 1 + tests/ui/crashes/ice-11422.rs | 1 + tests/ui/crashes/ice-11422.stderr | 2 +- tests/ui/implied_bounds_in_impls.fixed | 2 +- tests/ui/implied_bounds_in_impls.rs | 2 +- tests/ui/new_ret_no_self.rs | 2 +- tests/ui/unit_as_impl_trait.rs | 45 ++++++++++ tests/ui/unit_as_impl_trait.stderr | 120 +++++++++++++++++++++++++ 12 files changed, 270 insertions(+), 4 deletions(-) create mode 100644 clippy_lints/src/unit_as_impl_trait.rs create mode 100644 tests/ui/unit_as_impl_trait.rs create mode 100644 tests/ui/unit_as_impl_trait.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 664a7e766304..c2d8f79133ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6103,6 +6103,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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 7451fb909ef8..4fd562668efa 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -744,6 +744,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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fad6f9d08803..6e654cdf48ce 100644 --- a/clippy_lints/src/lib.rs +++ b/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; @@ -970,5 +971,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp)); 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(unit_as_impl_trait::UnitAsImplTrait)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unit_as_impl_trait.rs b/clippy_lints/src/unit_as_impl_trait.rs new file mode 100644 index 000000000000..0cd49f124f5c --- /dev/null +++ b/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"); + } + }, + ); + } + } +} diff --git a/tests/ui/crashes/ice-11422.fixed b/tests/ui/crashes/ice-11422.fixed index ca5721cbb2ba..55e7bd4063c0 100644 --- a/tests/ui/crashes/ice-11422.fixed +++ b/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::*; diff --git a/tests/ui/crashes/ice-11422.rs b/tests/ui/crashes/ice-11422.rs index 355ec2480bba..4119cf62f2f8 100644 --- a/tests/ui/crashes/ice-11422.rs +++ b/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::*; diff --git a/tests/ui/crashes/ice-11422.stderr b/tests/ui/crashes/ice-11422.stderr index a340977f4699..3207a67fc545 100644 --- a/tests/ui/crashes/ice-11422.stderr +++ b/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:31 + --> tests/ui/crashes/ice-11422.rs:7:31 | LL | fn gen() -> impl PartialOrd + PartialEq + Debug {} | ^^^^^^^^^ diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed index 6fd4cbd80fe4..1e348458b8c8 100644 --- a/tests/ui/implied_bounds_in_impls.fixed +++ b/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}; diff --git a/tests/ui/implied_bounds_in_impls.rs b/tests/ui/implied_bounds_in_impls.rs index 52076c9f998e..f10bba1f0071 100644 --- a/tests/ui/implied_bounds_in_impls.rs +++ b/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}; diff --git a/tests/ui/new_ret_no_self.rs b/tests/ui/new_ret_no_self.rs index 175b14d815a2..7a8a145aecbf 100644 --- a/tests/ui/new_ret_no_self.rs +++ b/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() {} diff --git a/tests/ui/unit_as_impl_trait.rs b/tests/ui/unit_as_impl_trait.rs new file mode 100644 index 000000000000..04f113d7339d --- /dev/null +++ b/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(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() {} diff --git a/tests/ui/unit_as_impl_trait.stderr b/tests/ui/unit_as_impl_trait.stderr new file mode 100644 index 000000000000..e6ff6c52ebb7 --- /dev/null +++ b/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(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(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 +