From b456ed31e474c65bfc8f80c71dd0762b6a434857 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 2 Apr 2024 01:27:17 +0800 Subject: [PATCH] fix suggestion for [`len_zero`] with macros --- clippy_lints/src/len_zero.rs | 24 ++++++++++++++++++++--- tests/ui/len_zero.fixed | 37 ++++++++++++++++++++++++++++++++++++ tests/ui/len_zero.rs | 37 ++++++++++++++++++++++++++++++++++++ tests/ui/len_zero.stderr | 20 ++++++++++++++++++- 4 files changed, 114 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index ef52121ce3e..5aa4e99121e 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -1,6 +1,6 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then}; -use clippy_utils::source::snippet_with_context; -use clippy_utils::sugg::Sugg; +use clippy_utils::source::{snippet_opt, snippet_with_context}; +use clippy_utils::sugg::{has_enclosing_paren, Sugg}; use clippy_utils::{get_item_name, get_parent_as_impl, is_lint_allowed, peel_ref_operators}; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; @@ -192,7 +192,7 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { if let ExprKind::Binary(Spanned { node: cmp, .. }, left, right) = expr.kind { // expr.span might contains parenthesis, see issue #10529 - let actual_span = left.span.with_hi(right.span.hi()); + let actual_span = span_without_enclosing_paren(cx, expr.span); match cmp { BinOpKind::Eq => { check_cmp(cx, actual_span, left, right, "", 0); // len == 0 @@ -218,6 +218,20 @@ impl<'tcx> LateLintPass<'tcx> for LenZero { } } +fn span_without_enclosing_paren(cx: &LateContext<'_>, span: Span) -> Span { + let Some(snippet) = snippet_opt(cx, span) else { + return span; + }; + if has_enclosing_paren(snippet) { + let source_map = cx.tcx.sess.source_map(); + let left_paren = source_map.start_point(span); + let right_parent = source_map.end_point(span); + left_paren.between(right_parent) + } else { + span + } +} + fn check_trait_items(cx: &LateContext<'_>, visited_trait: &Item<'_>, trait_items: &[TraitItemRef]) { fn is_named_self(cx: &LateContext<'_>, item: &TraitItemRef, name: Symbol) -> bool { item.ident.name == name @@ -495,6 +509,10 @@ 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, args, _), ExprKind::Lit(lit)) = (&method.kind, &lit.kind) { // check if we are in an is_empty() method if let Some(name) = get_item_name(cx, method) { diff --git a/tests/ui/len_zero.fixed b/tests/ui/len_zero.fixed index c16d7a26616..27319d9c20e 100644 --- a/tests/ui/len_zero.fixed +++ b/tests/ui/len_zero.fixed @@ -189,3 +189,40 @@ fn main() { fn test_slice(b: &[u8]) { if !b.is_empty() {} } + +// issue #11992 +fn binop_with_macros() { + macro_rules! len { + ($seq:ident) => { + $seq.len() + }; + } + + macro_rules! compare_to { + ($val:literal) => { + $val + }; + ($val:expr) => {{ $val }}; + } + + macro_rules! zero { + () => { + 0 + }; + } + + let has_is_empty = HasIsEmpty; + // Don't lint, suggesting changes might break macro compatibility. + (len!(has_is_empty) > 0).then(|| println!("This can happen.")); + // Don't lint, suggesting changes might break macro compatibility. + if len!(has_is_empty) == 0 {} + // Don't lint + if has_is_empty.len() == compare_to!(if true { 0 } else { 1 }) {} + // This is fine + if has_is_empty.len() == compare_to!(1) {} + + if has_is_empty.is_empty() {} + if has_is_empty.is_empty() {} + + (!has_is_empty.is_empty()).then(|| println!("This can happen.")); +} diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs index 5c49a5abf81..03c05bc6ed7 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -189,3 +189,40 @@ fn main() { fn test_slice(b: &[u8]) { if b.len() != 0 {} } + +// issue #11992 +fn binop_with_macros() { + macro_rules! len { + ($seq:ident) => { + $seq.len() + }; + } + + macro_rules! compare_to { + ($val:literal) => { + $val + }; + ($val:expr) => {{ $val }}; + } + + macro_rules! zero { + () => { + 0 + }; + } + + let has_is_empty = HasIsEmpty; + // Don't lint, suggesting changes might break macro compatibility. + (len!(has_is_empty) > 0).then(|| println!("This can happen.")); + // Don't lint, suggesting changes might break macro compatibility. + if len!(has_is_empty) == 0 {} + // Don't lint + if has_is_empty.len() == compare_to!(if true { 0 } else { 1 }) {} + // This is fine + if has_is_empty.len() == compare_to!(1) {} + + if has_is_empty.len() == compare_to!(0) {} + if has_is_empty.len() == zero!() {} + + (compare_to!(0) < has_is_empty.len()).then(|| println!("This can happen.")); +} diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr index dd07a85d62c..5c849a2aca6 100644 --- a/tests/ui/len_zero.stderr +++ b/tests/ui/len_zero.stderr @@ -142,5 +142,23 @@ error: length comparison to zero LL | if b.len() != 0 {} | ^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!b.is_empty()` -error: aborting due to 23 previous errors +error: length comparison to zero + --> tests/ui/len_zero.rs:224:8 + | +LL | if has_is_empty.len() == compare_to!(0) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` + +error: length comparison to zero + --> tests/ui/len_zero.rs:225:8 + | +LL | if has_is_empty.len() == zero!() {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `has_is_empty.is_empty()` + +error: length comparison to zero + --> tests/ui/len_zero.rs:227:6 + | +LL | (compare_to!(0) < has_is_empty.len()).then(|| println!("This can happen.")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!has_is_empty.is_empty()` + +error: aborting due to 26 previous errors