Skip to content

Commit b7f9216

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

File tree

3 files changed

+356
-222
lines changed

3 files changed

+356
-222
lines changed

‎clippy_lints/src/operators/arithmetic_side_effects.rs‎

Lines changed: 147 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
use super::ARITHMETIC_SIDE_EFFECTS;
2+
use crate::clippy_utils::res::MaybeQPath as _;
23
use clippy_config::Conf;
34
use clippy_utils::consts::{ConstEvalCtxt, Constant};
45
use clippy_utils::diagnostics::span_lint;
56
use clippy_utils::res::MaybeDef;
67
use clippy_utils::{expr_or_init, is_from_proc_macro, is_lint_allowed, peel_hir_expr_refs, peel_hir_expr_unary, sym};
78
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
89
use rustc_lint::{LateContext, LateLintPass};
9-
use rustc_middle::ty::{self, Ty};
10+
use rustc_middle::ty::{self, Ty, UintTy};
1011
use rustc_session::impl_lint_pass;
1112
use rustc_span::{Span, Symbol};
1213
use {rustc_ast as ast, rustc_hir as hir};
@@ -88,74 +89,16 @@ impl ArithmeticSideEffects {
8889
self.allowed_unary.contains(ty_string_elem)
8990
}
9091

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-
13192
// Common entry-point to avoid code duplication.
13293
fn issue_lint<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
13394
if is_from_proc_macro(cx, expr) {
13495
return;
13596
}
136-
13797
let msg = "arithmetic operation that can potentially result in unexpected side-effects";
13898
span_lint(cx, ARITHMETIC_SIDE_EFFECTS, expr.span, msg);
13999
self.expr_span = Some(expr.span);
140100
}
141101

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-
159102
/// Methods like `add_assign` are send to their `BinOps` references.
160103
fn manage_sugar_methods<'tcx>(
161104
&mut self,
@@ -213,59 +156,53 @@ impl ArithmeticSideEffects {
213156
&& let hir::ExprKind::MethodCall(method, receiver, [], _) = actual_lhs.kind
214157
&& method.ident.name == sym::get
215158
&& 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)
159+
&& is_non_zero_u(cx, receiver_ty)
160+
&& literal_integer(cx, actual_rhs) == Some(1)
218161
{
219162
return;
220163
}
221164

222165
let lhs_ty = cx.typeck_results().expr_ty(actual_lhs).peel_refs();
223166
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) {
167+
if self.has_allowed_binary(lhs_ty, rhs_ty)
168+
| has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty)
169+
| is_safe_due_to_smaller_source_type(cx, op, (actual_lhs, lhs_ty), actual_rhs)
170+
| is_safe_due_to_smaller_source_type(cx, op, (actual_rhs, rhs_ty), actual_lhs)
171+
{
228172
return;
229173
}
230-
231-
let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
174+
if is_integer(lhs_ty) && is_integer(rhs_ty) {
232175
if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {
233176
// At least for integers, shifts are already handled by the CTFE
234177
return;
235178
}
236-
match (
237-
Self::literal_integer(cx, actual_lhs),
238-
Self::literal_integer(cx, actual_rhs),
239-
) {
240-
(None, None) => false,
179+
match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
241180
(None, Some(n)) => match (&op, n) {
242181
// 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) {
182+
(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => return,
252183
// Adding or subtracting zeros is always a no-op
253184
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
254185
// Multiplication by 1 or 0 will never overflow
255186
| (hir::BinOpKind::Mul, 0 | 1)
256-
=> true,
257-
_ => false,
187+
=> return,
188+
_ => {},
258189
},
259-
(Some(_), Some(_)) => {
260-
matches!((lhs_ref_counter, rhs_ref_counter), (0, 0))
190+
(Some(n), None)
191+
if matches!(
192+
(&op, n),
193+
// Adding or subtracting zeros is always a no-op
194+
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
195+
// Multiplication by 1 or 0 will never overflow
196+
| (hir::BinOpKind::Mul, 0 | 1)
197+
) =>
198+
{
199+
return;
261200
},
201+
(Some(_), Some(_)) if matches!((lhs_ref_counter, rhs_ref_counter), (0, 0)) => return,
202+
_ => {},
262203
}
263-
} else {
264-
false
265-
};
266-
if !has_valid_op {
267-
self.issue_lint(cx, expr);
268204
}
205+
self.issue_lint(cx, expr);
269206
}
270207

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

‎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)