Fix manual is multiple of (#15205)

Fix several issues with `manual_is_multiple_of`

- `&a % &b == 0` compiles, but requires dereferencing `b` when replacing
with `a.is_multiple_of(b)`.
- In `a % b == 0`, if type of `a` is not certain, `a.is_multiple_of(b)`
might not be typable.
- In `a % b == 0`, `a` and `b` must be unsigned integers, not any
arbitrary types implementing `Rem` and outputing an integer.

Those fixes have required increasing the precision of type certainty
determination in the two first patches.

Fixes #15203
Fixes #15204

changelog: [`manual_is_multiple_of`]: fix various false positive

Author: Jarcho

clippy_lints/src/operators/manual_is_multiple_of.rs

@@ -2,11 +2,12 @@ use clippy_utils::consts::is_zero_integer_const;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::sugg::Sugg;
+use clippy_utils::ty::expr_type_is_certain;
 use rustc_ast::BinOpKind;
 use rustc_errors::Applicability;
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::LateContext;
-use rustc_middle::ty;
+use rustc_middle::ty::{self, Ty};
 
 use super::MANUAL_IS_MULTIPLE_OF;
 
@@ -22,9 +23,21 @@ pub(super) fn check<'tcx>(
         && let Some(operand) = uint_compare_to_zero(cx, op, lhs, rhs)
         && let ExprKind::Binary(operand_op, operand_left, operand_right) = operand.kind
         && operand_op.node == BinOpKind::Rem
+        && matches!(
+            cx.typeck_results().expr_ty_adjusted(operand_left).peel_refs().kind(),
+            ty::Uint(_)
+        )
+        && matches!(
+            cx.typeck_results().expr_ty_adjusted(operand_right).peel_refs().kind(),
+            ty::Uint(_)
+        )
+        && expr_type_is_certain(cx, operand_left)
     {
         let mut app = Applicability::MachineApplicable;
-        let divisor = Sugg::hir_with_applicability(cx, operand_right, "_", &mut app);
+        let divisor = deref_sugg(
+            Sugg::hir_with_applicability(cx, operand_right, "_", &mut app),
+            cx.typeck_results().expr_ty_adjusted(operand_right),
+        );
         span_lint_and_sugg(
             cx,
             MANUAL_IS_MULTIPLE_OF,
@@ -64,3 +77,11 @@ fn uint_compare_to_zero<'tcx>(
 
     matches!(cx.typeck_results().expr_ty_adjusted(operand).kind(), ty::Uint(_)).then_some(operand)
 }
+
+fn deref_sugg<'a>(sugg: Sugg<'a>, ty: Ty<'_>) -> Sugg<'a> {
+    if let ty::Ref(_, target_ty, _) = ty.kind() {
+        deref_sugg(sugg.deref(), *target_ty)
+    } else {
+        sugg
+    }
+}

clippy_utils/src/ty/type_certainty/mod.rs

@@ -12,10 +12,11 @@
 //! be considered a bug.
 
 use crate::paths::{PathNS, lookup_path};
+use rustc_ast::{LitFloatType, LitIntType, LitKind};
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
 use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt, walk_qpath, walk_ty};
-use rustc_hir::{self as hir, AmbigArg, Expr, ExprKind, GenericArgs, HirId, Node, PathSegment, QPath, TyKind};
+use rustc_hir::{self as hir, AmbigArg, Expr, ExprKind, GenericArgs, HirId, Node, Param, PathSegment, QPath, TyKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, AdtDef, GenericArgKind, Ty};
 use rustc_span::Span;
@@ -24,30 +25,32 @@ mod certainty;
 use certainty::{Certainty, Meet, join, meet};
 
 pub fn expr_type_is_certain(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-    expr_type_certainty(cx, expr).is_certain()
+    expr_type_certainty(cx, expr, false).is_certain()
 }
 
-fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty {
+/// Determine the type certainty of `expr`. `in_arg` indicates that the expression happens within
+/// the evaluation of a function or method call argument.
+fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>, in_arg: bool) -> Certainty {
     let certainty = match &expr.kind {
         ExprKind::Unary(_, expr)
         | ExprKind::Field(expr, _)
         | ExprKind::Index(expr, _, _)
-        | ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr),
+        | ExprKind::AddrOf(_, _, expr) => expr_type_certainty(cx, expr, in_arg),
 
-        ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr))),
+        ExprKind::Array(exprs) => join(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))),
 
         ExprKind::Call(callee, args) => {
-            let lhs = expr_type_certainty(cx, callee);
+            let lhs = expr_type_certainty(cx, callee, false);
             let rhs = if type_is_inferable_from_arguments(cx, expr) {
-                meet(args.iter().map(|arg| expr_type_certainty(cx, arg)))
+                meet(args.iter().map(|arg| expr_type_certainty(cx, arg, true)))
             } else {
                 Certainty::Uncertain
             };
             lhs.join_clearing_def_ids(rhs)
         },
 
         ExprKind::MethodCall(method, receiver, args, _) => {
-            let mut receiver_type_certainty = expr_type_certainty(cx, receiver);
+            let mut receiver_type_certainty = expr_type_certainty(cx, receiver, false);
             // Even if `receiver_type_certainty` is `Certain(Some(..))`, the `Self` type in the method
             // identified by `type_dependent_def_id(..)` can differ. This can happen as a result of a `deref`,
             // for example. So update the `DefId` in `receiver_type_certainty` (if any).
@@ -59,24 +62,48 @@ fn expr_type_certainty(cx: &LateContext<'_>, expr: &Expr<'_>) -> Certainty {
             let lhs = path_segment_certainty(cx, receiver_type_certainty, method, false);
             let rhs = if type_is_inferable_from_arguments(cx, expr) {
                 meet(
-                    std::iter::once(receiver_type_certainty).chain(args.iter().map(|arg| expr_type_certainty(cx, arg))),
+                    std::iter::once(receiver_type_certainty)
+                        .chain(args.iter().map(|arg| expr_type_certainty(cx, arg, true))),
                 )
             } else {
                 Certainty::Uncertain
             };
             lhs.join(rhs)
         },
 
-        ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr))),
+        ExprKind::Tup(exprs) => meet(exprs.iter().map(|expr| expr_type_certainty(cx, expr, in_arg))),
 
-        ExprKind::Binary(_, lhs, rhs) => expr_type_certainty(cx, lhs).meet(expr_type_certainty(cx, rhs)),
+        ExprKind::Binary(_, lhs, rhs) => {
+            // If one of the side of the expression is uncertain, the certainty will come from the other side,
+            // with no information on the type.
+            match (
+                expr_type_certainty(cx, lhs, in_arg),
+                expr_type_certainty(cx, rhs, in_arg),
+            ) {
+                (Certainty::Uncertain, Certainty::Certain(_)) | (Certainty::Certain(_), Certainty::Uncertain) => {
+                    Certainty::Certain(None)
+                },
+                (l, r) => l.meet(r),
+            }
+        },
 
-        ExprKind::Lit(_) => Certainty::Certain(None),
+        ExprKind::Lit(lit) => {
+            if !in_arg
+                && matches!(
+                    lit.node,
+                    LitKind::Int(_, LitIntType::Unsuffixed) | LitKind::Float(_, LitFloatType::Unsuffixed)
+                )
+            {
+                Certainty::Uncertain
+            } else {
+                Certainty::Certain(None)
+            }
+        },
 
         ExprKind::Cast(_, ty) => type_certainty(cx, ty),
 
         ExprKind::If(_, if_expr, Some(else_expr)) => {
-            expr_type_certainty(cx, if_expr).join(expr_type_certainty(cx, else_expr))
+            expr_type_certainty(cx, if_expr, in_arg).join(expr_type_certainty(cx, else_expr, in_arg))
         },
 
         ExprKind::Path(qpath) => qpath_certainty(cx, qpath, false),
@@ -188,6 +215,20 @@ fn qpath_certainty(cx: &LateContext<'_>, qpath: &QPath<'_>, resolves_to_type: bo
     certainty
 }
 
+/// Tries to tell whether `param` resolves to something certain, e.g., a non-wildcard type if
+/// present. The certainty `DefId` is cleared before returning.
+fn param_certainty(cx: &LateContext<'_>, param: &Param<'_>) -> Certainty {
+    let owner_did = cx.tcx.hir_enclosing_body_owner(param.hir_id);
+    let Some(fn_decl) = cx.tcx.hir_fn_decl_by_hir_id(cx.tcx.local_def_id_to_hir_id(owner_did)) else {
+        return Certainty::Uncertain;
+    };
+    let inputs = fn_decl.inputs;
+    let body_params = cx.tcx.hir_body_owned_by(owner_did).params;
+    std::iter::zip(body_params, inputs)
+        .find(|(p, _)| p.hir_id == param.hir_id)
+        .map_or(Certainty::Uncertain, |(_, ty)| type_certainty(cx, ty).clear_def_id())
+}
+
 fn path_segment_certainty(
     cx: &LateContext<'_>,
     parent_certainty: Certainty,
@@ -240,15 +281,16 @@ fn path_segment_certainty(
 
         // `get_parent` because `hir_id` refers to a `Pat`, and we're interested in the node containing the `Pat`.
         Res::Local(hir_id) => match cx.tcx.parent_hir_node(hir_id) {
-            // An argument's type is always certain.
-            Node::Param(..) => Certainty::Certain(None),
+            // A parameter's type is not always certain, as it may come from an untyped closure definition,
+            // or from a wildcard in a typed closure definition.
+            Node::Param(param) => param_certainty(cx, param),
             // A local's type is certain if its type annotation is certain or it has an initializer whose
             // type is certain.
             Node::LetStmt(local) => {
                 let lhs = local.ty.map_or(Certainty::Uncertain, |ty| type_certainty(cx, ty));
                 let rhs = local
                     .init
-                    .map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init));
+                    .map_or(Certainty::Uncertain, |init| expr_type_certainty(cx, init, false));
                 let certainty = lhs.join(rhs);
                 if resolves_to_type {
                     certainty

tests/ui/manual_is_multiple_of.fixed

@@ -23,3 +23,81 @@ fn f(a: u64, b: u64) {
 fn g(a: u64, b: u64) {
     let _ = a % b == 0;
 }
+
+fn needs_deref(a: &u64, b: &u64) {
+    let _ = a.is_multiple_of(*b); //~ manual_is_multiple_of
+}
+
+fn closures(a: u64, b: u64) {
+    // Do not lint, types are ambiguous at this point
+    let cl = |a, b| a % b == 0;
+    let _ = cl(a, b);
+
+    // Do not lint, types are ambiguous at this point
+    let cl = |a: _, b: _| a % b == 0;
+    let _ = cl(a, b);
+
+    // Type of `a` is enough
+    let cl = |a: u64, b| a.is_multiple_of(b); //~ manual_is_multiple_of
+    let _ = cl(a, b);
+
+    // Type of `a` is enough
+    let cl = |a: &u64, b| a.is_multiple_of(b); //~ manual_is_multiple_of
+    let _ = cl(&a, b);
+
+    // Type of `b` is not enough
+    let cl = |a, b: u64| a % b == 0;
+    let _ = cl(&a, b);
+}
+
+fn any_rem<T: std::ops::Rem<Output = u32>>(a: T, b: T) {
+    // An arbitrary `Rem` implementation should not lint
+    let _ = a % b == 0;
+}
+
+mod issue15103 {
+    fn foo() -> Option<u64> {
+        let mut n: u64 = 150_000_000;
+
+        (2..).find(|p| {
+            while n.is_multiple_of(*p) {
+                //~^ manual_is_multiple_of
+                n /= p;
+            }
+            n <= 1
+        })
+    }
+
+    const fn generate_primes<const N: usize>() -> [u64; N] {
+        let mut result = [0; N];
+        if N == 0 {
+            return result;
+        }
+        result[0] = 2;
+        if N == 1 {
+            return result;
+        }
+        let mut idx = 1;
+        let mut p = 3;
+        while idx < N {
+            let mut j = 0;
+            while j < idx && p % result[j] != 0 {
+                j += 1;
+            }
+            if j == idx {
+                result[idx] = p;
+                idx += 1;
+            }
+            p += 1;
+        }
+        result
+    }
+
+    fn bar() -> u32 {
+        let d = |n: u32| -> u32 { (1..=n / 2).filter(|i| n.is_multiple_of(*i)).sum() };
+        //~^ manual_is_multiple_of
+
+        let d = |n| (1..=n / 2).filter(|i| n % i == 0).sum();
+        (1..1_000).filter(|&i| i == d(d(i)) && i != d(i)).sum()
+    }
+}

tests/ui/manual_is_multiple_of.rs

@@ -23,3 +23,81 @@ fn f(a: u64, b: u64) {
 fn g(a: u64, b: u64) {
     let _ = a % b == 0;
 }
+
+fn needs_deref(a: &u64, b: &u64) {
+    let _ = a % b == 0; //~ manual_is_multiple_of
+}
+
+fn closures(a: u64, b: u64) {
+    // Do not lint, types are ambiguous at this point
+    let cl = |a, b| a % b == 0;
+    let _ = cl(a, b);
+
+    // Do not lint, types are ambiguous at this point
+    let cl = |a: _, b: _| a % b == 0;
+    let _ = cl(a, b);
+
+    // Type of `a` is enough
+    let cl = |a: u64, b| a % b == 0; //~ manual_is_multiple_of
+    let _ = cl(a, b);
+
+    // Type of `a` is enough
+    let cl = |a: &u64, b| a % b == 0; //~ manual_is_multiple_of
+    let _ = cl(&a, b);
+
+    // Type of `b` is not enough
+    let cl = |a, b: u64| a % b == 0;
+    let _ = cl(&a, b);
+}
+
+fn any_rem<T: std::ops::Rem<Output = u32>>(a: T, b: T) {
+    // An arbitrary `Rem` implementation should not lint
+    let _ = a % b == 0;
+}
+
+mod issue15103 {
+    fn foo() -> Option<u64> {
+        let mut n: u64 = 150_000_000;
+
+        (2..).find(|p| {
+            while n % p == 0 {
+                //~^ manual_is_multiple_of
+                n /= p;
+            }
+            n <= 1
+        })
+    }
+
+    const fn generate_primes<const N: usize>() -> [u64; N] {
+        let mut result = [0; N];
+        if N == 0 {
+            return result;
+        }
+        result[0] = 2;
+        if N == 1 {
+            return result;
+        }
+        let mut idx = 1;
+        let mut p = 3;
+        while idx < N {
+            let mut j = 0;
+            while j < idx && p % result[j] != 0 {
+                j += 1;
+            }
+            if j == idx {
+                result[idx] = p;
+                idx += 1;
+            }
+            p += 1;
+        }
+        result
+    }
+
+    fn bar() -> u32 {
+        let d = |n: u32| -> u32 { (1..=n / 2).filter(|i| n % i == 0).sum() };
+        //~^ manual_is_multiple_of
+
+        let d = |n| (1..=n / 2).filter(|i| n % i == 0).sum();
+        (1..1_000).filter(|&i| i == d(d(i)) && i != d(i)).sum()
+    }
+}

tests/ui/manual_is_multiple_of.stderr

@@ -37,5 +37,35 @@ error: manual implementation of `.is_multiple_of()`
 LL |     let _ = 0 < a % b;
    |             ^^^^^^^^^ help: replace with: `!a.is_multiple_of(b)`
 
-error: aborting due to 6 previous errors
+error: manual implementation of `.is_multiple_of()`
+  --> tests/ui/manual_is_multiple_of.rs:28:13
+   |
+LL |     let _ = a % b == 0;
+   |             ^^^^^^^^^^ help: replace with: `a.is_multiple_of(*b)`
+
+error: manual implementation of `.is_multiple_of()`
+  --> tests/ui/manual_is_multiple_of.rs:41:26
+   |
+LL |     let cl = |a: u64, b| a % b == 0;
+   |                          ^^^^^^^^^^ help: replace with: `a.is_multiple_of(b)`
+
+error: manual implementation of `.is_multiple_of()`
+  --> tests/ui/manual_is_multiple_of.rs:45:27
+   |
+LL |     let cl = |a: &u64, b| a % b == 0;
+   |                           ^^^^^^^^^^ help: replace with: `a.is_multiple_of(b)`
+
+error: manual implementation of `.is_multiple_of()`
+  --> tests/ui/manual_is_multiple_of.rs:63:19
+   |
+LL |             while n % p == 0 {
+   |                   ^^^^^^^^^^ help: replace with: `n.is_multiple_of(*p)`
+
+error: manual implementation of `.is_multiple_of()`
+  --> tests/ui/manual_is_multiple_of.rs:97:58
+   |
+LL |         let d = |n: u32| -> u32 { (1..=n / 2).filter(|i| n % i == 0).sum() };
+   |                                                          ^^^^^^^^^^ help: replace with: `n.is_multiple_of(*i)`
+
+error: aborting due to 11 previous errors