Skip to content

[arithmetic_side_effects] Fix #15943 #15950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+356 −222
Open

[arithmetic_side_effects] Fix #15943 #15950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 147 additions & 93 deletions clippy_lints/src/operators/arithmetic_side_effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use super::ARITHMETIC_SIDE_EFFECTS;
use crate::clippy_utils::res::MaybeQPath as _;
use clippy_config::Conf;
use clippy_utils::consts::{ConstEvalCtxt, Constant};
use clippy_utils::diagnostics::span_lint;
use clippy_utils::res::MaybeDef;
use clippy_utils::{expr_or_init, is_from_proc_macro, is_lint_allowed, peel_hir_expr_refs, peel_hir_expr_unary, sym};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{self, Ty};
use rustc_middle::ty::{self, Ty, UintTy};
use rustc_session::impl_lint_pass;
use rustc_span::{Span, Symbol};
use {rustc_ast as ast, rustc_hir as hir};
Expand Down Expand Up @@ -88,74 +89,16 @@ impl ArithmeticSideEffects {
self.allowed_unary.contains(ty_string_elem)
}

fn is_non_zero_u(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
if let ty::Adt(adt, substs) = ty.kind()
&& cx.tcx.is_diagnostic_item(sym::NonZero, adt.did())
&& let int_type = substs.type_at(0)
&& matches!(int_type.kind(), ty::Uint(_))
{
true
} else {
false
}
}

/// Verifies built-in types that have specific allowed operations
fn has_specific_allowed_type_and_operation<'tcx>(
cx: &LateContext<'tcx>,
lhs_ty: Ty<'tcx>,
op: hir::BinOpKind,
rhs_ty: Ty<'tcx>,
) -> bool {
let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
let is_sat_or_wrap = |ty: Ty<'_>| ty.is_diag_item(cx, sym::Saturating) || ty.is_diag_item(cx, sym::Wrapping);

// If the RHS is `NonZero<u*>`, then division or module by zero will never occur.
if Self::is_non_zero_u(cx, rhs_ty) && is_div_or_rem {
return true;
}

// `Saturation` and `Wrapping` can overflow if the RHS is zero in a division or module.
if is_sat_or_wrap(lhs_ty) {
return !is_div_or_rem;
}

false
}

// For example, 8i32 or &i64::MAX.
fn is_integral(ty: Ty<'_>) -> bool {
ty.peel_refs().is_integral()
}

// Common entry-point to avoid code duplication.
fn issue_lint<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if is_from_proc_macro(cx, expr) {
return;
}

let msg = "arithmetic operation that can potentially result in unexpected side-effects";
span_lint(cx, ARITHMETIC_SIDE_EFFECTS, expr.span, msg);
self.expr_span = Some(expr.span);
}

/// Returns the numeric value of a literal integer originated from `expr`, if any.
///
/// Literal integers can be originated from adhoc declarations like `1`, associated constants
/// like `i32::MAX` or constant references like `N` from `const N: i32 = 1;`,
fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
let actual = peel_hir_expr_unary(expr).0;
if let hir::ExprKind::Lit(lit) = actual.kind
&& let ast::LitKind::Int(n, _) = lit.node
{
return Some(n.get());
}
if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
return Some(n);
}
None
}
Comment on lines -146 to -157
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why I placed these non-self mehods inside the impl block. It it better to put them outside IMO.


/// Methods like `add_assign` are send to their `BinOps` references.
fn manage_sugar_methods<'tcx>(
&mut self,
Expand Down Expand Up @@ -213,59 +156,53 @@ impl ArithmeticSideEffects {
&& let hir::ExprKind::MethodCall(method, receiver, [], _) = actual_lhs.kind
&& method.ident.name == sym::get
&& let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs()
&& Self::is_non_zero_u(cx, receiver_ty)
&& let Some(1) = Self::literal_integer(cx, actual_rhs)
&& is_non_zero_u(cx, receiver_ty)
&& literal_integer(cx, actual_rhs) == Some(1)
{
return;
}

let lhs_ty = cx.typeck_results().expr_ty(actual_lhs).peel_refs();
let rhs_ty = cx.typeck_results().expr_ty_adjusted(actual_rhs).peel_refs();
if self.has_allowed_binary(lhs_ty, rhs_ty) {
return;
}
if Self::has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty) {
if self.has_allowed_binary(lhs_ty, rhs_ty)
| has_specific_allowed_type_and_operation(cx, lhs_ty, op, rhs_ty)
| is_safe_due_to_smaller_source_type(cx, op, (actual_lhs, lhs_ty), actual_rhs)
| is_safe_due_to_smaller_source_type(cx, op, (actual_rhs, rhs_ty), actual_lhs)
{
return;
}

let has_valid_op = if Self::is_integral(lhs_ty) && Self::is_integral(rhs_ty) {
if is_integer(lhs_ty) && is_integer(rhs_ty) {
if let hir::BinOpKind::Shl | hir::BinOpKind::Shr = op {
// At least for integers, shifts are already handled by the CTFE
return;
}
match (
Self::literal_integer(cx, actual_lhs),
Self::literal_integer(cx, actual_rhs),
) {
(None, None) => false,
match (literal_integer(cx, actual_lhs), literal_integer(cx, actual_rhs)) {
(None, Some(n)) => match (&op, n) {
// Division and module are always valid if applied to non-zero integers
(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => true,
// Adding or subtracting zeros is always a no-op
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
// Multiplication by 1 or 0 will never overflow
| (hir::BinOpKind::Mul, 0 | 1)
=> true,
_ => false,
},
(Some(n), None) => match (&op, n) {
(hir::BinOpKind::Div | hir::BinOpKind::Rem, local_n) if local_n != 0 => return,
// Adding or subtracting zeros is always a no-op
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
// Multiplication by 1 or 0 will never overflow
| (hir::BinOpKind::Mul, 0 | 1)
=> true,
_ => false,
=> return,
_ => {},
},
(Some(_), Some(_)) => {
matches!((lhs_ref_counter, rhs_ref_counter), (0, 0))
(Some(n), None)
if matches!(
(&op, n),
// Adding or subtracting zeros is always a no-op
(hir::BinOpKind::Add | hir::BinOpKind::Sub, 0)
// Multiplication by 1 or 0 will never overflow
| (hir::BinOpKind::Mul, 0 | 1)
) =>
{
return;
},
(Some(_), Some(_)) if matches!((lhs_ref_counter, rhs_ref_counter), (0, 0)) => return,
_ => {},
}
} else {
false
};
if !has_valid_op {
self.issue_lint(cx, expr);
}
self.issue_lint(cx, expr);
}

/// There are some integer methods like `wrapping_div` that will panic depending on the
Expand All @@ -285,15 +222,15 @@ impl ArithmeticSideEffects {
return;
}
let instance_ty = cx.typeck_results().expr_ty_adjusted(receiver);
if !Self::is_integral(instance_ty) {
if !is_integer(instance_ty) {
return;
}
self.manage_sugar_methods(cx, expr, receiver, ps, arg);
if !self.disallowed_int_methods.contains(&ps.ident.name) {
return;
}
let (actual_arg, _) = peel_hir_expr_refs(arg);
match Self::literal_integer(cx, actual_arg) {
match literal_integer(cx, actual_arg) {
None | Some(0) => self.issue_lint(cx, arg),
Some(_) => {},
}
Expand All @@ -317,7 +254,7 @@ impl ArithmeticSideEffects {
return;
}
let actual_un_expr = peel_hir_expr_refs(un_expr).0;
if Self::literal_integer(cx, actual_un_expr).is_some() {
if literal_integer(cx, actual_un_expr).is_some() {
return;
}
self.issue_lint(cx, expr);
Expand Down Expand Up @@ -385,3 +322,120 @@ impl<'tcx> LateLintPass<'tcx> for ArithmeticSideEffects {
}
}
}

/// Detects a type-casting conversion and returns the type of the original expression. For
/// example, `let foo = u64::from(bar)`.
fn find_original_primitive_ty<'tcx>(cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) -> Option<Ty<'tcx>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too broad

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

if let hir::ExprKind::Call(path, [arg]) = &expr.kind
&& path.res(cx).opt_def_id().is_diag_item(&cx.tcx, sym::from_fn)
{
Some(cx.typeck_results().expr_ty(arg))
} else {
None
}
}

/// Verifies built-in types that have specific allowed operations
fn has_specific_allowed_type_and_operation<'tcx>(
cx: &LateContext<'tcx>,
lhs_ty: Ty<'tcx>,
op: hir::BinOpKind,
rhs_ty: Ty<'tcx>,
) -> bool {
let is_div_or_rem = matches!(op, hir::BinOpKind::Div | hir::BinOpKind::Rem);
let is_sat_or_wrap = |ty: Ty<'_>| matches!(ty.opt_diag_name(cx), Some(sym::Saturating | sym::Wrapping));

// If the RHS is `NonZero<u*>`, then division or module by zero will never occur.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use proper doc comments /// to document functions (instead of simple code comments), so that the documentation can pop up in IDEs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my code -> a51fc2a

if is_non_zero_u(cx, rhs_ty) && is_div_or_rem {
return true;
}

// `Saturation` and `Wrapping` can overflow if the RHS is zero in a division or module.
if is_sat_or_wrap(lhs_ty) {
return !is_div_or_rem;
}

false
}

// For example, `i8` or `u128` and possible associated references like `&&u16`.
fn is_integer(ty: Ty<'_>) -> bool {
ty.peel_refs().is_integral()
}

fn is_non_zero_u(cx: &LateContext<'_>, ty: Ty<'_>) -> bool {
if let ty::Adt(adt, substs) = ty.kind()
&& cx.tcx.is_diagnostic_item(sym::NonZero, adt.did())
&& let int_type = substs.type_at(0)
&& matches!(int_type.kind(), ty::Uint(_))
{
true
} else {
false
}
}

/// If one side is a literal it is possible to evaluate overflows as long as the other side has a
/// smaller type. `0` and `1` suffixes indicate different sides.
///
/// For example, `1000u64 + u64::from(some_runtime_variable_of_type_u8)`.
fn is_safe_due_to_smaller_source_type(
cx: &LateContext<'_>,
op: hir::BinOpKind,
(expr0, ty0): (&hir::Expr<'_>, Ty<'_>),
expr1: &hir::Expr<'_>,
) -> bool {
let Some(num0) = literal_integer(cx, expr0) else {
return false;
};
let Some(orig_ty1) = find_original_primitive_ty(cx, expr1) else {
return false;
};
let Some(num1) = max_int_num(orig_ty1) else {
return false;
};
let Some(rslt) = (match op {
hir::BinOpKind::Add => num0.checked_add(num1),
hir::BinOpKind::Mul => num0.checked_mul(num1),
_ => None,
}) else {
return false;
};
match ty0.peel_refs().kind() {
ty::Uint(UintTy::U16) => u16::try_from(rslt).is_ok(),
ty::Uint(UintTy::U32) => u32::try_from(rslt).is_ok(),
ty::Uint(UintTy::U64) => u64::try_from(rslt).is_ok(),
ty::Uint(UintTy::U128) => true,
ty::Uint(UintTy::Usize) => usize::try_from(rslt).is_ok(),
_ => false,
}
}

/// Returns the numeric value of a literal integer originated from `expr`, if any.
///
/// Literal integers can be originated from adhoc declarations like `1`, associated constants
/// like `i32::MAX` or constant references like `N` from `const N: i32 = 1;`,
fn literal_integer(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> Option<u128> {
let actual = peel_hir_expr_unary(expr).0;
if let hir::ExprKind::Lit(lit) = actual.kind
&& let ast::LitKind::Int(n, _) = lit.node
{
return Some(n.get());
}
if let Some(Constant::Int(n)) = ConstEvalCtxt::new(cx).eval(expr) {
return Some(n);
}
None
}

fn max_int_num(ty: Ty<'_>) -> Option<u128> {
match ty.peel_refs().kind() {
ty::Uint(UintTy::U8) => Some(u8::MAX.into()),
ty::Uint(UintTy::U16) => Some(u16::MAX.into()),
ty::Uint(UintTy::U32) => Some(u32::MAX.into()),
ty::Uint(UintTy::U64) => Some(u64::MAX.into()),
ty::Uint(UintTy::U128) => Some(u128::MAX),
ty::Uint(UintTy::Usize) => usize::MAX.try_into().ok(),
_ => None,
}
}
56 changes: 56 additions & 0 deletions tests/ui/arithmetic_side_effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
extern crate proc_macro_derive;

use core::num::{NonZero, Saturating, Wrapping};
use core::time::Duration;

const ONE: i32 = 1;
const ZERO: i32 = 0;
Expand Down Expand Up @@ -687,4 +688,59 @@ pub fn explicit_methods() {
//~^ arithmetic_side_effects
}

pub fn issue_15943(days: u8) -> Duration {
Duration::from_secs(86400 * u64::from(days))
}

pub fn type_conversion_add() {
let _ = u128::MAX + u128::from(1u8);
//~^ arithmetic_side_effects
let _ = 1u128 + u128::from(1u16);
let _ = 1u128 + u128::from(1u32);
let _ = 1u128 + u128::from(1u64);

let _ = 1u64 + u64::from(1u8);
let _ = 1u64 + u64::from(1u16);
let _ = 1u64 + u64::from(1u32);

let _ = 1u32 + u32::from(1u8);
let _ = 1u32 + u32::from(1u16);

let _ = 1u16 + u16::from(1u8);
}

pub fn type_conversion_mul() {
let _ = u128::MAX * u128::from(1u8);
//~^ arithmetic_side_effects
let _ = 1u128 * u128::from(1u16);
let _ = 1u128 * u128::from(1u32);
let _ = 1u128 * u128::from(1u64);

let _ = 1u64 * u64::from(1u8);
let _ = 1u64 * u64::from(1u16);
let _ = 1u64 * u64::from(1u32);

let _ = 1u32 * u32::from(1u8);
let _ = 1u32 * u32::from(1u16);

let _ = 1u16 * u16::from(1u8);
}

pub fn type_conversion_does_not_escape_its_context() {
struct Foo;
impl Foo {
fn from(n: u8) -> u64 {
u64::from(n)
}
}
let _ = Duration::from_secs(86400 * Foo::from(1));
//~^ arithmetic_side_effects

fn shift(x: u8) -> u64 {
1 << u64::from(x)
}
let _ = Duration::from_secs(86400 * shift(1));
//~^ arithmetic_side_effects
}

fn main() {}
Loading