Skip to content

Commit 6b7a22e

Browse files
c410-f3rc410-f3r
committed
Fix #15943 
1 parent 82d729c commit 6b7a22e

File tree

3 files changed

+310
-200
lines changed

3 files changed

+310
-200
lines changed

‎clippy_lints/src/operators/arithmetic_side_effects.rs‎

Lines changed: 130 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use clippy_utils::res::MaybeDef;
66
use clippy_utils::{expr_or_init, is_from_proc_macro, is_lint_allowed, peel_hir_expr_refs, peel_hir_expr_unary, sym};
77
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
88
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_middle::ty::{self, Ty};
9+
use rustc_middle::ty::{self, Ty, UintTy};
1010
use rustc_session::impl_lint_pass;
1111
use rustc_span::{Span, Symbol};
1212
use {rustc_ast as ast, rustc_hir as hir};
@@ -88,74 +88,16 @@ impl ArithmeticSideEffects {
8888
self.allowed_unary.contains(ty_string_elem)
8989
}
9090

91-
fn is_non_zero_u(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
92-
if let ty::Adt(adt, substs) = ty.kind()
93-
&& cx.tcx.is_diagnostic_item(sym::NonZero, adt.did())
94-
&& let int_type = substs.type_at(0)
95-
&& matches!(int_type.kind(), ty::Uint(_))
96-
{
97-
true
98-
} else {
99-
false
100-
}
101-
}
102-
103-
/// Verifies built-in types that have specific allowed operations
104-
fn has_specific_allowed_type_and_operation<'tcx>(
105-
cx: &LateContext<'tcx>,
106-
lhs_ty: Ty<'tcx>,
107-
op: hir::BinOpKind,
108-
rhs_ty: Ty<'tcx>,
109-
) -> bool {
110-
let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
111-
let is_sat_or_wrap = |ty: Ty<'_>| ty.is_diag_item(cx, sym::Saturating) || ty.is_diag_item(cx, sym::Wrapping);
112-
113-
// If the RHS is `NonZero<u*>`, then division or module by zero will never occur.
114-
if Self::is_non_zero_u(cx, rhs_ty) && is_div_or_rem {
115-
return true;
116-
}
117-
118-
// `Saturation` and `Wrapping` can overflow if the RHS is zero in a division or module.
119-
if is_sat_or_wrap(lhs_ty) {
120-
return !is_div_or_rem;
121-
}
122-
123-
false
124-
}
125-
126-
// For example, 8i32 or &i64::MAX.
127-
fn is_integral(ty: Ty<'_>) -> bool {
128-
ty.peel_refs().is_integral()
129-
}
130-
13191
// Common entry-point to avoid code duplication.
13292
fn issue_lint<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
13393
if is_from_proc_macro(cx, expr) {
13494
return;
13595
}
136-
13796
let msg = "arithmetic operation that can potentially result in unexpected side-effects";
13897
span_lint(cx, ARITHMETIC_SIDE_EFFECTS, expr.span, msg);
13998
self.expr_span = Some(expr.span);
14099
}
141100

142-
/// Returns the numeric value of a literal integer originated from `expr`, if any.
143-
///
144-
/// Literal integers can be originated from adhoc declarations like `1`, associated constants
145-
/// like `i32::MAX` or constant references like `N` from `const N: i32 = 1;`,
146-
fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
147-
let actual = peel_hir_expr_unary(expr).0;
148-
if let hir::ExprKind::Lit(lit) = actual.kind
149-
&& let ast::LitKind::Int(n, _) = lit.node
150-
{
151-
return Some(n.get());
152-
}
153-
if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
154-
return Some(n);
155-
}
156-
None
157-
}
158-
159101
/// Methods like `add_assign` are send to their `BinOps` references.
160102
fn manage_sugar_methods<'tcx>(
161103
&mut self,
@@ -213,8 +155,8 @@ impl ArithmeticSideEffects {
213155
&& let hir::ExprKind::MethodCall(method, receiver, [], _) = actual_lhs.kind
214156
&& method.ident.name == sym::get
215157
&& let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs()
216-
&& Self::is_non_zero_u(cx, receiver_ty)
217-
&& let Some(1) = Self::literal_integer(cx, actual_rhs)
158+
&& is_non_zero_u(cx, receiver_ty)
159+
&& let Some(1) = literal_integer(cx, actual_rhs)
218160
{
219161
return;
220162
}
@@ -224,19 +166,21 @@ impl ArithmeticSideEffects {
224166
if self.has_allowed_binary(lhs_ty, rhs_ty) {
225167
return;
226168
}
227-
if Self::has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
169+
if has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
228170
return;
229171
}
230-
231-
let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
172+
if manage_integer_types(cx, op, (actual_lhs, lhs_ty), actual_rhs) {
173+
return;
174+
}
175+
if manage_integer_types(cx, op, (actual_rhs, rhs_ty), actual_lhs) {
176+
return;
177+
}
178+
let has_valid_op = if is_integer(lhs_ty) && is_integer(rhs_ty) {
232179
if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {
233180
// At least for integers, shifts are already handled by the CTFE
234181
return;
235182
}
236-
match (
237-
Self::literal_integer(cx, actual_lhs),
238-
Self::literal_integer(cx, actual_rhs),
239-
) {
183+
match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
240184
(None, None) => false,
241185
(None, Some(n)) => match (&op, n) {
242186
// Division and module are always valid if applied to non-zero integers
@@ -285,15 +229,15 @@ impl ArithmeticSideEffects {
285229
return;
286230
}
287231
let instance_ty = cx.typeck_results().expr_ty_adjusted(receiver);
288-
if !Self::is_integral(instance_ty) {
232+
if !is_integer(instance_ty) {
289233
return;
290234
}
291235
self.manage_sugar_methods(cx, expr, receiver, ps, arg);
292236
if !self.disallowed_int_methods.contains(&ps.ident.name) {
293237
return;
294238
}
295239
let (actual_arg, _) = peel_hir_expr_refs(arg);
296-
match Self::literal_integer(cx, actual_arg) {
240+
match literal_integer(cx, actual_arg) {
297241
None | Some(0) => self.issue_lint(cx, arg),
298242
Some(_) => {},
299243
}
@@ -317,7 +261,7 @@ impl ArithmeticSideEffects {
317261
return;
318262
}
319263
let actual_un_expr = peel_hir_expr_refs(un_expr).0;
320-
if Self::literal_integer(cx, actual_un_expr).is_some() {
264+
if literal_integer(cx, actual_un_expr).is_some() {
321265
return;
322266
}
323267
self.issue_lint(cx, expr);
@@ -385,3 +329,118 @@ impl<'tcx> LateLintPass<'tcx> for ArithmeticSideEffects {
385329
}
386330
}
387331
}
332+
333+
/// For example, if the variable `foo` of type `u32` comes from another variable `bar` of type
334+
/// `u8` like `let foo = u64::from(bar)`.
335+
fn find_original_primitive_ty<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {
336+
if let hir::ExprKind::Call(_, [arg]) = expr.kind {
337+
Some(cx.typeck_results().expr_ty(arg))
338+
} else {
339+
None
340+
}
341+
}
342+
343+
/// Verifies built-in types that have specific allowed operations
344+
fn has_specific_allowed_type_and_operation<'tcx>(
345+
cx: &LateContext<'tcx>,
346+
lhs_ty: Ty<'tcx>,
347+
op: hir::BinOpKind,
348+
rhs_ty: Ty<'tcx>,
349+
) -> bool {
350+
let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
351+
let is_sat_or_wrap = |ty: Ty<'_>| ty.is_diag_item(cx, sym::Saturating) || ty.is_diag_item(cx, sym::Wrapping);
352+
353+
// If the RHS is `NonZero<u*>`, then division or module by zero will never occur.
354+
if is_non_zero_u(cx, rhs_ty) && is_div_or_rem {
355+
return true;
356+
}
357+
358+
// `Saturation` and `Wrapping` can overflow if the RHS is zero in a division or module.
359+
if is_sat_or_wrap(lhs_ty) {
360+
return !is_div_or_rem;
361+
}
362+
363+
false
364+
}
365+
366+
// For example, `i8` or `u128`
367+
fn is_integer(ty: Ty<'_>) -> bool {
368+
ty.peel_refs().is_integral()
369+
}
370+
371+
fn is_non_zero_u(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
372+
if let ty::Adt(adt, substs) = ty.kind()
373+
&& cx.tcx.is_diagnostic_item(sym::NonZero, adt.did())
374+
&& let int_type = substs.type_at(0)
375+
&& matches!(int_type.kind(), ty::Uint(_))
376+
{
377+
true
378+
} else {
379+
false
380+
}
381+
}
382+
383+
/// Returns the numeric value of a literal integer originated from `expr`, if any.
384+
///
385+
/// Literal integers can be originated from adhoc declarations like `1`, associated constants
386+
/// like `i32::MAX` or constant references like `N` from `const N: i32 = 1;`,
387+
fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
388+
let actual = peel_hir_expr_unary(expr).0;
389+
if let hir::ExprKind::Lit(lit) = actual.kind
390+
&& let ast::LitKind::Int(n, _) = lit.node
391+
{
392+
return Some(n.get());
393+
}
394+
if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
395+
return Some(n);
396+
}
397+
None
398+
}
399+
400+
/// If one side is a literal it is possible to evaluate overflows as long as the other side has a
401+
/// smaller type. `0` and `1` suffixes indicate different sides.
402+
///
403+
/// For example, `1000u64 + u64::from(some_runtime_variable_of_type_u8)`.
404+
fn manage_integer_types(
405+
cx: &LateContext<'_>,
406+
op: hir::BinOpKind,
407+
(expr0, ty0): (&hir::Expr<'_>, Ty<'_>),
408+
expr1: &hir::Expr<'_>,
409+
) -> bool {
410+
let Some(num0) = literal_integer(cx, expr0) else {
411+
return false;
412+
};
413+
let Some(orig_ty1) = find_original_primitive_ty(cx, expr1) else {
414+
return false;
415+
};
416+
let Some(num1) = max_int_num(orig_ty1) else {
417+
return false;
418+
};
419+
let Some(rslt) = (match op {
420+
hir::BinOpKind::Add => num0.checked_add(num1),
421+
hir::BinOpKind::Mul => num0.checked_mul(num1),
422+
_ => None,
423+
}) else {
424+
return false;
425+
};
426+
match ty0.peel_refs().kind() {
427+
ty::Uint(UintTy::U16) => u16::try_from(rslt).is_ok(),
428+
ty::Uint(UintTy::U32) => u32::try_from(rslt).is_ok(),
429+
ty::Uint(UintTy::U64) => u64::try_from(rslt).is_ok(),
430+
ty::Uint(UintTy::U128) => true,
431+
ty::Uint(UintTy::Usize) => usize::try_from(rslt).is_ok(),
432+
_ => false,
433+
}
434+
}
435+
436+
fn max_int_num(ty: Ty<'_>) -> Option<u128> {
437+
match ty.peel_refs().kind() {
438+
ty::Uint(UintTy::U8) => Some(u8::MAX.into()),
439+
ty::Uint(UintTy::U16) => Some(u16::MAX.into()),
440+
ty::Uint(UintTy::U32) => Some(u32::MAX.into()),
441+
ty::Uint(UintTy::U64) => Some(u64::MAX.into()),
442+
ty::Uint(UintTy::U128) => Some(u128::MAX),
443+
ty::Uint(UintTy::Usize) => usize::MAX.try_into().ok(),
444+
_ => None,
445+
}
446+
}

‎tests/ui/arithmetic_side_effects.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
extern crate proc_macro_derive;
1818

1919
use core::num::{NonZero, Saturating, Wrapping};
20+
use core::time::Duration;
2021

2122
const ONE: i32 = 1;
2223
const ZERO: i32 = 0;
@@ -687,4 +688,42 @@ pub fn explicit_methods() {
687688
//~^ arithmetic_side_effects
688689
}
689690

691+
pub fn issue_15943(days: u8) -> Duration {
692+
Duration::from_secs(86400 * u64::from(days))
693+
}
694+
695+
pub fn type_conversion_add() {
696+
let _ = u128::MAX + u128::from(1u8);
697+
//~^ arithmetic_side_effects
698+
let _ = 1u128 + u128::from(1u16);
699+
let _ = 1u128 + u128::from(1u32);
700+
let _ = 1u128 + u128::from(1u64);
701+
702+
let _ = 1u64 + u64::from(1u8);
703+
let _ = 1u64 + u64::from(1u16);
704+
let _ = 1u64 + u64::from(1u32);
705+
706+
let _ = 1u32 + u32::from(1u8);
707+
let _ = 1u32 + u32::from(1u16);
708+
709+
let _ = 1u16 + u16::from(1u8);
710+
}
711+
712+
pub fn type_conversion_mul() {
713+
let _ = u128::MAX * u128::from(1u8);
714+
//~^ arithmetic_side_effects
715+
let _ = 1u128 * u128::from(1u16);
716+
let _ = 1u128 * u128::from(1u32);
717+
let _ = 1u128 * u128::from(1u64);
718+
719+
let _ = 1u64 * u64::from(1u8);
720+
let _ = 1u64 * u64::from(1u16);
721+
let _ = 1u64 * u64::from(1u32);
722+
723+
let _ = 1u32 * u32::from(1u8);
724+
let _ = 1u32 * u32::from(1u16);
725+
726+
let _ = 1u16 * u16::from(1u8);
727+
}
728+
690729
fn main() {}

0 commit comments

Comments
 (0)