Add unchecked_non_zero lint

CHANGELOG.md

@@ -7533,6 +7533,7 @@ Released 2018-09-13
 [`type_repetition_in_bounds`]: https://rust-lang.github.io/rust-clippy/master/index.html#type_repetition_in_bounds
 [`unbuffered_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#unbuffered_bytes
 [`unchecked_duration_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_duration_subtraction
+[`unchecked_non_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_non_zero
 [`unchecked_time_subtraction`]: https://rust-lang.github.io/rust-clippy/master/index.html#unchecked_time_subtraction
 [`unconditional_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#unconditional_recursion
 [`undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks

clippy_lints/src/declared_lints.rs

@@ -497,6 +497,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::SWAP_WITH_TEMPORARY_INFO,
     crate::methods::TYPE_ID_ON_BOX_INFO,
     crate::methods::UNBUFFERED_BYTES_INFO,
+    crate::methods::UNCHECKED_NON_ZERO_INFO,
     crate::methods::UNINIT_ASSUMED_INIT_INFO,
     crate::methods::UNIT_HASH_INFO,
     crate::methods::UNNECESSARY_FALLIBLE_CONVERSIONS_INFO,

clippy_lints/src/methods/mod.rs

@@ -127,6 +127,7 @@ mod suspicious_to_owned;
 mod swap_with_temporary;
 mod type_id_on_box;
 mod unbuffered_bytes;
+mod unchecked_non_zero;
 mod uninit_assumed_init;
 mod unit_hash;
 mod unnecessary_fallible_conversions;
@@ -4089,6 +4090,60 @@ declare_clippy_lint! {
     "calling .bytes() is very inefficient when data is not in memory"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for calls to standard library methods that panic when a value is zero, where
+    /// that value could be zero as far as Clippy can tell. Covered methods are:
+    ///
+    /// - `chunks`, `chunks_exact`, `rchunks`, `rchunks_exact`, `windows` and their `_mut`
+    ///   variants, which panic on a chunk or window size of `0`
+    /// - `Iterator::step_by`, which panics on a step of `0`
+    /// - `ilog2`, `ilog10` and `ilog`, which panic on a receiver of `0` (or, for signed
+    ///   integers, on a negative receiver). `ilog` also panics on a base below `2`.
+    ///
+    /// ### Why restrict this?
+    /// Nothing in these signatures rules out the value that panics, so it is easy to
+    /// overlook. When the value comes from a computation, user input or a configuration
+    /// file, a zero can reach the call and take the whole program down far away from where
+    /// the value was produced.
+    ///
+    /// Accepting a [`NonZero<usize>`](std::num::NonZero) moves the check to the boundary
+    /// where the value enters the program, so the call site cannot panic at all. For the
+    /// `ilog` family, `checked_ilog2` and friends return `None` rather than panicking.
+    ///
+    /// ### Known problems
+    /// Clippy only accepts a value as safe when it is a constant, `NonZero::get()` on an
+    /// unsigned `NonZero`, or `max(n)` for a large enough constant `n`. A value ruled out as
+    /// zero in some other way, such as by an earlier `assert!` or an enclosing `if`, still
+    /// triggers this lint.
+    ///
+    /// `step_by` with a literal `0` is left to `iterator_step_by_zero`, which is
+    /// warn-by-default, so the two lints do not both fire on it.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// fn print_rows(data: &[u8], row_len: usize) {
+    ///     for row in data.chunks(row_len) {
+    ///         println!("{row:?}");
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// use std::num::NonZero;
+    ///
+    /// fn print_rows(data: &[u8], row_len: NonZero<usize>) {
+    ///     for row in data.chunks(row_len.get()) {
+    ///         println!("{row:?}");
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.99.0"]
+    pub UNCHECKED_NON_ZERO,
+    restriction,
+    "calling a method that panics on a zero value, where the value could be zero"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for `MaybeUninit::uninit().assume_init()`.
@@ -5054,6 +5109,7 @@ impl_lint_pass!(Methods => [
     SWAP_WITH_TEMPORARY,
     TYPE_ID_ON_BOX,
     UNBUFFERED_BYTES,
+    UNCHECKED_NON_ZERO,
     UNINIT_ASSUMED_INIT,
     UNIT_HASH,
     UNNECESSARY_FALLIBLE_CONVERSIONS,
@@ -5267,6 +5323,9 @@ impl Methods {
     fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         // Handle method calls whose receiver and arguments may not come from expansion
         if let Some((name, recv, args, span, call_span)) = method_call(expr) {
+            // Spans several unrelated method families, so it does its own dispatch on `name`.
+            unchecked_non_zero::check(cx, expr, recv, args, call_span, name);
+
             match (name, args) {
                 (sym::add | sym::sub | sym::wrapping_add | sym::wrapping_sub, [_arg]) => {
                     zst_offset::check(cx, expr, recv);

clippy_lints/src/methods/unchecked_non_zero.rs

@@ -0,0 +1,214 @@
+use clippy_utils::consts::{ConstEvalCtxt, FullInt};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::{MaybeDef as _, MaybeTypeckRes as _};
+use clippy_utils::{is_from_proc_macro, sym};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_middle::ty::layout::LayoutOf as _;
+use rustc_span::{Span, Symbol};
+
+use super::UNCHECKED_NON_ZERO;
+
+/// A value that makes a call panic when it is below `min`.
+struct Precondition<'tcx> {
+    /// The expression producing the value.
+    value: &'tcx Expr<'tcx>,
+    /// Names the value, e.g. `"the chunk size"`.
+    what: &'static str,
+    /// Describes a violating value, e.g. `"zero"`.
+    bad: &'static str,
+    min: u32,
+    help: String,
+}
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    args: &'tcx [Expr<'tcx>],
+    call_span: Span,
+    method_name: Symbol,
+) {
+    let non_zero_arg = |value, what| Precondition {
+        value,
+        what,
+        bad: "zero",
+        min: 1,
+        help: format!("consider taking {what} as a `NonZero<usize>`, or checking it before this call"),
+    };
+
+    // This runs for every method call in the crate, so match on the name before looking anything
+    // up. `recv_ty` is only needed once an arm has matched.
+    match (method_name, args) {
+        // `chunk size must be non-zero` / `window size must be non-zero`. These names are also used
+        // by iterator adapters and third-party traits, so check that this is the inherent slice
+        // method. Autoderef means `Vec`, arrays and `Box<[T]>` all land here too.
+        (
+            sym::chunks
+            | sym::chunks_mut
+            | sym::chunks_exact
+            | sym::chunks_exact_mut
+            | sym::rchunks
+            | sym::rchunks_mut
+            | sym::rchunks_exact
+            | sym::rchunks_exact_mut
+            | sym::windows,
+            [arg],
+        ) => {
+            let recv_ty = cx.typeck_results().expr_ty_adjusted(recv);
+            if expr.span.from_expansion() || !matches!(recv_ty.kind(), ty::Ref(_, inner, _) if inner.is_slice()) {
+                return;
+            }
+            let what = if method_name == sym::windows {
+                "the window size"
+            } else {
+                "the chunk size"
+            };
+            emit(cx, expr, call_span, method_name, &non_zero_arg(arg, what), true);
+        },
+
+        // `Iterator::step_by` asserts `step != 0`.
+        (sym::step_by, [arg]) => {
+            if expr.span.from_expansion() || !cx.ty_based_def(expr).opt_parent(cx).is_diag_item(cx, sym::Iterator) {
+                return;
+            }
+            // A literal `0` is already covered by `iterator_step_by_zero`, which is warn-by-default.
+            emit(cx, expr, call_span, method_name, &non_zero_arg(arg, "the step"), false);
+        },
+
+        // `ilog2`/`ilog10`/`ilog` panic on a receiver of zero, or, when signed, on a negative one.
+        // `NonZero` has its own infallible `ilog2`/`ilog10`, and is an ADT rather than `is_integral`.
+        (sym::ilog | sym::ilog2 | sym::ilog10, _) => {
+            let recv_ty = cx.typeck_results().expr_ty_adjusted(recv);
+            if expr.span.from_expansion() || !recv_ty.is_integral() {
+                return;
+            }
+            let checked = format!("consider using `checked_{method_name}`, which returns `None` instead of panicking");
+            let receiver_is_valid = Precondition {
+                value: recv,
+                what: "the value",
+                bad: if recv_ty.is_signed() {
+                    "zero or negative"
+                } else {
+                    "zero"
+                },
+                min: 1,
+                help: checked.clone(),
+            };
+            if emit(cx, expr, call_span, method_name, &receiver_is_valid, true) {
+                return;
+            }
+
+            // `ilog` additionally panics when the base is less than 2.
+            if let [base] = args {
+                let base_is_valid = Precondition {
+                    value: base,
+                    what: "the base",
+                    bad: "less than `2`",
+                    min: 2,
+                    help: checked,
+                };
+                emit(cx, expr, call_span, method_name, &base_is_valid, true);
+            }
+        },
+
+        _ => {},
+    }
+}
+
+/// Emits the lint when `precondition` is violated or unproven. Returns whether it was emitted.
+///
+/// `report_known_violations` reports values proven to panic; pass `false` where another lint
+/// already owns that case.
+fn emit<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    call_span: Span,
+    method_name: Symbol,
+    precondition: &Precondition<'tcx>,
+    report_known_violations: bool,
+) -> bool {
+    let &Precondition {
+        value,
+        what,
+        bad,
+        min,
+        ref help,
+    } = precondition;
+
+    let holds = known_at_least(cx, value, min);
+    match holds {
+        Some(true) => return false,
+        Some(false) if !report_known_violations => return false,
+        _ => {},
+    }
+
+    if is_from_proc_macro(cx, expr) {
+        return false;
+    }
+
+    let msg = if holds == Some(false) {
+        format!("`{method_name}` will panic, as {what} is {bad}")
+    } else {
+        format!("`{method_name}` will panic if {what} is {bad}")
+    };
+
+    span_lint_and_then(cx, UNCHECKED_NON_ZERO, call_span, msg, |diag| {
+        if holds.is_none() {
+            diag.span_note(value.span, format!("this may be {bad}"));
+            diag.help(help.clone());
+        }
+    });
+    true
+}
+
+/// Whether `e` is known to evaluate to at least `min`.
+///
+/// `Some(true)` when proven, `Some(false)` when proven violated, `None` when either is possible.
+fn known_at_least<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>, min: u32) -> Option<bool> {
+    if let Some(int) = ConstEvalCtxt::new(cx)
+        .eval(e)
+        .and_then(|c| c.int_value(cx.tcx, cx.typeck_results().expr_ty(e)))
+    {
+        return Some(match int {
+            FullInt::S(v) => v >= i128::from(min),
+            FullInt::U(v) => v >= u128::from(min),
+        });
+    }
+
+    match e.kind {
+        // `n.get()` on an unsigned `NonZero` is at least 1. A signed one may still be negative.
+        ExprKind::MethodCall(name, recv, [], _) if name.ident.name == sym::get && min <= 1 => {
+            let ty = cx.typeck_results().expr_ty_adjusted(recv).peel_refs();
+            (ty.is_diag_item(cx, sym::NonZero) && matches!(ty.kind(), ty::Adt(_, args) if !args.type_at(0).is_signed()))
+                .then_some(true)
+        },
+        // `n.max(k)` is at least `k`, the usual way of guarding these calls by hand.
+        ExprKind::MethodCall(name, _, [other], _) if name.ident.name == sym::max => {
+            (known_at_least(cx, other, min) == Some(true)).then_some(true)
+        },
+        // `size_of::<T>()` is the size of `T` in bytes, which is known whenever `T`'s layout is.
+        // A zero-sized `T` makes this a proven violation rather than an unproven one.
+        ExprKind::Call(func, []) => {
+            if let ExprKind::Path(ref qpath) = func.kind
+                && let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
+                && cx.tcx.is_diagnostic_item(sym::mem_size_of, def_id)
+                && let Some(ty) = cx.typeck_results().node_args(func.hir_id).types().next()
+                && let Ok(layout) = cx.layout_of(ty)
+            {
+                Some(layout.size.bytes() >= u64::from(min))
+            } else {
+                None
+            }
+        },
+        // On an unsigned type a sum is at least as large as either operand, so one large enough
+        // side is proof. This does not hold for signed types, where the other side may be negative.
+        ExprKind::Binary(op, lhs, rhs)
+            if op.node == BinOpKind::Add && matches!(cx.typeck_results().expr_ty(e).kind(), ty::Uint(_)) =>
+        {
+            (known_at_least(cx, lhs, min) == Some(true) || known_at_least(cx, rhs, min) == Some(true)).then_some(true)
+        },
+        _ => None,
+    }
+}

clippy_utils/src/sym.rs

@@ -177,8 +177,10 @@ generate! {
     checked_sub,
     child_id,
     child_kill,
+    chunks,
     chunks_exact,
     chunks_exact_mut,
+    chunks_mut,
     clamp,
     clippy_utils,
     clone_into,
@@ -353,6 +355,8 @@ generate! {
     i8_legacy_fn_min_value,
     i8_legacy_mod,
     ilog,
+    ilog10,
+    ilog2,
     include_bytes_macro,
     include_str_macro,
     insert,
@@ -497,6 +501,10 @@ generate! {
     push_front,
     push_str,
     range_step,
+    rchunks,
+    rchunks_exact,
+    rchunks_exact_mut,
+    rchunks_mut,
     read,
     read_exact,
     read_line,