Skip to content

Commit 23ee332

Browse files
y21y21
committed
lint when relevant sides of binops are constants
1 parent c9d2961 commit 23ee332

File tree

4 files changed

+180
-51
lines changed

4 files changed

+180
-51
lines changed

‎clippy_utils/src/eager_or_lazy.rs‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
//! - or-fun-call
1010
//! - option-if-let-else
1111
12-
use crate::consts::{constant, FullInt};
12+
use crate::consts::constant;
1313
use crate::ty::{all_predicates_of, is_copy};
1414
use crate::visitors::is_const_evaluatable;
1515
use rustc_hir::def::{DefKind, Res};
@@ -195,8 +195,8 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS
195195
}
196196
},
197197

198-
// `-i32::MIN` panics with overflow checks, but `constant` lets us rule out some simple cases
199-
ExprKind::Unary(UnOp::Neg, _) if constant(self.cx, self.cx.typeck_results(), e).is_none() => {
198+
// `-i32::MIN` panics with overflow checks
199+
ExprKind::Unary(UnOp::Neg, right) if constant(self.cx, self.cx.typeck_results(), right).is_none() => {
200200
self.eagerness |= NoChange;
201201
},
202202

@@ -216,31 +216,28 @@ fn expr_eagerness<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> EagernessS
216216
) => {},
217217

218218
// `>>` and `<<` panic when the right-hand side is greater than or equal to the number of bits in the
219-
// type of the left-hand side, or is negative. Doesn't need `constant` on the whole expression
220-
// because only the right side matters.
221-
ExprKind::Binary(op, left, right)
219+
// type of the left-hand side, or is negative.
220+
// We intentionally only check if the right-hand isn't a constant, because even if the suggestion would
221+
// overflow with constants, the compiler emits an error for it and the programmer will have to fix it.
222+
// Thus, we would realistically only delay the lint.
223+
ExprKind::Binary(op, _, right)
222224
if matches!(op.node, BinOpKind::Shl | BinOpKind::Shr)
223-
&& let left_ty = self.cx.typeck_results().expr_ty(left)
224-
&& let left_bits = left_ty.int_size_and_signed(self.cx.tcx).0.bits()
225-
&& constant(self.cx, self.cx.typeck_results(), right)
226-
.and_then(|c| c.int_value(self.cx, left_ty))
227-
.map_or(true, |c| match c {
228-
FullInt::S(i) => i >= i128::from(left_bits) || i.is_negative(),
229-
FullInt::U(i) => i >= u128::from(left_bits),
230-
}) =>
225+
&& constant(self.cx, self.cx.typeck_results(), right).is_none() =>
231226
{
232227
self.eagerness |= NoChange;
233228
},
234229

235-
// Arithmetic operations panic on under-/overflow with overflow checks, so don't suggest changing it:
236-
// https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow
237-
// Using `constant` lets us allow some simple cases where we know for sure it can't overflow
238-
ExprKind::Binary(op, ..)
230+
// Similar to `>>` and `<<`, we only want to avoid linting entirely if both sides are unknown and the
231+
// compiler can't emit an error for an overflowing expression.
232+
// Suggesting eagerness for `true.then(|| i32::MAX + 1)` is okay because the compiler will emit an
233+
// error and it's good to have the eagerness warning up front when the user fixes the logic error.
234+
ExprKind::Binary(op, left, right)
239235
if matches!(
240236
op.node,
241-
BinOpKind::Add | BinOpKind::Mul | BinOpKind::Sub | BinOpKind::Div | BinOpKind::Rem
237+
BinOpKind::Add | BinOpKind::Sub | BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem
242238
) && !self.cx.typeck_results().expr_ty(e).is_floating_point()
243-
&& constant(self.cx, self.cx.typeck_results(), e).is_none() =>
239+
&& (constant(self.cx, self.cx.typeck_results(), left).is_none()
240+
|| constant(self.cx, self.cx.typeck_results(), right).is_none()) =>
244241
{
245242
self.eagerness |= NoChange;
246243
},

‎tests/ui/unnecessary_lazy_eval.fixed‎

Lines changed: 39 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,6 @@ fn main() {
185185
// neither bind_instead_of_map nor unnecessary_lazy_eval applies here
186186
let _: Result<usize, usize> = res.and_then(|x| Err(x));
187187
let _: Result<usize, usize> = res.or_else(|err| Ok(err));
188-
189-
issue9422(3);
190-
panicky_arithmetic_ops(3);
191188
}
192189

193190
#[allow(unused)]
@@ -201,33 +198,58 @@ fn issue9422(x: usize) -> Option<usize> {
201198
// (x >= 5).then_some(x - 5) // clippy suggestion panics
202199
}
203200

204-
// https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow
205-
fn panicky_arithmetic_ops(x: usize) {
206-
let _x = false.then(|| i32::MAX + 1);
207-
let _x = false.then(|| i32::MAX * 2);
201+
fn panicky_arithmetic_ops(x: usize, y: isize) {
202+
#![allow(clippy::identity_op, clippy::eq_op)]
203+
204+
// Even though some of these expressions overflow, they're entirely dependent on constants.
205+
// So, the compiler already emits a warning about overflowing expressions.
206+
// It's a logic error and we want both warnings up front.
207+
// ONLY when a binop side that "matters" for overflow (for `>>`, that is always the right side and
208+
// never the left side) has a non-constant value, avoid linting
209+
210+
let _x = false.then_some(i32::MAX + 1);
211+
//~^ ERROR: unnecessary closure used with `bool::then`
212+
let _x = false.then_some(i32::MAX * 2);
213+
//~^ ERROR: unnecessary closure used with `bool::then`
208214
let _x = false.then_some(i32::MAX - 1);
209215
//~^ ERROR: unnecessary closure used with `bool::then`
210-
let _x = false.then(|| i32::MIN - 1);
211-
#[allow(clippy::identity_op)]
216+
let _x = false.then_some(i32::MIN - 1);
217+
//~^ ERROR: unnecessary closure used with `bool::then`
212218
let _x = false.then_some((1 + 2 * 3 - 2 / 3 + 9) << 2);
213219
//~^ ERROR: unnecessary closure used with `bool::then`
214220
let _x = false.then_some(255u8 << 7);
215221
//~^ ERROR: unnecessary closure used with `bool::then`
216-
let _x = false.then(|| 255u8 << 8);
217-
let _x = false.then(|| 255u8 >> 8);
222+
let _x = false.then_some(255u8 << 8);
223+
//~^ ERROR: unnecessary closure used with `bool::then`
224+
let _x = false.then_some(255u8 >> 8);
225+
//~^ ERROR: unnecessary closure used with `bool::then`
218226
let _x = false.then(|| 255u8 >> x);
219-
let _x = false.then(|| i32::MIN / -1);
227+
let _x = false.then_some(i32::MIN / -1);
228+
//~^ ERROR: unnecessary closure used with `bool::then`
220229
let _x = false.then_some(i32::MAX + -1);
221230
//~^ ERROR: unnecessary closure used with `bool::then`
222231
let _x = false.then_some(-i32::MAX);
223232
//~^ ERROR: unnecessary closure used with `bool::then`
224-
let _x = false.then(|| -i32::MIN);
225-
let _x = false.then(|| 255 >> -7);
226-
let _x = false.then(|| 255 << -1);
227-
let _x = false.then(|| 1 / 0);
228-
let _x = false.then(|| x << -1);
233+
let _x = false.then_some(-i32::MIN);
234+
//~^ ERROR: unnecessary closure used with `bool::then`
235+
let _x = false.then(|| -y);
236+
let _x = false.then_some(255 >> -7);
237+
//~^ ERROR: unnecessary closure used with `bool::then`
238+
let _x = false.then_some(255 << -1);
239+
//~^ ERROR: unnecessary closure used with `bool::then`
240+
let _x = false.then_some(1 / 0);
241+
//~^ ERROR: unnecessary closure used with `bool::then`
242+
let _x = false.then_some(x << -1);
243+
//~^ ERROR: unnecessary closure used with `bool::then`
229244
let _x = false.then_some(x << 2);
230245
//~^ ERROR: unnecessary closure used with `bool::then`
246+
let _x = false.then(|| x + x);
247+
let _x = false.then(|| x * x);
248+
let _x = false.then(|| x - x);
249+
let _x = false.then(|| x / x);
250+
let _x = false.then(|| x % x);
251+
let _x = false.then(|| x + 1);
252+
let _x = false.then(|| 1 + x);
231253

232254
// const eval doesn't read variables, but floating point math never panics, so we can still emit a
233255
// warning

‎tests/ui/unnecessary_lazy_eval.rs‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,6 @@ fn main() {
185185
// neither bind_instead_of_map nor unnecessary_lazy_eval applies here
186186
let _: Result<usize, usize> = res.and_then(|x| Err(x));
187187
let _: Result<usize, usize> = res.or_else(|err| Ok(err));
188-
189-
issue9422(3);
190-
panicky_arithmetic_ops(3);
191188
}
192189

193190
#[allow(unused)]
@@ -201,33 +198,58 @@ fn issue9422(x: usize) -> Option<usize> {
201198
// (x >= 5).then_some(x - 5) // clippy suggestion panics
202199
}
203200

204-
// https://doc.rust-lang.org/stable/reference/expressions/operator-expr.html#overflow
205-
fn panicky_arithmetic_ops(x: usize) {
201+
fn panicky_arithmetic_ops(x: usize, y: isize) {
202+
#![allow(clippy::identity_op, clippy::eq_op)]
203+
204+
// Even though some of these expressions overflow, they're entirely dependent on constants.
205+
// So, the compiler already emits a warning about overflowing expressions.
206+
// It's a logic error and we want both warnings up front.
207+
// ONLY when a binop side that "matters" for overflow (for `>>`, that is always the right side and
208+
// never the left side) has a non-constant value, avoid linting
209+
206210
let _x = false.then(|| i32::MAX + 1);
211+
//~^ ERROR: unnecessary closure used with `bool::then`
207212
let _x = false.then(|| i32::MAX * 2);
213+
//~^ ERROR: unnecessary closure used with `bool::then`
208214
let _x = false.then(|| i32::MAX - 1);
209215
//~^ ERROR: unnecessary closure used with `bool::then`
210216
let _x = false.then(|| i32::MIN - 1);
211-
#[allow(clippy::identity_op)]
217+
//~^ ERROR: unnecessary closure used with `bool::then`
212218
let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2);
213219
//~^ ERROR: unnecessary closure used with `bool::then`
214220
let _x = false.then(|| 255u8 << 7);
215221
//~^ ERROR: unnecessary closure used with `bool::then`
216222
let _x = false.then(|| 255u8 << 8);
223+
//~^ ERROR: unnecessary closure used with `bool::then`
217224
let _x = false.then(|| 255u8 >> 8);
225+
//~^ ERROR: unnecessary closure used with `bool::then`
218226
let _x = false.then(|| 255u8 >> x);
219227
let _x = false.then(|| i32::MIN / -1);
228+
//~^ ERROR: unnecessary closure used with `bool::then`
220229
let _x = false.then(|| i32::MAX + -1);
221230
//~^ ERROR: unnecessary closure used with `bool::then`
222231
let _x = false.then(|| -i32::MAX);
223232
//~^ ERROR: unnecessary closure used with `bool::then`
224233
let _x = false.then(|| -i32::MIN);
234+
//~^ ERROR: unnecessary closure used with `bool::then`
235+
let _x = false.then(|| -y);
225236
let _x = false.then(|| 255 >> -7);
237+
//~^ ERROR: unnecessary closure used with `bool::then`
226238
let _x = false.then(|| 255 << -1);
239+
//~^ ERROR: unnecessary closure used with `bool::then`
227240
let _x = false.then(|| 1 / 0);
241+
//~^ ERROR: unnecessary closure used with `bool::then`
228242
let _x = false.then(|| x << -1);
243+
//~^ ERROR: unnecessary closure used with `bool::then`
229244
let _x = false.then(|| x << 2);
230245
//~^ ERROR: unnecessary closure used with `bool::then`
246+
let _x = false.then(|| x + x);
247+
let _x = false.then(|| x * x);
248+
let _x = false.then(|| x - x);
249+
let _x = false.then(|| x / x);
250+
let _x = false.then(|| x % x);
251+
let _x = false.then(|| x + 1);
252+
let _x = false.then(|| 1 + x);
231253

232254
// const eval doesn't read variables, but floating point math never panics, so we can still emit a
233255
// warning

‎tests/ui/unnecessary_lazy_eval.stderr‎

Lines changed: 96 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -329,60 +329,148 @@ LL | | or_else(|_| Ok(ext_str.some_field));
329329
| help: use `or(..)` instead: `or(Ok(ext_str.some_field))`
330330

331331
error: unnecessary closure used with `bool::then`
332-
--> $DIR/unnecessary_lazy_eval.rs:208:14
332+
--> $DIR/unnecessary_lazy_eval.rs:210:14
333+
|
334+
LL | let _x = false.then(|| i32::MAX + 1);
335+
| ^^^^^^---------------------
336+
| |
337+
| help: use `then_some(..)` instead: `then_some(i32::MAX + 1)`
338+
339+
error: unnecessary closure used with `bool::then`
340+
--> $DIR/unnecessary_lazy_eval.rs:212:14
341+
|
342+
LL | let _x = false.then(|| i32::MAX * 2);
343+
| ^^^^^^---------------------
344+
| |
345+
| help: use `then_some(..)` instead: `then_some(i32::MAX * 2)`
346+
347+
error: unnecessary closure used with `bool::then`
348+
--> $DIR/unnecessary_lazy_eval.rs:214:14
333349
|
334350
LL | let _x = false.then(|| i32::MAX - 1);
335351
| ^^^^^^---------------------
336352
| |
337353
| help: use `then_some(..)` instead: `then_some(i32::MAX - 1)`
338354

339355
error: unnecessary closure used with `bool::then`
340-
--> $DIR/unnecessary_lazy_eval.rs:212:14
356+
--> $DIR/unnecessary_lazy_eval.rs:216:14
357+
|
358+
LL | let _x = false.then(|| i32::MIN - 1);
359+
| ^^^^^^---------------------
360+
| |
361+
| help: use `then_some(..)` instead: `then_some(i32::MIN - 1)`
362+
363+
error: unnecessary closure used with `bool::then`
364+
--> $DIR/unnecessary_lazy_eval.rs:218:14
341365
|
342366
LL | let _x = false.then(|| (1 + 2 * 3 - 2 / 3 + 9) << 2);
343367
| ^^^^^^-------------------------------------
344368
| |
345369
| help: use `then_some(..)` instead: `then_some((1 + 2 * 3 - 2 / 3 + 9) << 2)`
346370

347371
error: unnecessary closure used with `bool::then`
348-
--> $DIR/unnecessary_lazy_eval.rs:214:14
372+
--> $DIR/unnecessary_lazy_eval.rs:220:14
349373
|
350374
LL | let _x = false.then(|| 255u8 << 7);
351375
| ^^^^^^-------------------
352376
| |
353377
| help: use `then_some(..)` instead: `then_some(255u8 << 7)`
354378

355379
error: unnecessary closure used with `bool::then`
356-
--> $DIR/unnecessary_lazy_eval.rs:220:14
380+
--> $DIR/unnecessary_lazy_eval.rs:222:14
381+
|
382+
LL | let _x = false.then(|| 255u8 << 8);
383+
| ^^^^^^-------------------
384+
| |
385+
| help: use `then_some(..)` instead: `then_some(255u8 << 8)`
386+
387+
error: unnecessary closure used with `bool::then`
388+
--> $DIR/unnecessary_lazy_eval.rs:224:14
389+
|
390+
LL | let _x = false.then(|| 255u8 >> 8);
391+
| ^^^^^^-------------------
392+
| |
393+
| help: use `then_some(..)` instead: `then_some(255u8 >> 8)`
394+
395+
error: unnecessary closure used with `bool::then`
396+
--> $DIR/unnecessary_lazy_eval.rs:227:14
397+
|
398+
LL | let _x = false.then(|| i32::MIN / -1);
399+
| ^^^^^^----------------------
400+
| |
401+
| help: use `then_some(..)` instead: `then_some(i32::MIN / -1)`
402+
403+
error: unnecessary closure used with `bool::then`
404+
--> $DIR/unnecessary_lazy_eval.rs:229:14
357405
|
358406
LL | let _x = false.then(|| i32::MAX + -1);
359407
| ^^^^^^----------------------
360408
| |
361409
| help: use `then_some(..)` instead: `then_some(i32::MAX + -1)`
362410

363411
error: unnecessary closure used with `bool::then`
364-
--> $DIR/unnecessary_lazy_eval.rs:222:14
412+
--> $DIR/unnecessary_lazy_eval.rs:231:14
365413
|
366414
LL | let _x = false.then(|| -i32::MAX);
367415
| ^^^^^^------------------
368416
| |
369417
| help: use `then_some(..)` instead: `then_some(-i32::MAX)`
370418

371419
error: unnecessary closure used with `bool::then`
372-
--> $DIR/unnecessary_lazy_eval.rs:229:14
420+
--> $DIR/unnecessary_lazy_eval.rs:233:14
421+
|
422+
LL | let _x = false.then(|| -i32::MIN);
423+
| ^^^^^^------------------
424+
| |
425+
| help: use `then_some(..)` instead: `then_some(-i32::MIN)`
426+
427+
error: unnecessary closure used with `bool::then`
428+
--> $DIR/unnecessary_lazy_eval.rs:236:14
429+
|
430+
LL | let _x = false.then(|| 255 >> -7);
431+
| ^^^^^^------------------
432+
| |
433+
| help: use `then_some(..)` instead: `then_some(255 >> -7)`
434+
435+
error: unnecessary closure used with `bool::then`
436+
--> $DIR/unnecessary_lazy_eval.rs:238:14
437+
|
438+
LL | let _x = false.then(|| 255 << -1);
439+
| ^^^^^^------------------
440+
| |
441+
| help: use `then_some(..)` instead: `then_some(255 << -1)`
442+
443+
error: unnecessary closure used with `bool::then`
444+
--> $DIR/unnecessary_lazy_eval.rs:240:14
445+
|
446+
LL | let _x = false.then(|| 1 / 0);
447+
| ^^^^^^--------------
448+
| |
449+
| help: use `then_some(..)` instead: `then_some(1 / 0)`
450+
451+
error: unnecessary closure used with `bool::then`
452+
--> $DIR/unnecessary_lazy_eval.rs:242:14
453+
|
454+
LL | let _x = false.then(|| x << -1);
455+
| ^^^^^^----------------
456+
| |
457+
| help: use `then_some(..)` instead: `then_some(x << -1)`
458+
459+
error: unnecessary closure used with `bool::then`
460+
--> $DIR/unnecessary_lazy_eval.rs:244:14
373461
|
374462
LL | let _x = false.then(|| x << 2);
375463
| ^^^^^^---------------
376464
| |
377465
| help: use `then_some(..)` instead: `then_some(x << 2)`
378466

379467
error: unnecessary closure used with `bool::then`
380-
--> $DIR/unnecessary_lazy_eval.rs:236:14
468+
--> $DIR/unnecessary_lazy_eval.rs:258:14
381469
|
382470
LL | let _x = false.then(|| f1 + f2);
383471
| ^^^^^^----------------
384472
| |
385473
| help: use `then_some(..)` instead: `then_some(f1 + f2)`
386474

387-
error: aborting due to 47 previous errors
475+
error: aborting due to 58 previous errors
388476

0 commit comments

Comments
 (0)