1 use clippy_utils
::ast_utils
::{eq_id, is_useless_with_eq_exprs, IdentIter}
;
2 use clippy_utils
::diagnostics
::span_lint_and_sugg
;
3 use clippy_utils
::source
::snippet_with_applicability
;
4 use core
::ops
::{Add, AddAssign}
;
5 use if_chain
::if_chain
;
6 use rustc_ast
::ast
::{BinOpKind, Expr, ExprKind, StmtKind}
;
7 use rustc_data_structures
::fx
::FxHashSet
;
8 use rustc_errors
::Applicability
;
9 use rustc_lint
::{EarlyContext, EarlyLintPass}
;
10 use rustc_session
::{declare_lint_pass, declare_tool_lint}
;
11 use rustc_span
::source_map
::Spanned
;
12 use rustc_span
::symbol
::Ident
;
15 declare_clippy_lint
! {
17 /// Checks for unlikely usages of binary operators that are almost
18 /// certainly typos and/or copy/paste errors, given the other usages
19 /// of binary operators nearby.
21 /// ### Why is this bad?
22 /// They are probably bugs and if they aren't then they look like bugs
23 /// and you should add a comment explaining why you are doing such an
24 /// odd set of operations.
26 /// ### Known problems
27 /// There may be some false positives if you are trying to do something
28 /// unusual that happens to look like a typo.
38 /// impl Eq for Vec3 {}
40 /// impl PartialEq for Vec3 {
41 /// fn eq(&self, other: &Self) -> bool {
42 /// // This should trigger the lint because `self.x` is compared to `other.y`
43 /// self.x == other.y && self.y == other.y && self.z == other.z
54 /// // same as above except:
55 /// impl PartialEq for Vec3 {
56 /// fn eq(&self, other: &Self) -> bool {
57 /// // Note we now compare other.x to self.x
58 /// self.x == other.x && self.y == other.y && self.z == other.z
62 #[clippy::version = "1.50.0"]
63 pub SUSPICIOUS_OPERATION_GROUPINGS
,
65 "groupings of binary operations that look suspiciously like typos"
68 declare_lint_pass
!(SuspiciousOperationGroupings
=> [SUSPICIOUS_OPERATION_GROUPINGS
]);
70 impl EarlyLintPass
for SuspiciousOperationGroupings
{
71 fn check_expr(&mut self, cx
: &EarlyContext
<'_
>, expr
: &Expr
) {
72 if expr
.span
.from_expansion() {
76 if let Some(binops
) = extract_related_binops(&expr
.kind
) {
77 check_binops(cx
, &binops
.iter().collect
::<Vec
<_
>>());
79 let mut op_types
= Vec
::with_capacity(binops
.len());
80 // We could use a hashmap, etc. to avoid being O(n*m) here, but
81 // we want the lints to be emitted in a consistent order. Besides,
82 // m, (the number of distinct `BinOpKind`s in `binops`)
83 // will often be small, and does have an upper limit.
84 binops
.iter().map(|b
| b
.op
).for_each(|op
| {
85 if !op_types
.contains(&op
) {
90 for op_type
in op_types
{
91 let ops
: Vec
<_
> = binops
.iter().filter(|b
| b
.op
== op_type
).collect();
93 check_binops(cx
, &ops
);
99 fn check_binops(cx
: &EarlyContext
<'_
>, binops
: &[&BinaryOp
<'_
>]) {
100 let binop_count
= binops
.len();
102 // Single binary operation expressions would likely be false
107 let mut one_ident_difference_count
= 0;
108 let mut no_difference_info
= None
;
109 let mut double_difference_info
= None
;
110 let mut expected_ident_loc
= None
;
112 let mut paired_identifiers
= FxHashSet
::default();
114 for (i
, BinaryOp { left, right, op, .. }
) in binops
.iter().enumerate() {
115 match ident_difference_expr(left
, right
) {
116 IdentDifference
::NoDifference
=> {
117 if is_useless_with_eq_exprs(*op
) {
118 // The `eq_op` lint should catch this in this case.
122 no_difference_info
= Some(i
);
124 IdentDifference
::Single(ident_loc
) => {
125 one_ident_difference_count
+= 1;
126 if let Some(previous_expected
) = expected_ident_loc
{
127 if previous_expected
!= ident_loc
{
128 // This expression doesn't match the form we're
133 expected_ident_loc
= Some(ident_loc
);
136 // If there was only a single difference, all other idents
137 // must have been the same, and thus were paired.
138 for id
in skip_index(IdentIter
::from(*left
), ident_loc
.index
) {
139 paired_identifiers
.insert(id
);
142 IdentDifference
::Double(ident_loc1
, ident_loc2
) => {
143 double_difference_info
= Some((i
, ident_loc1
, ident_loc2
));
145 IdentDifference
::Multiple
| IdentDifference
::NonIdent
=> {
146 // It's too hard to know whether this is a bug or not.
152 let mut applicability
= Applicability
::MachineApplicable
;
154 if let Some(expected_loc
) = expected_ident_loc
{
155 match (no_difference_info
, double_difference_info
) {
156 (Some(i
), None
) => attempt_to_emit_no_difference_lint(cx
, binops
, i
, expected_loc
),
157 (None
, Some((double_difference_index
, ident_loc1
, ident_loc2
))) => {
159 if one_ident_difference_count
== binop_count
- 1;
160 if let Some(binop
) = binops
.get(double_difference_index
);
162 let changed_loc
= if ident_loc1
== expected_loc
{
164 } else if ident_loc2
== expected_loc
{
167 // This expression doesn't match the form we're
172 if let Some(sugg
) = ident_swap_sugg(
194 fn attempt_to_emit_no_difference_lint(
195 cx
: &EarlyContext
<'_
>,
196 binops
: &[&BinaryOp
<'_
>],
198 expected_loc
: IdentLocation
,
200 if let Some(binop
) = binops
.get(i
).copied() {
201 // We need to try and figure out which identifier we should
202 // suggest using instead. Since there could be multiple
203 // replacement candidates in a given expression, and we're
204 // just taking the first one, we may get some bad lint
206 let mut applicability
= Applicability
::MaybeIncorrect
;
208 // We assume that the correct ident is one used elsewhere in
209 // the other binops, in a place that there was a single
210 // difference between idents before.
211 let old_left_ident
= get_ident(binop
.left
, expected_loc
);
212 let old_right_ident
= get_ident(binop
.right
, expected_loc
);
214 for b
in skip_index(binops
.iter(), i
) {
216 if let (Some(old_ident
), Some(new_ident
)) =
217 (old_left_ident
, get_ident(b
.left
, expected_loc
));
218 if old_ident
!= new_ident
;
219 if let Some(sugg
) = suggestion_with_swapped_ident(
230 replace_left_sugg(cx
, binop
, &sugg
, &mut applicability
),
238 if let (Some(old_ident
), Some(new_ident
)) =
239 (old_right_ident
, get_ident(b
.right
, expected_loc
));
240 if old_ident
!= new_ident
;
241 if let Some(sugg
) = suggestion_with_swapped_ident(
252 replace_right_sugg(cx
, binop
, &sugg
, &mut applicability
),
262 fn emit_suggestion(cx
: &EarlyContext
<'_
>, span
: Span
, sugg
: String
, applicability
: Applicability
) {
265 SUSPICIOUS_OPERATION_GROUPINGS
,
267 "this sequence of operators looks suspiciously like a bug",
275 cx
: &EarlyContext
<'_
>,
276 paired_identifiers
: &FxHashSet
<Ident
>,
277 binop
: &BinaryOp
<'_
>,
278 location
: IdentLocation
,
279 applicability
: &mut Applicability
,
280 ) -> Option
<String
> {
281 let left_ident
= get_ident(binop
.left
, location
)?
;
282 let right_ident
= get_ident(binop
.right
, location
)?
;
285 paired_identifiers
.contains(&left_ident
),
286 paired_identifiers
.contains(&right_ident
),
288 (true, true) | (false, false) => {
289 // We don't have a good guess of what ident should be
290 // used instead, in these cases.
291 *applicability
= Applicability
::MaybeIncorrect
;
293 // We arbitrarily choose one side to suggest changing,
294 // since we don't have a better guess. If the user
295 // ends up duplicating a clause, the `logic_bug` lint
298 let right_suggestion
= suggestion_with_swapped_ident(cx
, binop
.right
, location
, left_ident
, applicability
)?
;
300 replace_right_sugg(cx
, binop
, &right_suggestion
, applicability
)
303 // We haven't seen a pair involving the left one, so
304 // it's probably what is wanted.
306 let right_suggestion
= suggestion_with_swapped_ident(cx
, binop
.right
, location
, left_ident
, applicability
)?
;
308 replace_right_sugg(cx
, binop
, &right_suggestion
, applicability
)
311 // We haven't seen a pair involving the right one, so
312 // it's probably what is wanted.
313 let left_suggestion
= suggestion_with_swapped_ident(cx
, binop
.left
, location
, right_ident
, applicability
)?
;
315 replace_left_sugg(cx
, binop
, &left_suggestion
, applicability
)
322 fn replace_left_sugg(
323 cx
: &EarlyContext
<'_
>,
324 binop
: &BinaryOp
<'_
>,
325 left_suggestion
: &str,
326 applicability
: &mut Applicability
,
329 "{left_suggestion} {} {}",
330 binop
.op
.to_string(),
331 snippet_with_applicability(cx
, binop
.right
.span
, "..", applicability
),
335 fn replace_right_sugg(
336 cx
: &EarlyContext
<'_
>,
337 binop
: &BinaryOp
<'_
>,
338 right_suggestion
: &str,
339 applicability
: &mut Applicability
,
342 "{} {} {right_suggestion}",
343 snippet_with_applicability(cx
, binop
.left
.span
, "..", applicability
),
344 binop
.op
.to_string(),
348 #[derive(Clone, Debug)]
349 struct BinaryOp
<'exprs
> {
356 impl<'exprs
> BinaryOp
<'exprs
> {
357 fn new(op
: BinOpKind
, span
: Span
, (left
, right
): (&'exprs Expr
, &'exprs Expr
)) -> Self {
358 Self { op, span, left, right }
362 fn strip_non_ident_wrappers(expr
: &Expr
) -> &Expr
{
363 let mut output
= expr
;
365 output
= match &output
.kind
{
366 ExprKind
::Paren(ref inner
) | ExprKind
::Unary(_
, ref inner
) => inner
,
374 fn extract_related_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
375 append_opt_vecs(chained_binops(kind
), if_statement_binops(kind
))
378 fn if_statement_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
380 ExprKind
::If(ref condition
, _
, _
) => chained_binops(&condition
.kind
),
381 ExprKind
::Paren(ref e
) => if_statement_binops(&e
.kind
),
382 ExprKind
::Block(ref block
, _
) => {
383 let mut output
= None
;
384 for stmt
in &block
.stmts
{
386 StmtKind
::Expr(ref e
) | StmtKind
::Semi(ref e
) => {
387 output
= append_opt_vecs(output
, if_statement_binops(&e
.kind
));
398 fn append_opt_vecs
<A
>(target_opt
: Option
<Vec
<A
>>, source_opt
: Option
<Vec
<A
>>) -> Option
<Vec
<A
>> {
399 match (target_opt
, source_opt
) {
400 (Some(mut target
), Some(source
)) => {
401 target
.reserve(source
.len());
407 (Some(v
), None
) | (None
, Some(v
)) => Some(v
),
408 (None
, None
) => None
,
412 fn chained_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
414 ExprKind
::Binary(_
, left_outer
, right_outer
) => chained_binops_helper(left_outer
, right_outer
),
415 ExprKind
::Paren(ref e
) | ExprKind
::Unary(_
, ref e
) => chained_binops(&e
.kind
),
420 fn chained_binops_helper
<'expr
>(left_outer
: &'expr Expr
, right_outer
: &'expr Expr
) -> Option
<Vec
<BinaryOp
<'expr
>>> {
421 match (&left_outer
.kind
, &right_outer
.kind
) {
423 ExprKind
::Paren(ref left_e
) | ExprKind
::Unary(_
, ref left_e
),
424 ExprKind
::Paren(ref right_e
) | ExprKind
::Unary(_
, ref right_e
),
425 ) => chained_binops_helper(left_e
, right_e
),
426 (ExprKind
::Paren(ref left_e
) | ExprKind
::Unary(_
, ref left_e
), _
) => chained_binops_helper(left_e
, right_outer
),
427 (_
, ExprKind
::Paren(ref right_e
) | ExprKind
::Unary(_
, ref right_e
)) => {
428 chained_binops_helper(left_outer
, right_e
)
431 ExprKind
::Binary(Spanned { node: left_op, .. }
, ref left_left
, ref left_right
),
432 ExprKind
::Binary(Spanned { node: right_op, .. }
, ref right_left
, ref right_right
),
434 chained_binops_helper(left_left
, left_right
),
435 chained_binops_helper(right_left
, right_right
),
437 (Some(mut left_ops
), Some(right_ops
)) => {
438 left_ops
.reserve(right_ops
.len());
439 for op
in right_ops
{
444 (Some(mut left_ops
), _
) => {
445 left_ops
.push(BinaryOp
::new(*right_op
, right_outer
.span
, (right_left
, right_right
)));
448 (_
, Some(mut right_ops
)) => {
449 right_ops
.insert(0, BinaryOp
::new(*left_op
, left_outer
.span
, (left_left
, left_right
)));
452 (None
, None
) => Some(vec
![
453 BinaryOp
::new(*left_op
, left_outer
.span
, (left_left
, left_right
)),
454 BinaryOp
::new(*right_op
, right_outer
.span
, (right_left
, right_right
)),
461 #[derive(Clone, Copy, PartialEq, Eq, Default, Debug)]
462 struct IdentLocation
{
466 impl Add
for IdentLocation
{
467 type Output
= IdentLocation
;
469 fn add(self, other
: Self) -> Self::Output
{
471 index
: self.index
+ other
.index
,
476 impl AddAssign
for IdentLocation
{
477 fn add_assign(&mut self, other
: Self) {
478 *self = *self + other
;
482 #[derive(Clone, Copy, Debug)]
483 enum IdentDifference
{
485 Single(IdentLocation
),
486 Double(IdentLocation
, IdentLocation
),
491 impl Add
for IdentDifference
{
492 type Output
= IdentDifference
;
494 fn add(self, other
: Self) -> Self::Output
{
495 match (self, other
) {
496 (Self::NoDifference
, output
) | (output
, Self::NoDifference
) => output
,
498 | (_
, Self::Multiple
)
499 | (Self::Double(_
, _
), Self::Single(_
))
500 | (Self::Single(_
) | Self::Double(_
, _
), Self::Double(_
, _
)) => Self::Multiple
,
501 (Self::NonIdent
, _
) | (_
, Self::NonIdent
) => Self::NonIdent
,
502 (Self::Single(il1
), Self::Single(il2
)) => Self::Double(il1
, il2
),
507 impl AddAssign
for IdentDifference
{
508 fn add_assign(&mut self, other
: Self) {
509 *self = *self + other
;
513 impl IdentDifference
{
514 /// Returns true if learning about more differences will not change the value
515 /// of this `IdentDifference`, and false otherwise.
516 fn is_complete(&self) -> bool
{
518 Self::NoDifference
| Self::Single(_
) | Self::Double(_
, _
) => false,
519 Self::Multiple
| Self::NonIdent
=> true,
524 fn ident_difference_expr(left
: &Expr
, right
: &Expr
) -> IdentDifference
{
525 ident_difference_expr_with_base_location(left
, right
, IdentLocation
::default()).0
528 fn ident_difference_expr_with_base_location(
531 mut base
: IdentLocation
,
532 ) -> (IdentDifference
, IdentLocation
) {
533 // Ideally, this function should not use IdentIter because it should return
534 // early if the expressions have any non-ident differences. We want that early
535 // return because if without that restriction the lint would lead to false
538 // But, we cannot (easily?) use a `rustc_ast::visit::Visitor`, since we need
539 // the two expressions to be walked in lockstep. And without a `Visitor`, we'd
540 // have to do all the AST traversal ourselves, which is a lot of work, since to
541 // do it properly we'd need to be able to handle more or less every possible
542 // AST node since `Item`s can be written inside `Expr`s.
544 // In practice, it seems likely that expressions, above a certain size, that
545 // happen to use the exact same idents in the exact same order, and which are
546 // not structured the same, would be rare. Therefore it seems likely that if
547 // we do only the first layer of matching ourselves and eventually fallback on
548 // IdentIter, then the output of this function will be almost always be correct
551 // If it turns out that problematic cases are more prevalent than we assume,
552 // then we should be able to change this function to do the correct traversal,
553 // without needing to change the rest of the code.
555 #![allow(clippy::enum_glob_use)]
559 &strip_non_ident_wrappers(left
).kind
,
560 &strip_non_ident_wrappers(right
).kind
,
564 | (Paren(_
), Paren(_
))
565 | (Repeat(_
, _
), Repeat(_
, _
))
566 | (Struct(_
), Struct(_
))
567 | (MacCall(_
), MacCall(_
))
568 | (InlineAsm(_
), InlineAsm(_
))
570 | (Continue(_
), Continue(_
))
571 | (Break(_
, _
), Break(_
, _
))
572 | (AddrOf(_
, _
, _
), AddrOf(_
, _
, _
))
573 | (Path(_
, _
), Path(_
, _
))
574 | (Range(_
, _
, _
), Range(_
, _
, _
))
575 | (Index(_
, _
), Index(_
, _
))
576 | (Field(_
, _
), Field(_
, _
))
577 | (AssignOp(_
, _
, _
), AssignOp(_
, _
, _
))
578 | (Assign(_
, _
, _
), Assign(_
, _
, _
))
579 | (TryBlock(_
), TryBlock(_
))
580 | (Await(_
, _
), Await(_
, _
))
581 | (Async(_
, _
), Async(_
, _
))
582 | (Block(_
, _
), Block(_
, _
))
583 | (Closure(_
), Closure(_
))
584 | (Match(_
, _
), Match(_
, _
))
585 | (Loop(_
, _
, _
), Loop(_
, _
, _
))
586 | (ForLoop(_
, _
, _
, _
), ForLoop(_
, _
, _
, _
))
587 | (While(_
, _
, _
), While(_
, _
, _
))
588 | (If(_
, _
, _
), If(_
, _
, _
))
589 | (Let(_
, _
, _
), Let(_
, _
, _
))
590 | (Type(_
, _
), Type(_
, _
))
591 | (Cast(_
, _
), Cast(_
, _
))
593 | (Unary(_
, _
), Unary(_
, _
))
594 | (Binary(_
, _
, _
), Binary(_
, _
, _
))
596 | (MethodCall(_
), MethodCall(_
))
597 | (Call(_
, _
), Call(_
, _
))
598 | (ConstBlock(_
), ConstBlock(_
))
599 | (Array(_
), Array(_
)) => {
603 return (IdentDifference
::NonIdent
, base
);
607 let mut difference
= IdentDifference
::NoDifference
;
609 for (left_attr
, right_attr
) in left
.attrs
.iter().zip(right
.attrs
.iter()) {
610 let (new_difference
, new_base
) =
611 ident_difference_via_ident_iter_with_base_location(left_attr
, right_attr
, base
);
613 difference
+= new_difference
;
614 if difference
.is_complete() {
615 return (difference
, base
);
619 let (new_difference
, new_base
) = ident_difference_via_ident_iter_with_base_location(left
, right
, base
);
621 difference
+= new_difference
;
626 fn ident_difference_via_ident_iter_with_base_location
<Iterable
: Into
<IdentIter
>>(
629 mut base
: IdentLocation
,
630 ) -> (IdentDifference
, IdentLocation
) {
631 // See the note in `ident_difference_expr_with_base_location` about `IdentIter`
632 let mut difference
= IdentDifference
::NoDifference
;
634 let mut left_iterator
= left
.into();
635 let mut right_iterator
= right
.into();
638 match (left_iterator
.next(), right_iterator
.next()) {
639 (Some(left_ident
), Some(right_ident
)) => {
640 if !eq_id(left_ident
, right_ident
) {
641 difference
+= IdentDifference
::Single(base
);
642 if difference
.is_complete() {
643 return (difference
, base
);
647 (Some(_
), None
) | (None
, Some(_
)) => {
648 return (IdentDifference
::NonIdent
, base
);
651 return (difference
, base
);
654 base
+= IdentLocation { index: 1 }
;
658 fn get_ident(expr
: &Expr
, location
: IdentLocation
) -> Option
<Ident
> {
659 IdentIter
::from(expr
).nth(location
.index
)
662 fn suggestion_with_swapped_ident(
663 cx
: &EarlyContext
<'_
>,
665 location
: IdentLocation
,
667 applicability
: &mut Applicability
,
668 ) -> Option
<String
> {
669 get_ident(expr
, location
).and_then(|current_ident
| {
670 if eq_id(current_ident
, new_ident
) {
671 // We never want to suggest a non-change
677 snippet_with_applicability(cx
, expr
.span
.with_hi(current_ident
.span
.lo()), "..", applicability
),
678 snippet_with_applicability(cx
, expr
.span
.with_lo(current_ident
.span
.hi()), "..", applicability
),
683 fn skip_index
<A
, Iter
>(iter
: Iter
, index
: usize) -> impl Iterator
<Item
= A
>
685 Iter
: Iterator
<Item
= A
>,
688 .filter_map(move |(i
, a
)| if i
== index { None }
else { Some(a) }
)