]> git.proxmox.com Git - rustc.git/blob - src/tools/clippy/clippy_lints/src/suspicious_operation_groupings.rs
New upstream version 1.67.1+dfsg1
[rustc.git] / src / tools / clippy / clippy_lints / src / suspicious_operation_groupings.rs
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;
13 use rustc_span::Span;
14
15 declare_clippy_lint! {
16 /// ### What it does
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.
20 ///
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.
25 ///
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.
29 ///
30 /// ### Example
31 /// ```rust
32 /// struct Vec3 {
33 /// x: f64,
34 /// y: f64,
35 /// z: f64,
36 /// }
37 ///
38 /// impl Eq for Vec3 {}
39 ///
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
44 /// }
45 /// }
46 /// ```
47 /// Use instead:
48 /// ```rust
49 /// # struct Vec3 {
50 /// # x: f64,
51 /// # y: f64,
52 /// # z: f64,
53 /// # }
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
59 /// }
60 /// }
61 /// ```
62 #[clippy::version = "1.50.0"]
63 pub SUSPICIOUS_OPERATION_GROUPINGS,
64 nursery,
65 "groupings of binary operations that look suspiciously like typos"
66 }
67
68 declare_lint_pass!(SuspiciousOperationGroupings => [SUSPICIOUS_OPERATION_GROUPINGS]);
69
70 impl EarlyLintPass for SuspiciousOperationGroupings {
71 fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
72 if expr.span.from_expansion() {
73 return;
74 }
75
76 if let Some(binops) = extract_related_binops(&expr.kind) {
77 check_binops(cx, &binops.iter().collect::<Vec<_>>());
78
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) {
86 op_types.push(op);
87 }
88 });
89
90 for op_type in op_types {
91 let ops: Vec<_> = binops.iter().filter(|b| b.op == op_type).collect();
92
93 check_binops(cx, &ops);
94 }
95 }
96 }
97 }
98
99 fn check_binops(cx: &EarlyContext<'_>, binops: &[&BinaryOp<'_>]) {
100 let binop_count = binops.len();
101 if binop_count < 2 {
102 // Single binary operation expressions would likely be false
103 // positives.
104 return;
105 }
106
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;
111
112 let mut paired_identifiers = FxHashSet::default();
113
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.
119 return;
120 }
121
122 no_difference_info = Some(i);
123 },
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
129 // looking for.
130 return;
131 }
132 } else {
133 expected_ident_loc = Some(ident_loc);
134 }
135
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);
140 }
141 },
142 IdentDifference::Double(ident_loc1, ident_loc2) => {
143 double_difference_info = Some((i, ident_loc1, ident_loc2));
144 },
145 IdentDifference::Multiple | IdentDifference::NonIdent => {
146 // It's too hard to know whether this is a bug or not.
147 return;
148 },
149 }
150 }
151
152 let mut applicability = Applicability::MachineApplicable;
153
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))) => {
158 if_chain! {
159 if one_ident_difference_count == binop_count - 1;
160 if let Some(binop) = binops.get(double_difference_index);
161 then {
162 let changed_loc = if ident_loc1 == expected_loc {
163 ident_loc2
164 } else if ident_loc2 == expected_loc {
165 ident_loc1
166 } else {
167 // This expression doesn't match the form we're
168 // looking for.
169 return;
170 };
171
172 if let Some(sugg) = ident_swap_sugg(
173 cx,
174 &paired_identifiers,
175 binop,
176 changed_loc,
177 &mut applicability,
178 ) {
179 emit_suggestion(
180 cx,
181 binop.span,
182 sugg,
183 applicability,
184 );
185 }
186 }
187 }
188 },
189 _ => {},
190 }
191 }
192 }
193
194 fn attempt_to_emit_no_difference_lint(
195 cx: &EarlyContext<'_>,
196 binops: &[&BinaryOp<'_>],
197 i: usize,
198 expected_loc: IdentLocation,
199 ) {
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
205 // messages.
206 let mut applicability = Applicability::MaybeIncorrect;
207
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);
213
214 for b in skip_index(binops.iter(), i) {
215 if_chain! {
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(
220 cx,
221 binop.left,
222 expected_loc,
223 new_ident,
224 &mut applicability,
225 );
226 then {
227 emit_suggestion(
228 cx,
229 binop.span,
230 replace_left_sugg(cx, binop, &sugg, &mut applicability),
231 applicability,
232 );
233 return;
234 }
235 }
236
237 if_chain! {
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(
242 cx,
243 binop.right,
244 expected_loc,
245 new_ident,
246 &mut applicability,
247 );
248 then {
249 emit_suggestion(
250 cx,
251 binop.span,
252 replace_right_sugg(cx, binop, &sugg, &mut applicability),
253 applicability,
254 );
255 return;
256 }
257 }
258 }
259 }
260 }
261
262 fn emit_suggestion(cx: &EarlyContext<'_>, span: Span, sugg: String, applicability: Applicability) {
263 span_lint_and_sugg(
264 cx,
265 SUSPICIOUS_OPERATION_GROUPINGS,
266 span,
267 "this sequence of operators looks suspiciously like a bug",
268 "did you mean",
269 sugg,
270 applicability,
271 );
272 }
273
274 fn ident_swap_sugg(
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)?;
283
284 let sugg = match (
285 paired_identifiers.contains(&left_ident),
286 paired_identifiers.contains(&right_ident),
287 ) {
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;
292
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
296 // should catch it.
297
298 let right_suggestion = suggestion_with_swapped_ident(cx, binop.right, location, left_ident, applicability)?;
299
300 replace_right_sugg(cx, binop, &right_suggestion, applicability)
301 },
302 (false, true) => {
303 // We haven't seen a pair involving the left one, so
304 // it's probably what is wanted.
305
306 let right_suggestion = suggestion_with_swapped_ident(cx, binop.right, location, left_ident, applicability)?;
307
308 replace_right_sugg(cx, binop, &right_suggestion, applicability)
309 },
310 (true, false) => {
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)?;
314
315 replace_left_sugg(cx, binop, &left_suggestion, applicability)
316 },
317 };
318
319 Some(sugg)
320 }
321
322 fn replace_left_sugg(
323 cx: &EarlyContext<'_>,
324 binop: &BinaryOp<'_>,
325 left_suggestion: &str,
326 applicability: &mut Applicability,
327 ) -> String {
328 format!(
329 "{left_suggestion} {} {}",
330 binop.op.to_string(),
331 snippet_with_applicability(cx, binop.right.span, "..", applicability),
332 )
333 }
334
335 fn replace_right_sugg(
336 cx: &EarlyContext<'_>,
337 binop: &BinaryOp<'_>,
338 right_suggestion: &str,
339 applicability: &mut Applicability,
340 ) -> String {
341 format!(
342 "{} {} {right_suggestion}",
343 snippet_with_applicability(cx, binop.left.span, "..", applicability),
344 binop.op.to_string(),
345 )
346 }
347
348 #[derive(Clone, Debug)]
349 struct BinaryOp<'exprs> {
350 op: BinOpKind,
351 span: Span,
352 left: &'exprs Expr,
353 right: &'exprs Expr,
354 }
355
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 }
359 }
360 }
361
362 fn strip_non_ident_wrappers(expr: &Expr) -> &Expr {
363 let mut output = expr;
364 loop {
365 output = match &output.kind {
366 ExprKind::Paren(ref inner) | ExprKind::Unary(_, ref inner) => inner,
367 _ => {
368 return output;
369 },
370 };
371 }
372 }
373
374 fn extract_related_binops(kind: &ExprKind) -> Option<Vec<BinaryOp<'_>>> {
375 append_opt_vecs(chained_binops(kind), if_statement_binops(kind))
376 }
377
378 fn if_statement_binops(kind: &ExprKind) -> Option<Vec<BinaryOp<'_>>> {
379 match kind {
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 {
385 match stmt.kind {
386 StmtKind::Expr(ref e) | StmtKind::Semi(ref e) => {
387 output = append_opt_vecs(output, if_statement_binops(&e.kind));
388 },
389 _ => {},
390 }
391 }
392 output
393 },
394 _ => None,
395 }
396 }
397
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());
402 for op in source {
403 target.push(op);
404 }
405 Some(target)
406 },
407 (Some(v), None) | (None, Some(v)) => Some(v),
408 (None, None) => None,
409 }
410 }
411
412 fn chained_binops(kind: &ExprKind) -> Option<Vec<BinaryOp<'_>>> {
413 match kind {
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),
416 _ => None,
417 }
418 }
419
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) {
422 (
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)
429 },
430 (
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),
433 ) => match (
434 chained_binops_helper(left_left, left_right),
435 chained_binops_helper(right_left, right_right),
436 ) {
437 (Some(mut left_ops), Some(right_ops)) => {
438 left_ops.reserve(right_ops.len());
439 for op in right_ops {
440 left_ops.push(op);
441 }
442 Some(left_ops)
443 },
444 (Some(mut left_ops), _) => {
445 left_ops.push(BinaryOp::new(*right_op, right_outer.span, (right_left, right_right)));
446 Some(left_ops)
447 },
448 (_, Some(mut right_ops)) => {
449 right_ops.insert(0, BinaryOp::new(*left_op, left_outer.span, (left_left, left_right)));
450 Some(right_ops)
451 },
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)),
455 ]),
456 },
457 _ => None,
458 }
459 }
460
461 #[derive(Clone, Copy, PartialEq, Eq, Default, Debug)]
462 struct IdentLocation {
463 index: usize,
464 }
465
466 impl Add for IdentLocation {
467 type Output = IdentLocation;
468
469 fn add(self, other: Self) -> Self::Output {
470 Self {
471 index: self.index + other.index,
472 }
473 }
474 }
475
476 impl AddAssign for IdentLocation {
477 fn add_assign(&mut self, other: Self) {
478 *self = *self + other;
479 }
480 }
481
482 #[derive(Clone, Copy, Debug)]
483 enum IdentDifference {
484 NoDifference,
485 Single(IdentLocation),
486 Double(IdentLocation, IdentLocation),
487 Multiple,
488 NonIdent,
489 }
490
491 impl Add for IdentDifference {
492 type Output = IdentDifference;
493
494 fn add(self, other: Self) -> Self::Output {
495 match (self, other) {
496 (Self::NoDifference, output) | (output, Self::NoDifference) => output,
497 (Self::Multiple, _)
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),
503 }
504 }
505 }
506
507 impl AddAssign for IdentDifference {
508 fn add_assign(&mut self, other: Self) {
509 *self = *self + other;
510 }
511 }
512
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 {
517 match self {
518 Self::NoDifference | Self::Single(_) | Self::Double(_, _) => false,
519 Self::Multiple | Self::NonIdent => true,
520 }
521 }
522 }
523
524 fn ident_difference_expr(left: &Expr, right: &Expr) -> IdentDifference {
525 ident_difference_expr_with_base_location(left, right, IdentLocation::default()).0
526 }
527
528 fn ident_difference_expr_with_base_location(
529 left: &Expr,
530 right: &Expr,
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
536 // positives.
537 //
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.
543 //
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
549 // in practice.
550 //
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.
554
555 #![allow(clippy::enum_glob_use)]
556 use ExprKind::*;
557
558 match (
559 &strip_non_ident_wrappers(left).kind,
560 &strip_non_ident_wrappers(right).kind,
561 ) {
562 (Yield(_), Yield(_))
563 | (Try(_), Try(_))
564 | (Paren(_), Paren(_))
565 | (Repeat(_, _), Repeat(_, _))
566 | (Struct(_), Struct(_))
567 | (MacCall(_), MacCall(_))
568 | (InlineAsm(_), InlineAsm(_))
569 | (Ret(_), Ret(_))
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(_, _))
592 | (Lit(_), Lit(_))
593 | (Unary(_, _), Unary(_, _))
594 | (Binary(_, _, _), Binary(_, _, _))
595 | (Tup(_), Tup(_))
596 | (MethodCall(_), MethodCall(_))
597 | (Call(_, _), Call(_, _))
598 | (ConstBlock(_), ConstBlock(_))
599 | (Array(_), Array(_))
600 | (Box(_), Box(_)) => {
601 // keep going
602 },
603 _ => {
604 return (IdentDifference::NonIdent, base);
605 },
606 }
607
608 let mut difference = IdentDifference::NoDifference;
609
610 for (left_attr, right_attr) in left.attrs.iter().zip(right.attrs.iter()) {
611 let (new_difference, new_base) =
612 ident_difference_via_ident_iter_with_base_location(left_attr, right_attr, base);
613 base = new_base;
614 difference += new_difference;
615 if difference.is_complete() {
616 return (difference, base);
617 }
618 }
619
620 let (new_difference, new_base) = ident_difference_via_ident_iter_with_base_location(left, right, base);
621 base = new_base;
622 difference += new_difference;
623
624 (difference, base)
625 }
626
627 fn ident_difference_via_ident_iter_with_base_location<Iterable: Into<IdentIter>>(
628 left: Iterable,
629 right: Iterable,
630 mut base: IdentLocation,
631 ) -> (IdentDifference, IdentLocation) {
632 // See the note in `ident_difference_expr_with_base_location` about `IdentIter`
633 let mut difference = IdentDifference::NoDifference;
634
635 let mut left_iterator = left.into();
636 let mut right_iterator = right.into();
637
638 loop {
639 match (left_iterator.next(), right_iterator.next()) {
640 (Some(left_ident), Some(right_ident)) => {
641 if !eq_id(left_ident, right_ident) {
642 difference += IdentDifference::Single(base);
643 if difference.is_complete() {
644 return (difference, base);
645 }
646 }
647 },
648 (Some(_), None) | (None, Some(_)) => {
649 return (IdentDifference::NonIdent, base);
650 },
651 (None, None) => {
652 return (difference, base);
653 },
654 }
655 base += IdentLocation { index: 1 };
656 }
657 }
658
659 fn get_ident(expr: &Expr, location: IdentLocation) -> Option<Ident> {
660 IdentIter::from(expr).nth(location.index)
661 }
662
663 fn suggestion_with_swapped_ident(
664 cx: &EarlyContext<'_>,
665 expr: &Expr,
666 location: IdentLocation,
667 new_ident: Ident,
668 applicability: &mut Applicability,
669 ) -> Option<String> {
670 get_ident(expr, location).and_then(|current_ident| {
671 if eq_id(current_ident, new_ident) {
672 // We never want to suggest a non-change
673 return None;
674 }
675
676 Some(format!(
677 "{}{new_ident}{}",
678 snippet_with_applicability(cx, expr.span.with_hi(current_ident.span.lo()), "..", applicability),
679 snippet_with_applicability(cx, expr.span.with_lo(current_ident.span.hi()), "..", applicability),
680 ))
681 })
682 }
683
684 fn skip_index<A, Iter>(iter: Iter, index: usize) -> impl Iterator<Item = A>
685 where
686 Iter: Iterator<Item = A>,
687 {
688 iter.enumerate()
689 .filter_map(move |(i, a)| if i == index { None } else { Some(a) })
690 }