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 pub SUSPICIOUS_OPERATION_GROUPINGS
,
64 "groupings of binary operations that look suspiciously like typos"
67 declare_lint_pass
!(SuspiciousOperationGroupings
=> [SUSPICIOUS_OPERATION_GROUPINGS
]);
69 impl EarlyLintPass
for SuspiciousOperationGroupings
{
70 fn check_expr(&mut self, cx
: &EarlyContext
<'_
>, expr
: &Expr
) {
71 if expr
.span
.from_expansion() {
75 if let Some(binops
) = extract_related_binops(&expr
.kind
) {
76 check_binops(cx
, &binops
.iter().collect
::<Vec
<_
>>());
78 let mut op_types
= Vec
::with_capacity(binops
.len());
79 // We could use a hashmap, etc. to avoid being O(n*m) here, but
80 // we want the lints to be emitted in a consistent order. Besides,
81 // m, (the number of distinct `BinOpKind`s in `binops`)
82 // will often be small, and does have an upper limit.
83 binops
.iter().map(|b
| b
.op
).for_each(|op
| {
84 if !op_types
.contains(&op
) {
89 for op_type
in op_types
{
90 let ops
: Vec
<_
> = binops
.iter().filter(|b
| b
.op
== op_type
).collect();
92 check_binops(cx
, &ops
);
98 fn check_binops(cx
: &EarlyContext
<'_
>, binops
: &[&BinaryOp
<'_
>]) {
99 let binop_count
= binops
.len();
101 // Single binary operation expressions would likely be false
106 let mut one_ident_difference_count
= 0;
107 let mut no_difference_info
= None
;
108 let mut double_difference_info
= None
;
109 let mut expected_ident_loc
= None
;
111 let mut paired_identifiers
= FxHashSet
::default();
113 for (i
, BinaryOp { left, right, op, .. }
) in binops
.iter().enumerate() {
114 match ident_difference_expr(left
, right
) {
115 IdentDifference
::NoDifference
=> {
116 if is_useless_with_eq_exprs(*op
) {
117 // The `eq_op` lint should catch this in this case.
121 no_difference_info
= Some(i
);
123 IdentDifference
::Single(ident_loc
) => {
124 one_ident_difference_count
+= 1;
125 if let Some(previous_expected
) = expected_ident_loc
{
126 if previous_expected
!= ident_loc
{
127 // This expression doesn't match the form we're
132 expected_ident_loc
= Some(ident_loc
);
135 // If there was only a single difference, all other idents
136 // must have been the same, and thus were paired.
137 for id
in skip_index(IdentIter
::from(*left
), ident_loc
.index
) {
138 paired_identifiers
.insert(id
);
141 IdentDifference
::Double(ident_loc1
, ident_loc2
) => {
142 double_difference_info
= Some((i
, ident_loc1
, ident_loc2
));
144 IdentDifference
::Multiple
| IdentDifference
::NonIdent
=> {
145 // It's too hard to know whether this is a bug or not.
151 let mut applicability
= Applicability
::MachineApplicable
;
153 if let Some(expected_loc
) = expected_ident_loc
{
154 match (no_difference_info
, double_difference_info
) {
155 (Some(i
), None
) => attempt_to_emit_no_difference_lint(cx
, binops
, i
, expected_loc
),
156 (None
, Some((double_difference_index
, ident_loc1
, ident_loc2
))) => {
158 if one_ident_difference_count
== binop_count
- 1;
159 if let Some(binop
) = binops
.get(double_difference_index
);
161 let changed_loc
= if ident_loc1
== expected_loc
{
163 } else if ident_loc2
== expected_loc
{
166 // This expression doesn't match the form we're
171 if let Some(sugg
) = ident_swap_sugg(
193 fn attempt_to_emit_no_difference_lint(
194 cx
: &EarlyContext
<'_
>,
195 binops
: &[&BinaryOp
<'_
>],
197 expected_loc
: IdentLocation
,
199 if let Some(binop
) = binops
.get(i
).copied() {
200 // We need to try and figure out which identifier we should
201 // suggest using instead. Since there could be multiple
202 // replacement candidates in a given expression, and we're
203 // just taking the first one, we may get some bad lint
205 let mut applicability
= Applicability
::MaybeIncorrect
;
207 // We assume that the correct ident is one used elsewhere in
208 // the other binops, in a place that there was a single
209 // difference between idents before.
210 let old_left_ident
= get_ident(binop
.left
, expected_loc
);
211 let old_right_ident
= get_ident(binop
.right
, expected_loc
);
213 for b
in skip_index(binops
.iter(), i
) {
215 if let (Some(old_ident
), Some(new_ident
)) =
216 (old_left_ident
, get_ident(b
.left
, expected_loc
));
217 if old_ident
!= new_ident
;
218 if let Some(sugg
) = suggestion_with_swapped_ident(
229 replace_left_sugg(cx
, binop
, &sugg
, &mut applicability
),
237 if let (Some(old_ident
), Some(new_ident
)) =
238 (old_right_ident
, get_ident(b
.right
, expected_loc
));
239 if old_ident
!= new_ident
;
240 if let Some(sugg
) = suggestion_with_swapped_ident(
251 replace_right_sugg(cx
, binop
, &sugg
, &mut applicability
),
261 fn emit_suggestion(cx
: &EarlyContext
<'_
>, span
: Span
, sugg
: String
, applicability
: Applicability
) {
264 SUSPICIOUS_OPERATION_GROUPINGS
,
266 "this sequence of operators looks suspiciously like a bug",
274 cx
: &EarlyContext
<'_
>,
275 paired_identifiers
: &FxHashSet
<Ident
>,
276 binop
: &BinaryOp
<'_
>,
277 location
: IdentLocation
,
278 applicability
: &mut Applicability
,
279 ) -> Option
<String
> {
280 let left_ident
= get_ident(binop
.left
, location
)?
;
281 let right_ident
= get_ident(binop
.right
, location
)?
;
284 paired_identifiers
.contains(&left_ident
),
285 paired_identifiers
.contains(&right_ident
),
287 (true, true) | (false, false) => {
288 // We don't have a good guess of what ident should be
289 // used instead, in these cases.
290 *applicability
= Applicability
::MaybeIncorrect
;
292 // We arbitraily choose one side to suggest changing,
293 // since we don't have a better guess. If the user
294 // ends up duplicating a clause, the `logic_bug` lint
297 let right_suggestion
= suggestion_with_swapped_ident(cx
, binop
.right
, location
, left_ident
, applicability
)?
;
299 replace_right_sugg(cx
, binop
, &right_suggestion
, applicability
)
302 // We haven't seen a pair involving the left one, so
303 // it's probably what is wanted.
305 let right_suggestion
= suggestion_with_swapped_ident(cx
, binop
.right
, location
, left_ident
, applicability
)?
;
307 replace_right_sugg(cx
, binop
, &right_suggestion
, applicability
)
310 // We haven't seen a pair involving the right one, so
311 // it's probably what is wanted.
312 let left_suggestion
= suggestion_with_swapped_ident(cx
, binop
.left
, location
, right_ident
, applicability
)?
;
314 replace_left_sugg(cx
, binop
, &left_suggestion
, applicability
)
321 fn replace_left_sugg(
322 cx
: &EarlyContext
<'_
>,
323 binop
: &BinaryOp
<'_
>,
324 left_suggestion
: &str,
325 applicability
: &mut Applicability
,
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
,
343 snippet_with_applicability(cx
, binop
.left
.span
, "..", applicability
),
344 binop
.op
.to_string(),
349 #[derive(Clone, Debug)]
350 struct BinaryOp
<'exprs
> {
357 impl BinaryOp
<'exprs
> {
358 fn new(op
: BinOpKind
, span
: Span
, (left
, right
): (&'exprs Expr
, &'exprs Expr
)) -> Self {
359 Self { op, span, left, right }
363 fn strip_non_ident_wrappers(expr
: &Expr
) -> &Expr
{
364 let mut output
= expr
;
366 output
= match &output
.kind
{
367 ExprKind
::Paren(ref inner
) | ExprKind
::Unary(_
, ref inner
) => inner
,
375 fn extract_related_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
376 append_opt_vecs(chained_binops(kind
), if_statment_binops(kind
))
379 fn if_statment_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
381 ExprKind
::If(ref condition
, _
, _
) => chained_binops(&condition
.kind
),
382 ExprKind
::Paren(ref e
) => if_statment_binops(&e
.kind
),
383 ExprKind
::Block(ref block
, _
) => {
384 let mut output
= None
;
385 for stmt
in &block
.stmts
{
387 StmtKind
::Expr(ref e
) | StmtKind
::Semi(ref e
) => {
388 output
= append_opt_vecs(output
, if_statment_binops(&e
.kind
));
399 fn append_opt_vecs
<A
>(target_opt
: Option
<Vec
<A
>>, source_opt
: Option
<Vec
<A
>>) -> Option
<Vec
<A
>> {
400 match (target_opt
, source_opt
) {
401 (Some(mut target
), Some(mut source
)) => {
402 target
.reserve(source
.len());
403 for op
in source
.drain(..) {
408 (Some(v
), None
) | (None
, Some(v
)) => Some(v
),
409 (None
, None
) => None
,
413 fn chained_binops(kind
: &ExprKind
) -> Option
<Vec
<BinaryOp
<'_
>>> {
415 ExprKind
::Binary(_
, left_outer
, right_outer
) => chained_binops_helper(left_outer
, right_outer
),
416 ExprKind
::Paren(ref e
) | ExprKind
::Unary(_
, ref e
) => chained_binops(&e
.kind
),
421 fn chained_binops_helper(left_outer
: &'expr Expr
, right_outer
: &'expr Expr
) -> Option
<Vec
<BinaryOp
<'expr
>>> {
422 match (&left_outer
.kind
, &right_outer
.kind
) {
424 ExprKind
::Paren(ref left_e
) | ExprKind
::Unary(_
, ref left_e
),
425 ExprKind
::Paren(ref right_e
) | ExprKind
::Unary(_
, ref right_e
),
426 ) => chained_binops_helper(left_e
, right_e
),
427 (ExprKind
::Paren(ref left_e
) | ExprKind
::Unary(_
, ref left_e
), _
) => chained_binops_helper(left_e
, right_outer
),
428 (_
, ExprKind
::Paren(ref right_e
) | ExprKind
::Unary(_
, ref right_e
)) => {
429 chained_binops_helper(left_outer
, right_e
)
432 ExprKind
::Binary(Spanned { node: left_op, .. }
, ref left_left
, ref left_right
),
433 ExprKind
::Binary(Spanned { node: right_op, .. }
, ref right_left
, ref right_right
),
435 chained_binops_helper(left_left
, left_right
),
436 chained_binops_helper(right_left
, right_right
),
438 (Some(mut left_ops
), Some(mut right_ops
)) => {
439 left_ops
.reserve(right_ops
.len());
440 for op
in right_ops
.drain(..) {
445 (Some(mut left_ops
), _
) => {
446 left_ops
.push(BinaryOp
::new(*right_op
, right_outer
.span
, (right_left
, right_right
)));
449 (_
, Some(mut right_ops
)) => {
450 right_ops
.insert(0, BinaryOp
::new(*left_op
, left_outer
.span
, (left_left
, left_right
)));
453 (None
, None
) => Some(vec
![
454 BinaryOp
::new(*left_op
, left_outer
.span
, (left_left
, left_right
)),
455 BinaryOp
::new(*right_op
, right_outer
.span
, (right_left
, right_right
)),
462 #[derive(Clone, Copy, PartialEq, Eq, Default, Debug)]
463 struct IdentLocation
{
467 impl Add
for IdentLocation
{
468 type Output
= IdentLocation
;
470 fn add(self, other
: Self) -> Self::Output
{
472 index
: self.index
+ other
.index
,
477 impl AddAssign
for IdentLocation
{
478 fn add_assign(&mut self, other
: Self) {
479 *self = *self + other
;
483 #[derive(Clone, Copy, Debug)]
484 enum IdentDifference
{
486 Single(IdentLocation
),
487 Double(IdentLocation
, IdentLocation
),
492 impl Add
for IdentDifference
{
493 type Output
= IdentDifference
;
495 fn add(self, other
: Self) -> Self::Output
{
496 match (self, other
) {
497 (Self::NoDifference
, output
) | (output
, Self::NoDifference
) => output
,
499 | (_
, Self::Multiple
)
500 | (Self::Double(_
, _
), Self::Single(_
))
501 | (Self::Single(_
) | Self::Double(_
, _
), Self::Double(_
, _
)) => Self::Multiple
,
502 (Self::NonIdent
, _
) | (_
, Self::NonIdent
) => Self::NonIdent
,
503 (Self::Single(il1
), Self::Single(il2
)) => Self::Double(il1
, il2
),
508 impl AddAssign
for IdentDifference
{
509 fn add_assign(&mut self, other
: Self) {
510 *self = *self + other
;
514 impl IdentDifference
{
515 /// Returns true if learning about more differences will not change the value
516 /// of this `IdentDifference`, and false otherwise.
517 fn is_complete(&self) -> bool
{
519 Self::NoDifference
| Self::Single(_
) | Self::Double(_
, _
) => false,
520 Self::Multiple
| Self::NonIdent
=> true,
525 fn ident_difference_expr(left
: &Expr
, right
: &Expr
) -> IdentDifference
{
526 ident_difference_expr_with_base_location(left
, right
, IdentLocation
::default()).0
529 fn ident_difference_expr_with_base_location(
532 mut base
: IdentLocation
,
533 ) -> (IdentDifference
, IdentLocation
) {
534 // Ideally, this function should not use IdentIter because it should return
535 // early if the expressions have any non-ident differences. We want that early
536 // return because if without that restriction the lint would lead to false
539 // But, we cannot (easily?) use a `rustc_ast::visit::Visitor`, since we need
540 // the two expressions to be walked in lockstep. And without a `Visitor`, we'd
541 // have to do all the AST traversal ourselves, which is a lot of work, since to
542 // do it properly we'd need to be able to handle more or less every possible
543 // AST node since `Item`s can be written inside `Expr`s.
545 // In practice, it seems likely that expressions, above a certain size, that
546 // happen to use the exact same idents in the exact same order, and which are
547 // not structured the same, would be rare. Therefore it seems likely that if
548 // we do only the first layer of matching ourselves and eventually fallback on
549 // IdentIter, then the output of this function will be almost always be correct
552 // If it turns out that problematic cases are more prelavent than we assume,
553 // then we should be able to change this function to do the correct traversal,
554 // without needing to change the rest of the code.
556 #![allow(clippy::enum_glob_use)]
560 &strip_non_ident_wrappers(left
).kind
,
561 &strip_non_ident_wrappers(right
).kind
,
565 | (Paren(_
), Paren(_
))
566 | (Repeat(_
, _
), Repeat(_
, _
))
567 | (Struct(_
), Struct(_
))
568 | (MacCall(_
), MacCall(_
))
569 | (LlvmInlineAsm(_
), LlvmInlineAsm(_
))
570 | (InlineAsm(_
), InlineAsm(_
))
572 | (Continue(_
), Continue(_
))
573 | (Break(_
, _
), Break(_
, _
))
574 | (AddrOf(_
, _
, _
), AddrOf(_
, _
, _
))
575 | (Path(_
, _
), Path(_
, _
))
576 | (Range(_
, _
, _
), Range(_
, _
, _
))
577 | (Index(_
, _
), Index(_
, _
))
578 | (Field(_
, _
), Field(_
, _
))
579 | (AssignOp(_
, _
, _
), AssignOp(_
, _
, _
))
580 | (Assign(_
, _
, _
), Assign(_
, _
, _
))
581 | (TryBlock(_
), TryBlock(_
))
582 | (Await(_
), Await(_
))
583 | (Async(_
, _
, _
), Async(_
, _
, _
))
584 | (Block(_
, _
), Block(_
, _
))
585 | (Closure(_
, _
, _
, _
, _
, _
), Closure(_
, _
, _
, _
, _
, _
))
586 | (Match(_
, _
), Match(_
, _
))
587 | (Loop(_
, _
), Loop(_
, _
))
588 | (ForLoop(_
, _
, _
, _
), ForLoop(_
, _
, _
, _
))
589 | (While(_
, _
, _
), While(_
, _
, _
))
590 | (If(_
, _
, _
), If(_
, _
, _
))
591 | (Let(_
, _
, _
), Let(_
, _
, _
))
592 | (Type(_
, _
), Type(_
, _
))
593 | (Cast(_
, _
), Cast(_
, _
))
595 | (Unary(_
, _
), Unary(_
, _
))
596 | (Binary(_
, _
, _
), Binary(_
, _
, _
))
598 | (MethodCall(_
, _
, _
), MethodCall(_
, _
, _
))
599 | (Call(_
, _
), Call(_
, _
))
600 | (ConstBlock(_
), ConstBlock(_
))
601 | (Array(_
), Array(_
))
602 | (Box(_
), Box(_
)) => {
606 return (IdentDifference
::NonIdent
, base
);
610 let mut difference
= IdentDifference
::NoDifference
;
612 for (left_attr
, right_attr
) in left
.attrs
.iter().zip(right
.attrs
.iter()) {
613 let (new_difference
, new_base
) =
614 ident_difference_via_ident_iter_with_base_location(left_attr
, right_attr
, base
);
616 difference
+= new_difference
;
617 if difference
.is_complete() {
618 return (difference
, base
);
622 let (new_difference
, new_base
) = ident_difference_via_ident_iter_with_base_location(left
, right
, base
);
624 difference
+= new_difference
;
629 fn ident_difference_via_ident_iter_with_base_location
<Iterable
: Into
<IdentIter
>>(
632 mut base
: IdentLocation
,
633 ) -> (IdentDifference
, IdentLocation
) {
634 // See the note in `ident_difference_expr_with_base_location` about `IdentIter`
635 let mut difference
= IdentDifference
::NoDifference
;
637 let mut left_iterator
= left
.into();
638 let mut right_iterator
= right
.into();
641 match (left_iterator
.next(), right_iterator
.next()) {
642 (Some(left_ident
), Some(right_ident
)) => {
643 if !eq_id(left_ident
, right_ident
) {
644 difference
+= IdentDifference
::Single(base
);
645 if difference
.is_complete() {
646 return (difference
, base
);
650 (Some(_
), None
) | (None
, Some(_
)) => {
651 return (IdentDifference
::NonIdent
, base
);
654 return (difference
, base
);
657 base
+= IdentLocation { index: 1 }
;
661 fn get_ident(expr
: &Expr
, location
: IdentLocation
) -> Option
<Ident
> {
662 IdentIter
::from(expr
).nth(location
.index
)
665 fn suggestion_with_swapped_ident(
666 cx
: &EarlyContext
<'_
>,
668 location
: IdentLocation
,
670 applicability
: &mut Applicability
,
671 ) -> Option
<String
> {
672 get_ident(expr
, location
).and_then(|current_ident
| {
673 if eq_id(current_ident
, new_ident
) {
674 // We never want to suggest a non-change
680 snippet_with_applicability(cx
, expr
.span
.with_hi(current_ident
.span
.lo()), "..", applicability
),
682 snippet_with_applicability(cx
, expr
.span
.with_lo(current_ident
.span
.hi()), "..", applicability
),
687 fn skip_index
<A
, Iter
>(iter
: Iter
, index
: usize) -> impl Iterator
<Item
= A
>
689 Iter
: Iterator
<Item
= A
>,
692 .filter_map(move |(i
, a
)| if i
== index { None }
else { Some(a) }
)