about summary refs log tree commit diff
diff options
context:
space:
mode:
authorteor <teor@riseup.net>2024-01-17 12:22:23 +1000
committerteor <teor@riseup.net>2024-02-20 07:51:21 +1000
commit1cbb58bd061a72f07f087c7f0befad6b0b214538 (patch)
treeab80a2cadecb462228e06f5acb10e3444d908911
parentd109e681782d71c6426b155d225b816893538a9c (diff)
downloadrust-1cbb58bd061a72f07f087c7f0befad6b0b214538.tar.gz
rust-1cbb58bd061a72f07f087c7f0befad6b0b214538.zip
Fix divmul peeling
-rw-r--r--clippy_lints/src/casts/cast_sign_loss.rs15
1 files changed, 11 insertions, 4 deletions
diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs
index 1100625bc4a..9c18989701e 100644
--- a/clippy_lints/src/casts/cast_sign_loss.rs
+++ b/clippy_lints/src/casts/cast_sign_loss.rs
@@ -229,23 +229,30 @@ fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> {
                 // For binary operators which both contribute to the sign of the result,
                 // collect all their operands, recursively. This ignores overflow.
                 ControlFlow::Continue(Descend::Yes)
-            } else if matches!(op.node, BinOpKind::Rem) {
+            } else if matches!(op.node, BinOpKind::Rem | BinOpKind::Shr) {
                 // For binary operators where the left hand side determines the sign of the result,
                 // only collect that side, recursively. Overflow panics, so this always holds.
                 //
+                // Large left shifts turn negatives into zeroes, so we can't use it here.
+                //
                 // > Given remainder = dividend % divisor, the remainder will have the same sign as the dividend
+                // > ...
+                // > Arithmetic right shift on signed integer types
                 // https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators
+
+                // We want to descend into the lhs, but skip the rhs.
+                // That's tricky to do using for_each_expr(), so we just keep the lhs intact.
                 res.push(lhs);
-                ControlFlow::Break(())
+                ControlFlow::Continue(Descend::No)
             } else {
                 // The sign of the result of other binary operators depends on the values of the operands,
                 // so try to evaluate the expression.
-                res.push(expr);
+                res.push(sub_expr);
                 ControlFlow::Continue(Descend::No)
             }
         } else {
             // For other expressions, including unary operators and constants, try to evaluate the expression.
-            res.push(expr);
+            res.push(sub_expr);
             ControlFlow::Continue(Descend::No)
         }
     });