From 9eba2cccd897814923b21a1c8665c462334eaaa2 Mon Sep 17 00:00:00 2001 From: yanglsh Date: Thu, 16 Oct 2025 23:52:12 +0800 Subject: [PATCH] fix: `len_zero` FP on unstable methods --- book/src/lint_configuration.md | 1 + clippy_config/src/conf.rs | 1 + clippy_lints/src/len_zero.rs | 228 +++++++++++++++++------------- clippy_lints/src/lib.rs | 2 +- tests/ui/len_zero.fixed | 4 + tests/ui/len_zero.rs | 4 + tests/ui/len_zero_unstable.fixed | 7 + tests/ui/len_zero_unstable.rs | 7 + tests/ui/len_zero_unstable.stderr | 11 ++ 9 files changed, 169 insertions(+), 96 deletions(-) create mode 100644 tests/ui/len_zero_unstable.fixed create mode 100644 tests/ui/len_zero_unstable.rs create mode 100644 tests/ui/len_zero_unstable.stderr diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index b9ecff1fa364..6569bdabf115 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -859,6 +859,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`io_other_error`](https://rust-lang.github.io/rust-clippy/master/index.html#io_other_error) * [`iter_kv_map`](https://rust-lang.github.io/rust-clippy/master/index.html#iter_kv_map) * [`legacy_numeric_constants`](https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants) +* [`len_zero`](https://rust-lang.github.io/rust-clippy/master/index.html#len_zero) * [`lines_filter_map_ok`](https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok) * [`manual_abs_diff`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_abs_diff) * [`manual_bits`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 9993765b4bdc..2a042e6c3d85 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -748,6 +748,7 @@ define_Conf! { io_other_error, iter_kv_map, legacy_numeric_constants, + len_zero, lines_filter_map_ok, manual_abs_diff, manual_bits, diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 04a8e4739b85..877bd34a732b 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -1,4 +1,6 @@ +use clippy_config::Conf; use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::msrvs::Msrv; use clippy_utils::res::{MaybeDef, MaybeTypeckRes}; use clippy_utils::source::{SpanRangeExt, snippet_with_context}; use clippy_utils::sugg::{Sugg, has_enclosing_paren}; @@ -10,12 +12,12 @@ use rustc_hir::def::Res; use rustc_hir::def_id::{DefId, DefIdSet}; use rustc_hir::{ BinOpKind, Expr, ExprKind, FnRetTy, GenericArg, GenericBound, HirId, ImplItem, ImplItemKind, ImplicitSelfKind, - Item, ItemKind, Mutability, Node, OpaqueTyOrigin, PatExprKind, PatKind, PathSegment, PrimTy, QPath, TraitItemId, - TyKind, + Item, ItemKind, Mutability, Node, OpaqueTyOrigin, PatExprKind, PatKind, PathSegment, PrimTy, QPath, RustcVersion, + StabilityLevel, StableSince, TraitItemId, TyKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, FnSig, Ty}; -use rustc_session::declare_lint_pass; +use rustc_session::impl_lint_pass; use rustc_span::source_map::Spanned; use rustc_span::symbol::kw; use rustc_span::{Ident, Span, Symbol}; @@ -120,7 +122,17 @@ declare_clippy_lint! { "checking `x == \"\"` or `x == []` (or similar) when `.is_empty()` could be used instead" } -declare_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY, COMPARISON_TO_EMPTY]); +pub struct LenZero { + msrv: Msrv, +} + +impl_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY, COMPARISON_TO_EMPTY]); + +impl LenZero { + pub fn new(conf: &'static Conf) -> Self { + Self { msrv: conf.msrv } + } +} impl<'tcx> LateLintPass<'tcx> for LenZero { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { @@ -184,7 +196,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { _ => false, } && !expr.span.from_expansion() - && has_is_empty(cx, lt.init) + && has_is_empty(cx, lt.init, self.msrv) { let mut applicability = Applicability::MachineApplicable; @@ -206,7 +218,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { && cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::PartialEq) && !expr.span.from_expansion() { - check_empty_expr( + self.check_empty_expr( cx, expr.span, lhs_expr, @@ -226,29 +238,110 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { let actual_span = span_without_enclosing_paren(cx, expr.span); match cmp { BinOpKind::Eq => { - check_cmp(cx, actual_span, left, right, "", 0); // len == 0 - check_cmp(cx, actual_span, right, left, "", 0); // 0 == len + self.check_cmp(cx, actual_span, left, right, "", 0); // len == 0 + self.check_cmp(cx, actual_span, right, left, "", 0); // 0 == len }, BinOpKind::Ne => { - check_cmp(cx, actual_span, left, right, "!", 0); // len != 0 - check_cmp(cx, actual_span, right, left, "!", 0); // 0 != len + self.check_cmp(cx, actual_span, left, right, "!", 0); // len != 0 + self.check_cmp(cx, actual_span, right, left, "!", 0); // 0 != len }, BinOpKind::Gt => { - check_cmp(cx, actual_span, left, right, "!", 0); // len > 0 - check_cmp(cx, actual_span, right, left, "", 1); // 1 > len + self.check_cmp(cx, actual_span, left, right, "!", 0); // len > 0 + self.check_cmp(cx, actual_span, right, left, "", 1); // 1 > len }, BinOpKind::Lt => { - check_cmp(cx, actual_span, left, right, "", 1); // len < 1 - check_cmp(cx, actual_span, right, left, "!", 0); // 0 < len + self.check_cmp(cx, actual_span, left, right, "", 1); // len < 1 + self.check_cmp(cx, actual_span, right, left, "!", 0); // 0 < len }, - BinOpKind::Ge => check_cmp(cx, actual_span, left, right, "!", 1), // len >= 1 - BinOpKind::Le => check_cmp(cx, actual_span, right, left, "!", 1), // 1 <= len + BinOpKind::Ge => self.check_cmp(cx, actual_span, left, right, "!", 1), // len >= 1 + BinOpKind::Le => self.check_cmp(cx, actual_span, right, left, "!", 1), // 1 <= len _ => (), } } } } +impl LenZero { + fn check_cmp( + &self, + cx: &LateContext<'_>, + span: Span, + method: &Expr<'_>, + lit: &Expr<'_>, + op: &str, + compare_to: u32, + ) { + if method.span.from_expansion() { + return; + } + + if let (&ExprKind::MethodCall(method_path, receiver, [], _), ExprKind::Lit(lit)) = (&method.kind, &lit.kind) { + // check if we are in an is_empty() method + if parent_item_name(cx, method) == Some(sym::is_empty) { + return; + } + + self.check_len(cx, span, method_path.ident.name, receiver, &lit.node, op, compare_to); + } else { + self.check_empty_expr(cx, span, method, lit, op); + } + } + + #[expect(clippy::too_many_arguments)] + fn check_len( + &self, + cx: &LateContext<'_>, + span: Span, + method_name: Symbol, + receiver: &Expr<'_>, + lit: &LitKind, + op: &str, + compare_to: u32, + ) { + if let LitKind::Int(lit, _) = *lit { + // check if length is compared to the specified number + if lit != u128::from(compare_to) { + return; + } + + if method_name == sym::len && has_is_empty(cx, receiver, self.msrv) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + LEN_ZERO, + span, + format!("length comparison to {}", if compare_to == 0 { "zero" } else { "one" }), + format!("using `{op}is_empty` is clearer and more explicit"), + format!( + "{op}{}.is_empty()", + snippet_with_context(cx, receiver.span, span.ctxt(), "_", &mut applicability).0, + ), + applicability, + ); + } + } + } + + fn check_empty_expr(&self, cx: &LateContext<'_>, span: Span, lit1: &Expr<'_>, lit2: &Expr<'_>, op: &str) { + if (is_empty_array(lit2) || is_empty_string(lit2)) && has_is_empty(cx, lit1, self.msrv) { + let mut applicability = Applicability::MachineApplicable; + + let lit1 = peel_ref_operators(cx, lit1); + let lit_str = Sugg::hir_with_context(cx, lit1, span.ctxt(), "_", &mut applicability).maybe_paren(); + + span_lint_and_sugg( + cx, + COMPARISON_TO_EMPTY, + span, + "comparison to empty slice", + format!("using `{op}is_empty` is clearer and more explicit"), + format!("{op}{lit_str}.is_empty()"), + applicability, + ); + } + } +} + fn span_without_enclosing_paren(cx: &LateContext<'_>, span: Span) -> Span { let Some(snippet) = span.get_source_text(cx) else { return span; @@ -513,75 +606,6 @@ fn check_for_is_empty( } } -fn check_cmp(cx: &LateContext<'_>, span: Span, method: &Expr<'_>, lit: &Expr<'_>, op: &str, compare_to: u32) { - if method.span.from_expansion() { - return; - } - - if let (&ExprKind::MethodCall(method_path, receiver, [], _), ExprKind::Lit(lit)) = (&method.kind, &lit.kind) { - // check if we are in an is_empty() method - if parent_item_name(cx, method) == Some(sym::is_empty) { - return; - } - - check_len(cx, span, method_path.ident.name, receiver, &lit.node, op, compare_to); - } else { - check_empty_expr(cx, span, method, lit, op); - } -} - -fn check_len( - cx: &LateContext<'_>, - span: Span, - method_name: Symbol, - receiver: &Expr<'_>, - lit: &LitKind, - op: &str, - compare_to: u32, -) { - if let LitKind::Int(lit, _) = *lit { - // check if length is compared to the specified number - if lit != u128::from(compare_to) { - return; - } - - if method_name == sym::len && has_is_empty(cx, receiver) { - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - LEN_ZERO, - span, - format!("length comparison to {}", if compare_to == 0 { "zero" } else { "one" }), - format!("using `{op}is_empty` is clearer and more explicit"), - format!( - "{op}{}.is_empty()", - snippet_with_context(cx, receiver.span, span.ctxt(), "_", &mut applicability).0, - ), - applicability, - ); - } - } -} - -fn check_empty_expr(cx: &LateContext<'_>, span: Span, lit1: &Expr<'_>, lit2: &Expr<'_>, op: &str) { - if (is_empty_array(lit2) || is_empty_string(lit2)) && has_is_empty(cx, lit1) { - let mut applicability = Applicability::MachineApplicable; - - let lit1 = peel_ref_operators(cx, lit1); - let lit_str = Sugg::hir_with_context(cx, lit1, span.ctxt(), "_", &mut applicability).maybe_paren(); - - span_lint_and_sugg( - cx, - COMPARISON_TO_EMPTY, - span, - "comparison to empty slice", - format!("using `{op}is_empty` is clearer and more explicit"), - format!("{op}{lit_str}.is_empty()"), - applicability, - ); - } -} - fn is_empty_string(expr: &Expr<'_>) -> bool { if let ExprKind::Lit(lit) = expr.kind && let LitKind::Str(lit, _) = lit.node @@ -600,45 +624,59 @@ fn is_empty_array(expr: &Expr<'_>) -> bool { } /// Checks if this type has an `is_empty` method. -fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { +fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>, msrv: Msrv) -> bool { /// Gets an `AssocItem` and return true if it matches `is_empty(self)`. - fn is_is_empty(cx: &LateContext<'_>, item: &ty::AssocItem) -> bool { + fn is_is_empty_and_stable(cx: &LateContext<'_>, item: &ty::AssocItem, msrv: Msrv) -> bool { if item.is_fn() { let sig = cx.tcx.fn_sig(item.def_id).skip_binder(); let ty = sig.skip_binder(); ty.inputs().len() == 1 + && cx.tcx.lookup_stability(item.def_id).is_none_or(|stability| { + if let StabilityLevel::Stable { since, .. } = stability.level { + let version = match since { + StableSince::Version(version) => version, + StableSince::Current => RustcVersion::CURRENT, + StableSince::Err(_) => return false, + }; + + msrv.meets(cx, version) + } else { + // Unstable fn, check if the feature is enabled. + cx.tcx.features().enabled(stability.feature) && msrv.current(cx).is_none() + } + }) } else { false } } /// Checks the inherent impl's items for an `is_empty(self)` method. - fn has_is_empty_impl(cx: &LateContext<'_>, id: DefId) -> bool { + fn has_is_empty_impl(cx: &LateContext<'_>, id: DefId, msrv: Msrv) -> bool { cx.tcx.inherent_impls(id).iter().any(|imp| { cx.tcx .associated_items(*imp) .filter_by_name_unhygienic(sym::is_empty) - .any(|item| is_is_empty(cx, item)) + .any(|item| is_is_empty_and_stable(cx, item, msrv)) }) } - fn ty_has_is_empty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, depth: usize) -> bool { + fn ty_has_is_empty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, depth: usize, msrv: Msrv) -> bool { match ty.kind() { ty::Dynamic(tt, ..) => tt.principal().is_some_and(|principal| { cx.tcx .associated_items(principal.def_id()) .filter_by_name_unhygienic(sym::is_empty) - .any(|item| is_is_empty(cx, item)) + .any(|item| is_is_empty_and_stable(cx, item, msrv)) }), - ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id), + ty::Alias(ty::Projection, proj) => has_is_empty_impl(cx, proj.def_id, msrv), ty::Adt(id, _) => { - has_is_empty_impl(cx, id.did()) + has_is_empty_impl(cx, id.did(), msrv) || (cx.tcx.recursion_limit().value_within_limit(depth) && cx.tcx.get_diagnostic_item(sym::Deref).is_some_and(|deref_id| { implements_trait(cx, ty, deref_id, &[]) && cx .get_associated_type(ty, deref_id, sym::Target) - .is_some_and(|deref_ty| ty_has_is_empty(cx, deref_ty, depth + 1)) + .is_some_and(|deref_ty| ty_has_is_empty(cx, deref_ty, depth + 1, msrv)) })) }, ty::Array(..) | ty::Slice(..) | ty::Str => true, @@ -646,5 +684,5 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { } } - ty_has_is_empty(cx, cx.typeck_results().expr_ty(expr).peel_refs(), 0) + ty_has_is_empty(cx, cx.typeck_results().expr_ty(expr).peel_refs(), 0, msrv) } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a4ad9424b3eb..aabeee54c308 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -481,7 +481,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(mut_mut::MutMut::default())); store.register_late_pass(|_| Box::new(unnecessary_mut_passed::UnnecessaryMutPassed)); store.register_late_pass(|_| Box::>::default()); - store.register_late_pass(|_| Box::new(len_zero::LenZero)); + store.register_late_pass(move |_| Box::new(len_zero::LenZero::new(conf))); store.register_late_pass(move |_| Box::new(attrs::Attributes::new(conf))); store.register_late_pass(|_| Box::new(blocks_in_conditions::BlocksInConditions)); store.register_late_pass(|_| Box::new(unicode::Unicode)); diff --git a/tests/ui/len_zero.fixed b/tests/ui/len_zero.fixed index b8573ef13b0e..679414f5ea4c 100644 --- a/tests/ui/len_zero.fixed +++ b/tests/ui/len_zero.fixed @@ -275,3 +275,7 @@ fn no_infinite_recursion() -> bool { // Do not crash while checking if S implements `.is_empty()` S == "" } + +fn issue15890(vertices: &mut dyn ExactSizeIterator) -> bool { + vertices.len() == 0 +} diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs index ef3c49c1ab30..a019ea7ca654 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -275,3 +275,7 @@ fn no_infinite_recursion() -> bool { // Do not crash while checking if S implements `.is_empty()` S == "" } + +fn issue15890(vertices: &mut dyn ExactSizeIterator) -> bool { + vertices.len() == 0 +} diff --git a/tests/ui/len_zero_unstable.fixed b/tests/ui/len_zero_unstable.fixed new file mode 100644 index 000000000000..8d4e6c2cc006 --- /dev/null +++ b/tests/ui/len_zero_unstable.fixed @@ -0,0 +1,7 @@ +#![warn(clippy::len_zero)] +#![feature(exact_size_is_empty)] + +fn issue15890(vertices: &mut dyn ExactSizeIterator) -> bool { + vertices.is_empty() + //~^ len_zero +} diff --git a/tests/ui/len_zero_unstable.rs b/tests/ui/len_zero_unstable.rs new file mode 100644 index 000000000000..f59056c5c55b --- /dev/null +++ b/tests/ui/len_zero_unstable.rs @@ -0,0 +1,7 @@ +#![warn(clippy::len_zero)] +#![feature(exact_size_is_empty)] + +fn issue15890(vertices: &mut dyn ExactSizeIterator) -> bool { + vertices.len() == 0 + //~^ len_zero +} diff --git a/tests/ui/len_zero_unstable.stderr b/tests/ui/len_zero_unstable.stderr new file mode 100644 index 000000000000..103ccf3dcbf5 --- /dev/null +++ b/tests/ui/len_zero_unstable.stderr @@ -0,0 +1,11 @@ +error: length comparison to zero + --> tests/ui/len_zero_unstable.rs:5:5 + | +LL | vertices.len() == 0 + | ^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `vertices.is_empty()` + | + = note: `-D clippy::len-zero` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::len_zero)]` + +error: aborting due to 1 previous error +