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