Skip to content

Commit 22ec98f

Browse files
c410-f3rc410-f3r
committed
Consider type conversion that won't overflow
1 parent 82d729c commit 22ec98f

File tree

3 files changed

+357
-222
lines changed

3 files changed

+357
-222
lines changed

‎clippy_lints/src/operators/arithmetic_side_effects.rs‎

Lines changed: 148 additions & 93 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,59 +155,53 @@ 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+
&& literal_integer(cx, actual_rhs) == Some(1)
218160
{
219161
return;
220162
}
221163

222164
let lhs_ty = cx.typeck_results().expr_ty(actual_lhs).peel_refs();
223165
let rhs_ty = cx.typeck_results().expr_ty_adjusted(actual_rhs).peel_refs();
224-
if self.has_allowed_binary(lhs_ty, rhs_ty) {
225-
return;
226-
}
227-
if Self::has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
166+
if self.has_allowed_binary(lhs_ty, rhs_ty)
167+
| has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty)
168+
| is_safe_due_to_smaller_source_type(cx, op, (actual_lhs, lhs_ty), actual_rhs)
169+
| is_safe_due_to_smaller_source_type(cx, op, (actual_rhs, rhs_ty), actual_lhs)
170+
{
228171
return;
229172
}
230-
231-
let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
173+
if is_integer(lhs_ty) && is_integer(rhs_ty) {
232174
if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {
233175
// At least for integers, shifts are already handled by the CTFE
234176
return;
235177
}
236-
match (
237-
Self::literal_integer(cx, actual_lhs),
238-
Self::literal_integer(cx, actual_rhs),
239-
) {
240-
(None, None) => false,
178+
match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
241179
(None, Some(n)) => match (&op, n) {
242180
// Division and module are always valid if applied to non-zero integers
243-
(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => true,
244-
// Adding or subtracting zeros is always a no-op
245-
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
246-
// Multiplication by 1 or 0 will never overflow
247-
| (hir::BinOpKind::Mul, 0 | 1)
248-
=> true,
249-
_ => false,
250-
},
251-
(Some(n), None) => match (&op, n) {
181+
(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => return,
252182
// Adding or subtracting zeros is always a no-op
253183
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
254184
// Multiplication by 1 or 0 will never overflow
255185
| (hir::BinOpKind::Mul, 0 | 1)
256-
=> true,
257-
_ => false,
186+
=> return,
187+
_ => {},
258188
},
259-
(Some(_), Some(_)) => {
260-
matches!((lhs_ref_counter, rhs_ref_counter), (0, 0))
189+
(Some(n), None)
190+
if matches!(
191+
(&op, n),
192+
// Adding or subtracting zeros is always a no-op
193+
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
194+
// Multiplication by 1 or 0 will never overflow
195+
| (hir::BinOpKind::Mul, 0 | 1)
196+
) =>
197+
{
198+
return;
261199
},
200+
(Some(_), Some(_)) if matches!((lhs_ref_counter, rhs_ref_counter), (0, 0)) => return,
201+
_ => {},
262202
}
263-
} else {
264-
false
265-
};
266-
if !has_valid_op {
267-
self.issue_lint(cx, expr);
268203
}
204+
self.issue_lint(cx, expr);
269205
}
270206

271207
/// There are some integer methods like `wrapping_div` that will panic depending on the
@@ -285,15 +221,15 @@ impl ArithmeticSideEffects {
285221
return;
286222
}
287223
let instance_ty = cx.typeck_results().expr_ty_adjusted(receiver);
288-
if !Self::is_integral(instance_ty) {
224+
if !is_integer(instance_ty) {
289225
return;
290226
}
291227
self.manage_sugar_methods(cx, expr, receiver, ps, arg);
292228
if !self.disallowed_int_methods.contains(&ps.ident.name) {
293229
return;
294230
}
295231
let (actual_arg, _) = peel_hir_expr_refs(arg);
296-
match Self::literal_integer(cx, actual_arg) {
232+
match literal_integer(cx, actual_arg) {
297233
None | Some(0) => self.issue_lint(cx, arg),
298234
Some(_) => {},
299235
}
@@ -317,7 +253,7 @@ impl ArithmeticSideEffects {
317253
return;
318254
}
319255
let actual_un_expr = peel_hir_expr_refs(un_expr).0;
320-
if Self::literal_integer(cx, actual_un_expr).is_some() {
256+
if literal_integer(cx, actual_un_expr).is_some() {
321257
return;
322258
}
323259
self.issue_lint(cx, expr);
@@ -385,3 +321,122 @@ impl<'tcx> LateLintPass<'tcx> for ArithmeticSideEffects {
385321
}
386322
}
387323
}
324+
325+
/// Detects a type-casting conversion and returns the type of the original expression. For
326+
/// example, `let foo = u64::from(bar)`.
327+
fn find_original_primitive_ty<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {
328+
if let hir::ExprKind::Call(path, [arg]) = &expr.kind
329+
&& let hir::ExprKind::Path(qpath) = &path.kind
330+
&& let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id()
331+
&& let [sym::core, sym::convert, sym::From, sym::from] = cx.get_def_path(def_id).as_slice()
332+
{
333+
Some(cx.typeck_results().expr_ty(arg))
334+
} else {
335+
None
336+
}
337+
}
338+
339+
/// Verifies built-in types that have specific allowed operations
340+
fn has_specific_allowed_type_and_operation<'tcx>(
341+
cx: &LateContext<'tcx>,
342+
lhs_ty: Ty<'tcx>,
343+
op: hir::BinOpKind,
344+
rhs_ty: Ty<'tcx>,
345+
) -> bool {
346+
let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
347+
let is_sat_or_wrap = |ty: Ty<'_>| matches!(ty.opt_diag_name(cx), Some(sym::Saturating | sym::Wrapping));
348+
349+
// If the RHS is `NonZero<u*>`, then division or module by zero will never occur.
350+
if is_non_zero_u(cx, rhs_ty) && is_div_or_rem {
351+
return true;
352+
}
353+
354+
// `Saturation` and `Wrapping` can overflow if the RHS is zero in a division or module.
355+
if is_sat_or_wrap(lhs_ty) {
356+
return !is_div_or_rem;
357+
}
358+
359+
false
360+
}
361+
362+
// For example, `i8` or `u128` and possible associated references like `&&u16`.
363+
fn is_integer(ty: Ty<'_>) -> bool {
364+
ty.peel_refs().is_integral()
365+
}
366+
367+
fn is_non_zero_u(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
368+
if let ty::Adt(adt, substs) = ty.kind()
369+
&& cx.tcx.is_diagnostic_item(sym::NonZero, adt.did())
370+
&& let int_type = substs.type_at(0)
371+
&& matches!(int_type.kind(), ty::Uint(_))
372+
{
373+
true
374+
} else {
375+
false
376+
}
377+
}
378+
379+
/// If one side is a literal it is possible to evaluate overflows as long as the other side has a
380+
/// smaller type. `0` and `1` suffixes indicate different sides.
381+
///
382+
/// For example, `1000u64 + u64::from(some_runtime_variable_of_type_u8)`.
383+
fn is_safe_due_to_smaller_source_type(
384+
cx: &LateContext<'_>,
385+
op: hir::BinOpKind,
386+
(expr0, ty0): (&hir::Expr<'_>, Ty<'_>),
387+
expr1: &hir::Expr<'_>,
388+
) -> bool {
389+
let Some(num0) = literal_integer(cx, expr0) else {
390+
return false;
391+
};
392+
let Some(orig_ty1) = find_original_primitive_ty(cx, expr1) else {
393+
return false;
394+
};
395+
let Some(num1) = max_int_num(orig_ty1) else {
396+
return false;
397+
};
398+
let Some(rslt) = (match op {
399+
hir::BinOpKind::Add => num0.checked_add(num1),
400+
hir::BinOpKind::Mul => num0.checked_mul(num1),
401+
_ => None,
402+
}) else {
403+
return false;
404+
};
405+
match ty0.peel_refs().kind() {
406+
ty::Uint(UintTy::U16) => u16::try_from(rslt).is_ok(),
407+
ty::Uint(UintTy::U32) => u32::try_from(rslt).is_ok(),
408+
ty::Uint(UintTy::U64) => u64::try_from(rslt).is_ok(),
409+
ty::Uint(UintTy::U128) => true,
410+
ty::Uint(UintTy::Usize) => usize::try_from(rslt).is_ok(),
411+
_ => false,
412+
}
413+
}
414+
415+
/// Returns the numeric value of a literal integer originated from `expr`, if any.
416+
///
417+
/// Literal integers can be originated from adhoc declarations like `1`, associated constants
418+
/// like `i32::MAX` or constant references like `N` from `const N: i32 = 1;`,
419+
fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
420+
let actual = peel_hir_expr_unary(expr).0;
421+
if let hir::ExprKind::Lit(lit) = actual.kind
422+
&& let ast::LitKind::Int(n, _) = lit.node
423+
{
424+
return Some(n.get());
425+
}
426+
if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
427+
return Some(n);
428+
}
429+
None
430+
}
431+
432+
fn max_int_num(ty: Ty<'_>) -> Option<u128> {
433+
match ty.peel_refs().kind() {
434+
ty::Uint(UintTy::U8) => Some(u8::MAX.into()),
435+
ty::Uint(UintTy::U16) => Some(u16::MAX.into()),
436+
ty::Uint(UintTy::U32) => Some(u32::MAX.into()),
437+
ty::Uint(UintTy::U64) => Some(u64::MAX.into()),
438+
ty::Uint(UintTy::U128) => Some(u128::MAX),
439+
ty::Uint(UintTy::Usize) => usize::MAX.try_into().ok(),
440+
_ => None,
441+
}
442+
}

‎tests/ui/arithmetic_side_effects.rs‎

Lines changed: 56 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,59 @@ 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+
729+
pub fn type_conversion_does_not_escape_its_context() {
730+
struct Foo;
731+
impl Foo {
732+
fn from(n: u8) -> u64 {
733+
u64::from(n)
734+
}
735+
}
736+
let _ = Duration::from_secs(86400 * Foo::from(1));
737+
//~^ arithmetic_side_effects
738+
739+
fn shift(x: u8) -> u64 {
740+
1 << u64::from(x)
741+
}
742+
let _ = Duration::from_secs(86400 * shift(1));
743+
//~^ arithmetic_side_effects
744+
}
745+
690746
fn main() {}

0 commit comments

Comments
 (0)