Skip to content

Commit c368649

Browse files
JarchoJarcho
committed
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.
1 parent 060068b commit c368649

11 files changed

+130
-31
lines changed

‎clippy_utils/src/consts.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
#![allow(clippy::float_cmp)]
22

3-
use crate::source::{span_source_range, walk_span_to_context};
3+
use crate::source::{get_source_text, walk_span_to_context};
44
use crate::{clip, is_direct_expn_of, sext, unsext};
55
use if_chain::if_chain;
66
use rustc_ast::ast::{self, LitFloatType, LitKind};
@@ -520,7 +520,7 @@ impl<'a, 'tcx> ConstEvalLateContext<'a, 'tcx> {
520520
if let Some(expr_span) = walk_span_to_context(expr.span, span.ctxt)
521521
&& let expr_lo = expr_span.lo()
522522
&& expr_lo >= span.lo
523-
&& let Some(src) = span_source_range(self.lcx, span.lo..expr_lo)
523+
&& let Some(src) = get_source_text(self.lcx, span.lo..expr_lo)
524524
&& let Some(src) = src.as_str()
525525
{
526526
use rustc_lexer::TokenKind::{Whitespace, LineComment, BlockComment, Semi, OpenBrace};

‎clippy_utils/src/hir_utils.rs

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustc_hir::{
1414
use rustc_lexer::{tokenize, TokenKind};
1515
use rustc_lint::LateContext;
1616
use rustc_middle::ty::TypeckResults;
17-
use rustc_span::{sym, BytePos, Symbol, SyntaxContext};
17+
use rustc_span::{sym, BytePos, ExpnKind, MacroKind, Symbol, SyntaxContext};
1818
use std::hash::{Hash, Hasher};
1919
use std::ops::Range;
2020

@@ -67,6 +67,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
6767
pub fn inter_expr(&mut self) -> HirEqInterExpr<'_, 'a, 'tcx> {
6868
HirEqInterExpr {
6969
inner: self,
70+
left_ctxt: SyntaxContext::root(),
71+
right_ctxt: SyntaxContext::root(),
7072
locals: HirIdMap::default(),
7173
}
7274
}
@@ -94,6 +96,8 @@ impl<'a, 'tcx> SpanlessEq<'a, 'tcx> {
9496

9597
pub struct HirEqInterExpr<'a, 'b, 'tcx> {
9698
inner: &'a mut SpanlessEq<'b, 'tcx>,
99+
left_ctxt: SyntaxContext,
100+
right_ctxt: SyntaxContext,
97101

98102
// When binding are declared, the binding ID in the left expression is mapped to the one on the
99103
// right. For example, when comparing `{ let x = 1; x + 2 }` and `{ let y = 1; y + 2 }`,
@@ -243,7 +247,7 @@ impl HirEqInterExpr<'_, '_, '_> {
243247

244248
#[expect(clippy::similar_names)]
245249
pub fn eq_expr(&mut self, left: &Expr<'_>, right: &Expr<'_>) -> bool {
246-
if !self.inner.allow_side_effects && left.span.ctxt() != right.span.ctxt() {
250+
if !self.check_ctxt(left.span.ctxt(), right.span.ctxt()) {
247251
return false;
248252
}
249253

@@ -479,6 +483,46 @@ impl HirEqInterExpr<'_, '_, '_> {
479483
fn eq_type_binding(&mut self, left: &TypeBinding<'_>, right: &TypeBinding<'_>) -> bool {
480484
left.ident.name == right.ident.name && self.eq_ty(left.ty(), right.ty())
481485
}
486+
487+
fn check_ctxt(&mut self, left: SyntaxContext, right: SyntaxContext) -> bool {
488+
if self.left_ctxt == left && self.right_ctxt == right {
489+
return true;
490+
} else if self.left_ctxt == left || self.right_ctxt == right {
491+
// Only one context has changed. This can only happen if the two nodes are written differently.
492+
return false;
493+
} else if left != SyntaxContext::root() {
494+
let mut left_data = left.outer_expn_data();
495+
let mut right_data = right.outer_expn_data();
496+
let cfg = sym!(cfg);
497+
loop {
498+
use TokenKind::{BlockComment, LineComment, Whitespace};
499+
if left_data.macro_def_id != right_data.macro_def_id
500+
|| (matches!(left_data.kind, ExpnKind::Macro(MacroKind::Bang, name) if name == cfg)
501+
&& !eq_span_tokens(self.inner.cx, left_data.call_site, right_data.call_site, |t| {
502+
!matches!(t, Whitespace | LineComment { .. } | BlockComment { .. })
503+
}))
504+
{
505+
// Either a different chain of macro calls, or different arguments to the `cfg` macro.
506+
return false;
507+
}
508+
let left_ctxt = left_data.call_site.ctxt();
509+
let right_ctxt = right_data.call_site.ctxt();
510+
if left_ctxt == SyntaxContext::root() && right_ctxt == SyntaxContext::root() {
511+
break;
512+
}
513+
if left_ctxt == SyntaxContext::root() || right_ctxt == SyntaxContext::root() {
514+
// Different lengths for the expansion stack. This can only happen if nodes are written differently,
515+
// or shouldn't be compared to start with.
516+
return false;
517+
}
518+
left_data = left_ctxt.outer_expn_data();
519+
right_data = right_ctxt.outer_expn_data();
520+
}
521+
}
522+
self.left_ctxt = left;
523+
self.right_ctxt = right;
524+
true
525+
}
482526
}
483527

484528
/// Some simple reductions like `{ return }` => `return`

‎tests/ui/collapsible_if.fixed

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
// run-rustfix
2-
#![allow(clippy::assertions_on_constants, clippy::equatable_if_let)]
2+
#![allow(
3+
clippy::assertions_on_constants,
4+
clippy::equatable_if_let,
5+
clippy::nonminimal_bool,
6+
clippy::eq_op
7+
z)]
38

49
#[rustfmt::skip]
510
#[warn(clippy::collapsible_if)]

‎tests/ui/collapsible_if.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
// run-rustfix
2-
#![allow(clippy::assertions_on_constants, clippy::equatable_if_let)]
2+
#![allow(
3+
clippy::assertions_on_constants,
4+
clippy::equatable_if_let,
5+
clippy::nonminimal_bool,
6+
clippy::eq_op
7+
)]
38

49
#[rustfmt::skip]
510
#[warn(clippy::collapsible_if)]

‎tests/ui/collapsible_if.stderr

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: this `if` statement can be collapsed
2-
--> $DIR/collapsible_if.rs:9:5
2+
--> $DIR/collapsible_if.rs:14:5
33
|
44
LL | / if x == "hello" {
55
LL | | if y == "world" {
@@ -17,7 +17,7 @@ LL + }
1717
|
1818

1919
error: this `if` statement can be collapsed
20-
--> $DIR/collapsible_if.rs:15:5
20+
--> $DIR/collapsible_if.rs:20:5
2121
|
2222
LL | / if x == "hello" || x == "world" {
2323
LL | | if y == "world" || y == "hello" {
@@ -34,7 +34,7 @@ LL + }
3434
|
3535

3636
error: this `if` statement can be collapsed
37-
--> $DIR/collapsible_if.rs:21:5
37+
--> $DIR/collapsible_if.rs:26:5
3838
|
3939
LL | / if x == "hello" && x == "world" {
4040
LL | | if y == "world" || y == "hello" {
@@ -51,7 +51,7 @@ LL + }
5151
|
5252

5353
error: this `if` statement can be collapsed
54-
--> $DIR/collapsible_if.rs:27:5
54+
--> $DIR/collapsible_if.rs:32:5
5555
|
5656
LL | / if x == "hello" || x == "world" {
5757
LL | | if y == "world" && y == "hello" {
@@ -68,7 +68,7 @@ LL + }
6868
|
6969

7070
error: this `if` statement can be collapsed
71-
--> $DIR/collapsible_if.rs:33:5
71+
--> $DIR/collapsible_if.rs:38:5
7272
|
7373
LL | / if x == "hello" && x == "world" {
7474
LL | | if y == "world" && y == "hello" {
@@ -85,7 +85,7 @@ LL + }
8585
|
8686

8787
error: this `if` statement can be collapsed
88-
--> $DIR/collapsible_if.rs:39:5
88+
--> $DIR/collapsible_if.rs:44:5
8989
|
9090
LL | / if 42 == 1337 {
9191
LL | | if 'a' != 'A' {
@@ -102,7 +102,7 @@ LL + }
102102
|
103103

104104
error: this `if` statement can be collapsed
105-
--> $DIR/collapsible_if.rs:95:5
105+
--> $DIR/collapsible_if.rs:100:5
106106
|
107107
LL | / if x == "hello" {
108108
LL | | if y == "world" { // Collapsible
@@ -119,15 +119,15 @@ LL + }
119119
|
120120

121121
error: this `if` statement can be collapsed
122-
--> $DIR/collapsible_if.rs:154:5
122+
--> $DIR/collapsible_if.rs:159:5
123123
|
124124
LL | / if matches!(true, true) {
125125
LL | | if matches!(true, true) {}
126126
LL | | }
127127
| |_____^ help: collapse nested if block: `if matches!(true, true) && matches!(true, true) {}`
128128

129129
error: this `if` statement can be collapsed
130-
--> $DIR/collapsible_if.rs:159:5
130+
--> $DIR/collapsible_if.rs:164:5
131131
|
132132
LL | / if matches!(true, true) && truth() {
133133
LL | | if matches!(true, true) {}

‎tests/ui/match_same_arms.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,16 @@ macro_rules! m {
5757
(foo) => {};
5858
(bar) => {};
5959
}
60+
macro_rules! foo {
61+
() => {
62+
1
63+
};
64+
}
65+
macro_rules! bar {
66+
() => {
67+
1
68+
};
69+
}
6070

6171
fn main() {
6272
let x = 0;
@@ -111,4 +121,16 @@ fn main() {
111121
},
112122
_ => 0,
113123
};
124+
125+
let _ = match 0 {
126+
0 => foo!(),
127+
1 => bar!(),
128+
_ => 1,
129+
};
130+
131+
let _ = match 0 {
132+
0 => cfg!(not_enabled),
133+
1 => cfg!(also_not_enabled),
134+
_ => false,
135+
};
114136
}

‎tests/ui/match_same_arms2.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,4 +239,10 @@ fn main() {
239239
3 => core::convert::identity::<u32>(todo!()),
240240
_ => 5,
241241
};
242+
243+
let _ = match 0 {
244+
0 => cfg!(not_enable),
245+
1 => cfg!(not_enable),
246+
_ => false,
247+
};
242248
}

‎tests/ui/match_same_arms2.stderr

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,5 +192,20 @@ note: other arm here
192192
LL | Some(Bar { x: 0, y: 5, .. }) => 1,
193193
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
194194

195-
error: aborting due to 12 previous errors
195+
error: this match arm has an identical body to another arm
196+
--> $DIR/match_same_arms2.rs:245:9
197+
|
198+
LL | 1 => cfg!(not_enable),
199+
| -^^^^^^^^^^^^^^^^^^^^
200+
| |
201+
| help: try merging the arm patterns: `1 | 0`
202+
|
203+
= help: or try changing either arm body
204+
note: other arm here
205+
--> $DIR/match_same_arms2.rs:244:9
206+
|
207+
LL | 0 => cfg!(not_enable),
208+
| ^^^^^^^^^^^^^^^^^^^^^
209+
210+
error: aborting due to 13 previous errors
196211

‎tests/ui/partialeq_to_none.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// run-rustfix
22
#![warn(clippy::partialeq_to_none)]
3+
#![allow(clippy::eq_op)]
34

45
struct Foobar;
56

‎tests/ui/partialeq_to_none.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// run-rustfix
22
#![warn(clippy::partialeq_to_none)]
3+
#![allow(clippy::eq_op)]
34

45
struct Foobar;
56

‎tests/ui/partialeq_to_none.stderr

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,61 @@
11
error: binary comparison to literal `Option::None`
2-
--> $DIR/partialeq_to_none.rs:14:8
2+
--> $DIR/partialeq_to_none.rs:15:8
33
|
44
LL | if f != None { "yay" } else { "nay" }
55
| ^^^^^^^^^ help: use `Option::is_some()` instead: `f.is_some()`
66
|
77
= note: `-D clippy::partialeq-to-none` implied by `-D warnings`
88

99
error: binary comparison to literal `Option::None`
10-
--> $DIR/partialeq_to_none.rs:44:13
10+
--> $DIR/partialeq_to_none.rs:45:13
1111
|
1212
LL | let _ = x == None;
1313
| ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
1414

1515
error: binary comparison to literal `Option::None`
16-
--> $DIR/partialeq_to_none.rs:45:13
16+
--> $DIR/partialeq_to_none.rs:46:13
1717
|
1818
LL | let _ = x != None;
1919
| ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
2020

2121
error: binary comparison to literal `Option::None`
22-
--> $DIR/partialeq_to_none.rs:46:13
22+
--> $DIR/partialeq_to_none.rs:47:13
2323
|
2424
LL | let _ = None == x;
2525
| ^^^^^^^^^ help: use `Option::is_none()` instead: `x.is_none()`
2626

2727
error: binary comparison to literal `Option::None`
28-
--> $DIR/partialeq_to_none.rs:47:13
28+
--> $DIR/partialeq_to_none.rs:48:13
2929
|
3030
LL | let _ = None != x;
3131
| ^^^^^^^^^ help: use `Option::is_some()` instead: `x.is_some()`
3232

3333
error: binary comparison to literal `Option::None`
34-
--> $DIR/partialeq_to_none.rs:49:8
34+
--> $DIR/partialeq_to_none.rs:50:8
3535
|
3636
LL | if foobar() == None {}
3737
| ^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `foobar().is_none()`
3838

3939
error: binary comparison to literal `Option::None`
40-
--> $DIR/partialeq_to_none.rs:51:8
40+
--> $DIR/partialeq_to_none.rs:52:8
4141
|
4242
LL | if bar().ok() != None {}
4343
| ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `bar().ok().is_some()`
4444

4545
error: binary comparison to literal `Option::None`
46-
--> $DIR/partialeq_to_none.rs:53:13
46+
--> $DIR/partialeq_to_none.rs:54:13
4747
|
4848
LL | let _ = Some(1 + 2) != None;
4949
| ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `Some(1 + 2).is_some()`
5050

5151
error: binary comparison to literal `Option::None`
52-
--> $DIR/partialeq_to_none.rs:55:13
52+
--> $DIR/partialeq_to_none.rs:56:13
5353
|
5454
LL | let _ = { Some(0) } == None;
5555
| ^^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `{ Some(0) }.is_none()`
5656

5757
error: binary comparison to literal `Option::None`
58-
--> $DIR/partialeq_to_none.rs:57:13
58+
--> $DIR/partialeq_to_none.rs:58:13
5959
|
6060
LL | let _ = {
6161
| _____________^
@@ -77,31 +77,31 @@ LL ~ }.is_some();
7777
|
7878

7979
error: binary comparison to literal `Option::None`
80-
--> $DIR/partialeq_to_none.rs:67:13
80+
--> $DIR/partialeq_to_none.rs:68:13
8181
|
8282
LL | let _ = optref() == &&None;
8383
| ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
8484

8585
error: binary comparison to literal `Option::None`
86-
--> $DIR/partialeq_to_none.rs:68:13
86+
--> $DIR/partialeq_to_none.rs:69:13
8787
|
8888
LL | let _ = &&None != optref();
8989
| ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
9090

9191
error: binary comparison to literal `Option::None`
92-
--> $DIR/partialeq_to_none.rs:69:13
92+
--> $DIR/partialeq_to_none.rs:70:13
9393
|
9494
LL | let _ = **optref() == None;
9595
| ^^^^^^^^^^^^^^^^^^ help: use `Option::is_none()` instead: `optref().is_none()`
9696

9797
error: binary comparison to literal `Option::None`
98-
--> $DIR/partialeq_to_none.rs:70:13
98+
--> $DIR/partialeq_to_none.rs:71:13
9999
|
100100
LL | let _ = &None != *optref();
101101
| ^^^^^^^^^^^^^^^^^^ help: use `Option::is_some()` instead: `optref().is_some()`
102102

103103
error: binary comparison to literal `Option::None`
104-
--> $DIR/partialeq_to_none.rs:73:13
104+
--> $DIR/partialeq_to_none.rs:74:13
105105
|
106106
LL | let _ = None != *x;
107107
| ^^^^^^^^^^ help: use `Option::is_some()` instead: `(*x).is_some()`

0 commit comments

Comments
 (0)