about summary refs log tree commit diff
diff options
context:
space:
mode:
authorJason Newcomb <jsnewcomb@pm.me>2023-03-10 10:43:04 -0500
committerJason Newcomb <jsnewcomb@pm.me>2023-05-18 16:42:13 -0400
commit58132cb3b0ddadc2004a121f0bd85d12bf089ff6 (patch)
treee5f20d63c7ed387b380fe27d51f2f65738e16a87
parent5351170744bdc6dfca0f57908cd072cfa84a3f30 (diff)
downloadrust-58132cb3b0ddadc2004a121f0bd85d12bf089ff6.tar.gz
rust-58132cb3b0ddadc2004a121f0bd85d12bf089ff6.zip
Improve `SpanlessEq`
* Don't consider expansions of different macros to be the same, even if they expand to the same tokens
* Don't consider `cfg!` expansions to be equal if they check different configs.
-rw-r--r--clippy_utils/src/consts.rs4
-rw-r--r--clippy_utils/src/hir_utils.rs52
-rw-r--r--clippy_utils/src/lib.rs4
-rw-r--r--tests/ui/collapsible_if.fixed7
-rw-r--r--tests/ui/collapsible_if.rs7
-rw-r--r--tests/ui/collapsible_if.stderr18
-rw-r--r--tests/ui/match_same_arms.rs22
-rw-r--r--tests/ui/match_same_arms2.rs6
-rw-r--r--tests/ui/match_same_arms2.stderr17
-rw-r--r--tests/ui/partialeq_to_none.fixed1
-rw-r--r--tests/ui/partialeq_to_none.rs1
-rw-r--r--tests/ui/partialeq_to_none.stderr30
12 files changed, 135 insertions, 34 deletions
diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs
index 34ca9b2d67e..992d41bc378 100644
--- a/clippy_utils/src/consts.rs
+++ b/clippy_utils/src/consts.rs
@@ -1,6 +1,6 @@
 #![allow(clippy::float_cmp)]
 
-use crate::source::{span_source_range, walk_span_to_context};
+use crate::source::{get_source_text, walk_span_to_context};
 use crate::{clip, is_direct_expn_of, sext, unsext};
 use if_chain::if_chain;
 use rustc_ast::ast::{self, LitFloatType, LitKind};
@@ -516,7 +516,7 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
                 if let Some(expr_span) = walk_span_to_context(expr.span, span.ctxt)
                     && let expr_lo = expr_span.lo()
                     && expr_lo >= span.lo
-                    && let Some(src) = span_source_range(self.lcx, span.lo..expr_lo)
+                    && let Some(src) = get_source_text(self.lcx, span.lo..expr_lo)
                     && let Some(src) = src.as_str()
                 {
                     use rustc_lexer::TokenKind::{Whitespace, LineComment, BlockComment, Semi, OpenBrace};
diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs
index 2aec5a10e5d..a49246a7832 100644
--- a/clippy_utils/src/hir_utils.rs
+++ b/clippy_utils/src/hir_utils.rs
@@ -14,7 +14,7 @@ use rustc_hir::{
 use rustc_lexer::{tokenize, TokenKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::TypeckResults;
-use rustc_span::{sym, BytePos, Symbol, SyntaxContext};
+use rustc_span::{sym, BytePos, ExpnKind, MacroKind, Symbol, SyntaxContext};
 use std::hash::{Hash, Hasher};
 use std::ops::Range;
 
@@ -67,6 +67,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
     pub fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> {
         HirEqInterExpr {
             inner: self,
+            left_ctxt: SyntaxContext::root(),
+            right_ctxt: SyntaxContext::root(),
             locals: HirIdMap::default(),
         }
     }
@@ -94,6 +96,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
 
 pub struct HirEqInterExpr<'a, 'b, 'tcx> {
     inner: &'a mut SpanlessEq<'b, 'tcx>,
+    left_ctxt: SyntaxContext,
+    right_ctxt: SyntaxContext,
 
     // When binding are declared, the binding ID in the left expression is mapped to the one on the
     // right. For example, when comparing `{ let x = 1; x + 2 }` and `{ let y = 1; y + 2 }`,
@@ -128,6 +132,7 @@ impl HirEqInterExpr<'_, '_, '_> {
     }
 
     /// Checks whether two blocks are the same.
+    #[expect(clippy::similar_names)]
     fn eq_block(&mut self, left: &Block<'_>, right: &Block<'_>) -> bool {
         use TokenKind::{BlockComment, LineComment, Semi, Whitespace};
         if left.stmts.len() != right.stmts.len() {
@@ -166,7 +171,8 @@ impl HirEqInterExpr<'_, '_, '_> {
                 // Can happen when macros expand to multiple statements, or rearrange statements.
                 // Nothing in between the statements to check in this case.
                 continue;
-            } else if lstmt_span.lo < lstart || rstmt_span.lo < rstart {
+            }
+            if lstmt_span.lo < lstart || rstmt_span.lo < rstart {
                 // Only one of the blocks had a weird macro.
                 return false;
             }
@@ -243,7 +249,7 @@ impl HirEqInterExpr<'_, '_, '_> {
 
     #[expect(clippy::similar_names)]
     pub fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool {
-        if !self.inner.allow_side_effects && left.span.ctxt() != right.span.ctxt() {
+        if !self.check_ctxt(left.span.ctxt(), right.span.ctxt()) {
             return false;
         }
 
@@ -476,6 +482,45 @@ impl HirEqInterExpr<'_, '_, '_> {
     fn eq_type_binding(&mut self, left: &TypeBinding<'_>, right: &TypeBinding<'_>) -> bool {
         left.ident.name == right.ident.name && self.eq_ty(left.ty(), right.ty())
     }
+
+    fn check_ctxt(&mut self, left: SyntaxContext, right: SyntaxContext) -> bool {
+        if self.left_ctxt == left && self.right_ctxt == right {
+            return true;
+        } else if self.left_ctxt == left || self.right_ctxt == right {
+            // Only one context has changed. This can only happen if the two nodes are written differently.
+            return false;
+        } else if left != SyntaxContext::root() {
+            let mut left_data = left.outer_expn_data();
+            let mut right_data = right.outer_expn_data();
+            loop {
+                use TokenKind::{BlockComment, LineComment, Whitespace};
+                if left_data.macro_def_id != right_data.macro_def_id
+                    || (matches!(left_data.kind, ExpnKind::Macro(MacroKind::Bang, name) if name == sym::cfg)
+                        && !eq_span_tokens(self.inner.cx, left_data.call_site, right_data.call_site, |t| {
+                            !matches!(t, Whitespace | LineComment { .. } | BlockComment { .. })
+                        }))
+                {
+                    // Either a different chain of macro calls, or different arguments to the `cfg` macro.
+                    return false;
+                }
+                let left_ctxt = left_data.call_site.ctxt();
+                let right_ctxt = right_data.call_site.ctxt();
+                if left_ctxt == SyntaxContext::root() && right_ctxt == SyntaxContext::root() {
+                    break;
+                }
+                if left_ctxt == SyntaxContext::root() || right_ctxt == SyntaxContext::root() {
+                    // Different lengths for the expansion stack. This can only happen if nodes are written differently,
+                    // or shouldn't be compared to start with.
+                    return false;
+                }
+                left_data = left_ctxt.outer_expn_data();
+                right_data = right_ctxt.outer_expn_data();
+            }
+        }
+        self.left_ctxt = left;
+        self.right_ctxt = right;
+        true
+    }
 }
 
 /// Some simple reductions like `{ return }` => `return`
@@ -1075,6 +1120,7 @@ pub fn hash_expr(cx: &LateContext<'_>, e: &Expr<'_>) -> u64 {
     h.finish()
 }
 
+#[expect(clippy::similar_names)]
 fn eq_span_tokens(
     cx: &LateContext<'_>,
     left: impl SpanRange,
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index c857595b844..575c29a6b6f 100644
--- a/clippy_utils/src/lib.rs
+++ b/clippy_utils/src/lib.rs
@@ -1499,7 +1499,7 @@ pub fn is_range_full(cx: &LateContext<'_>, expr: &Expr<'_>, container_path: Opti
                 && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, min_val.to_valtree()))
                 && let min_const_kind = ConstantKind::from_value(const_val, bnd_ty)
                 && let Some(min_const) = miri_to_const(cx.tcx, min_const_kind)
-                && let Some((start_const, _)) = constant(cx, cx.typeck_results(), start)
+                && let Some(start_const) = constant(cx, cx.typeck_results(), start)
             {
                 start_const == min_const
             } else {
@@ -1515,7 +1515,7 @@ pub fn is_range_full(cx: &LateContext<'_>, expr: &Expr<'_>, container_path: Opti
                         && let const_val = cx.tcx.valtree_to_const_val((bnd_ty, max_val.to_valtree()))
                         && let max_const_kind = ConstantKind::from_value(const_val, bnd_ty)
                         && let Some(max_const) = miri_to_const(cx.tcx, max_const_kind)
-                        && let Some((end_const, _)) = constant(cx, cx.typeck_results(), end)
+                        && let Some(end_const) = constant(cx, cx.typeck_results(), end)
                     {
                         end_const == max_const
                     } else {
diff --git a/tests/ui/collapsible_if.fixed b/tests/ui/collapsible_if.fixed
index d2aba2ac59b..c6514a55934 100644
--- a/tests/ui/collapsible_if.fixed
+++ b/tests/ui/collapsible_if.fixed
@@ -1,5 +1,10 @@
 //@run-rustfix
-#![allow(clippy::assertions_on_constants, clippy::equatable_if_let)]
+#![allow(
+    clippy::assertions_on_constants,
+    clippy::equatable_if_let,
+    clippy::nonminimal_bool,
+    clippy::eq_op
+)]
 
 #[rustfmt::skip]
 #[warn(clippy::collapsible_if)]
diff --git a/tests/ui/collapsible_if.rs b/tests/ui/collapsible_if.rs
index e0bef7f9c97..2c85b68df63 100644
--- a/tests/ui/collapsible_if.rs
+++ b/tests/ui/collapsible_if.rs
@@ -1,5 +1,10 @@
 //@run-rustfix
-#![allow(clippy::assertions_on_constants, clippy::equatable_if_let)]
+#![allow(
+    clippy::assertions_on_constants,
+    clippy::equatable_if_let,
+    clippy::nonminimal_bool,
+    clippy::eq_op
+)]
 
 #[rustfmt::skip]
 #[warn(clippy::collapsible_if)]
diff --git a/tests/ui/collapsible_if.stderr b/tests/ui/collapsible_if.stderr
index 6327444df21..c687bae1acc 100644
--- a/tests/ui/collapsible_if.stderr
+++ b/tests/ui/collapsible_if.stderr
@@ -1,5 +1,5 @@
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:9:5
+  --> $DIR/collapsible_if.rs:14:5
    |
 LL | /     if x == "hello" {
 LL | |         if y == "world" {
@@ -17,7 +17,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:15:5
+  --> $DIR/collapsible_if.rs:20:5
    |
 LL | /     if x == "hello" || x == "world" {
 LL | |         if y == "world" || y == "hello" {
@@ -34,7 +34,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:21:5
+  --> $DIR/collapsible_if.rs:26:5
    |
 LL | /     if x == "hello" && x == "world" {
 LL | |         if y == "world" || y == "hello" {
@@ -51,7 +51,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:27:5
+  --> $DIR/collapsible_if.rs:32:5
    |
 LL | /     if x == "hello" || x == "world" {
 LL | |         if y == "world" && y == "hello" {
@@ -68,7 +68,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:33:5
+  --> $DIR/collapsible_if.rs:38:5
    |
 LL | /     if x == "hello" && x == "world" {
 LL | |         if y == "world" && y == "hello" {
@@ -85,7 +85,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:39:5
+  --> $DIR/collapsible_if.rs:44:5
    |
 LL | /     if 42 == 1337 {
 LL | |         if 'a' != 'A' {
@@ -102,7 +102,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:95:5
+  --> $DIR/collapsible_if.rs:100:5
    |
 LL | /     if x == "hello" {
 LL | |         if y == "world" { // Collapsible
@@ -119,7 +119,7 @@ LL +     }
    |
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:154:5
+  --> $DIR/collapsible_if.rs:159:5
    |
 LL | /     if matches!(true, true) {
 LL | |         if matches!(true, true) {}
@@ -127,7 +127,7 @@ LL | |     }
    | |_____^ help: collapse nested if block: `if matches!(true, true) && matches!(true, true) {}`
 
 error: this `if` statement can be collapsed
-  --> $DIR/collapsible_if.rs:159:5
+  --> $DIR/collapsible_if.rs:164:5
    |
 LL | /     if matches!(true, true) && truth() {
 LL | |         if matches!(true, true) {}
diff --git a/tests/ui/match_same_arms.rs b/tests/ui/match_same_arms.rs
index 26e2ca86758..3914b45464c 100644
--- a/tests/ui/match_same_arms.rs
+++ b/tests/ui/match_same_arms.rs
@@ -57,6 +57,16 @@ macro_rules! m {
     (foo) => {};
     (bar) => {};
 }
+macro_rules! foo {
+    () => {
+        1
+    };
+}
+macro_rules! bar {
+    () => {
+        1
+    };
+}
 
 fn main() {
     let x = 0;
@@ -111,4 +121,16 @@ fn main() {
         },
         _ => 0,
     };
+
+    let _ = match 0 {
+        0 => foo!(),
+        1 => bar!(),
+        _ => 1,
+    };
+
+    let _ = match 0 {
+        0 => cfg!(not_enabled),
+        1 => cfg!(also_not_enabled),
+        _ => false,
+    };
 }
diff --git a/tests/ui/match_same_arms2.rs b/tests/ui/match_same_arms2.rs
index 82b2c433d99..60b2975be04 100644
--- a/tests/ui/match_same_arms2.rs
+++ b/tests/ui/match_same_arms2.rs
@@ -239,4 +239,10 @@ fn main() {
         3 => core::convert::identity::<u32>(todo!()),
         _ => 5,
     };
+
+    let _ = match 0 {
+        0 => cfg!(not_enable),
+        1 => cfg!(not_enable),
+        _ => false,
+    };
 }
diff --git a/tests/ui/match_same_arms2.stderr b/tests/ui/match_same_arms2.stderr
index 06cd4300054..8fb461bd286 100644
--- a/tests/ui/match_same_arms2.stderr
+++ b/tests/ui/match_same_arms2.stderr
@@ -192,5 +192,20 @@ note: other arm here
 LL |         Some(Bar { x: 0, y: 5, .. }) => 1,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: aborting due to 12 previous errors
+error: this match arm has an identical body to another arm
+  --> $DIR/match_same_arms2.rs:245:9
+   |
+LL |         1 => cfg!(not_enable),
+   |         -^^^^^^^^^^^^^^^^^^^^
+   |         |
+   |         help: try merging the arm patterns: `1 | 0`
+   |
+   = help: or try changing either arm body
+note: other arm here
+  --> $DIR/match_same_arms2.rs:244:9
+   |
+LL |         0 => cfg!(not_enable),
+   |         ^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 13 previous errors
 
diff --git a/tests/ui/partialeq_to_none.fixed b/tests/ui/partialeq_to_none.fixed
index 81a716bd276..2df87a26d6d 100644
--- a/tests/ui/partialeq_to_none.fixed
+++ b/tests/ui/partialeq_to_none.fixed
@@ -1,5 +1,6 @@
 //@run-rustfix
 #![warn(clippy::partialeq_to_none)]
+#![allow(clippy::eq_op)]
 
 struct Foobar;
 
diff --git a/tests/ui/partialeq_to_none.rs b/tests/ui/partialeq_to_none.rs
index f454715fa30..df6233b9afd 100644
--- a/tests/ui/partialeq_to_none.rs
+++ b/tests/ui/partialeq_to_none.rs
@@ -1,5 +1,6 @@
 //@run-rustfix
 #![warn(clippy::partialeq_to_none)]
+#![allow(clippy::eq_op)]
 
 struct Foobar;
 
diff --git a/tests/ui/partialeq_to_none.stderr b/tests/ui/partialeq_to_none.stderr
index d06ab7aee55..4f84862a22b 100644
--- a/tests/ui/partialeq_to_none.stderr
+++ b/tests/ui/partialeq_to_none.stderr
@@ -1,5 +1,5 @@
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:14:8
+  --> $DIR/partialeq_to_none.rs:15:8
    |
 LL |     if f != None { "yay" } else { "nay" }
    |        ^^^^^^^^^ help: use `Option::is_some()` instead: `f.is_some()`
@@ -7,55 +7,55 @@ LL |     if f != None { "yay" } else { "nay" }
    = note: `-D clippy::partialeq-to-none` implied by `-D warnings`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:44:13
+  --> $DIR/partialeq_to_none.rs:45:13
    |
 LL |     let _ = x == None;
    |             ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:45:13
+  --> $DIR/partialeq_to_none.rs:46:13
    |
 LL |     let _ = x != None;
    |             ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:46:13
+  --> $DIR/partialeq_to_none.rs:47:13
    |
 LL |     let _ = None == x;
    |             ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:47:13
+  --> $DIR/partialeq_to_none.rs:48:13
    |
 LL |     let _ = None != x;
    |             ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:49:8
+  --> $DIR/partialeq_to_none.rs:50:8
    |
 LL |     if foobar() == None {}
    |        ^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `foobar().is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:51:8
+  --> $DIR/partialeq_to_none.rs:52:8
    |
 LL |     if bar().ok() != None {}
    |        ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `bar().ok().is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:53:13
+  --> $DIR/partialeq_to_none.rs:54:13
    |
 LL |     let _ = Some(1 + 2) != None;
    |             ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `Some(1 + 2).is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:55:13
+  --> $DIR/partialeq_to_none.rs:56:13
    |
 LL |     let _ = { Some(0) } == None;
    |             ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `{ Some(0) }.is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:57:13
+  --> $DIR/partialeq_to_none.rs:58:13
    |
 LL |       let _ = {
    |  _____________^
@@ -77,31 +77,31 @@ LL ~     }.is_some();
    |
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:67:13
+  --> $DIR/partialeq_to_none.rs:68:13
    |
 LL |     let _ = optref() == &&None;
    |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:68:13
+  --> $DIR/partialeq_to_none.rs:69:13
    |
 LL |     let _ = &&None != optref();
    |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:69:13
+  --> $DIR/partialeq_to_none.rs:70:13
    |
 LL |     let _ = **optref() == None;
    |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:70:13
+  --> $DIR/partialeq_to_none.rs:71:13
    |
 LL |     let _ = &None != *optref();
    |             ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
 
 error: binary comparison to literal `Option::None`
-  --> $DIR/partialeq_to_none.rs:73:13
+  --> $DIR/partialeq_to_none.rs:74:13
    |
 LL |     let _ = None != *x;
    |             ^^^^^^^^^^ help: use `Option::is_some()` instead: `(*x).is_some()`