teach eager_or_lazy about panicky arithmetic operations

[y21]

Fixes #9422
Fixes #9814
Fixes #11793

It's a bit sad that we have to do this because arithmetic operations seemed to me like the prime example where a closure would not be necessary, but this has "side effects" (changes behavior when going from lazy to eager) as some of these panic on overflow/underflow if compiled with -Coverflow-checks (which is the default in debug mode).
Given the number of backlinks in the mentioned issues, this seems to be a FP that is worth fixing, probably.

changelog: [unnecessary_lazy_evaluations]: don't lint if closure has panicky arithmetic operations

[rustbot]

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

[Centri3]

Just a thought, but maybe you can call clippy_utils::consts::constant to see if it'll panic or not? (It'll return None if either it's not a constant or over/underflows), in that case it can still lint.

(That could also be solid new lint potential!)

[y21]

@Centri3 Hmm, so you mean, if constant(lhs, rhs) returns Some(_), then changing lazy to eager is always safe because it never returns Some(_) on overflow? I think that could work

[Centri3]

Yep :D Unfortunately won't catch stuff like x + 2 where x is 3 but that should be enough

[y21]

It doesn't look like Some(_) is guaranteed to mean "no overflow".
Clippy's const eval represents numbers in its bit representation as a u128 and it does checked operations with i128, so it does return None if it overflows a i128, but not if it overflows an i32. So e.g. i32::MAX + 1 still fits in a i128 so that's fine, but i128::MAX + 1 is not.

I'm not sure if this could be considered a bug on its own. 🤔
Maybe it could also store the integer type and check that it fits in the target type when evaluating a binop

[Centri3]

That's bizarre, that should probably be fixed (as it seems binop was intended to return None on overflow.)

The type of the integer at hand is known so this is probably a pretty simple fix (though maybe this should be a separate function? binop_checked and binop?)

[y21]

@Centri3 just so we don't try to fix something both at the same time, should I fix it or do you want to do it? But also, why a separate function if binop was intended to catch overflows (now that I think about it, this should really be considered a bug, because it means const eval has a different value from what it would be at runtime w/o overflow checks)?
And how would that binop_checked function be used? ConstEvalLateContext can't be constructed outside of the module, so I'm not sure how one could call binop_checked then. Or am I misunderstanding?

[Centri3]

Nah I'm not, feel free to :D I was thinking perhaps a flag to expr, but it should definitely be changed on binop (that was just thrown out in case it was, in fact, not a bug)

[bors]

☔ The latest upstream changes (presumably #11095) made this pull request unmergeable. Please resolve the merge conflicts.

[guilliamxavier]

Also bitten by the issue (in a case where checked_* would not be appropriate), would love to see this fix land 🙂 Two remarks though

[bors]

☔ The latest upstream changes (presumably #11565) made this pull request unmergeable. Please resolve the merge conflicts.

[holly-hacker]

Hi, are there any blockers for this PR besides the current merge conflicts, or is it just waiting for a review?

[y21]

Just waiting for a review. I've been holding off fixing the conflicts because it's been a quiet PR for months and I don't want to rebase this every few days to weeks when it's not clear how long until a review.

@Jarcho Do you plan to review this still? Or should we reroll to get another reviewer to look at this, if you're busy? 🙂

[Jarcho]

This would be better done with is_const_evaluable. If evaluating the constant would panic it's most likely a bug as it would always panic when run.

Also floating point math never panics.

[y21]

Do we still want to keep the changes in clippy's const eval (returning None in case of overflow)? I'd consider it a bug that given the expression Binary(Add, u32::MAX, 1) it returns 4294967296, a number that is out of range for a u32. It should really return either None (or wrap around), no? None seems safer since the result depends on a cfg

[Jarcho]

That can be kept as is. Not handling overflow properly was definitely a bug.

[y21]

This would be better done with is_const_evaluable. If evaluating the constant would panic it's most likely a bug as it would always panic when run.

I thought about this for a bit. I'm not sure. If I understand this right, doing that would mean we would still lint here, right?

trait Foo {
  const ASSOC: i32;
  fn bar() {
    (Self::ASSOC > 0).then(|| Self::ASSOC - 1);
  }
}

is_const_evaluatable would return true for the Self::ASSOC - 1 expression, but I don't think this is enough that we should suggest replacing it with then_some. A downstream impl can still implement the trait and give the associated constant the value zero. Then the function will unconditionally overflow.

[Jarcho]

Fair point. That just leaves changing the ops that only depend on the second argument.

[y21]

I think this should be ready for review now. / and % was pretty confusing to allow certain cases that rustc can check and disallow others that depend on runtime values and are able to panic.

I also had to fix a bug in match_same_arms because it didn't respect #[allow] attributes on arms.
(I found that merging the arms was less readable, especially because there were comments that explained the patterns)

[Jarcho]

r=me whether you make the change or not.

[y21]

Is this waiting on me to do r=Jarcho? Just making sure. Because I can't do that, I don't think. I did make the requested change.

[Jarcho]

For some reason I thought you did. @bors r+

[bors]

📌 Commit aca4086 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit aca4086 with merge e8e9510...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing e8e9510 to master...