new lint: missing_assert_for_indexing

y21 · y21 · commit 08718aea122c · 2023-04-29T18:21:10.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4829,6 +4829,7 @@ Released 2018-09-13
 [`mismatching_type_param_order`]: https://rust-lang.github.io/rust-clippy/master/index.html#mismatching_type_param_order
 [`misnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#misnamed_getters
 [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op
+[`missing_assert_for_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_assert_for_indexing
 [`missing_assert_message`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_assert_message
 [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn
 [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -425,6 +425,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::misc_early::UNSEPARATED_LITERAL_SUFFIX_INFO,
     crate::misc_early::ZERO_PREFIXED_LITERAL_INFO,
     crate::mismatching_type_param_order::MISMATCHING_TYPE_PARAM_ORDER_INFO,
+    crate::missing_assert_for_indexing::MISSING_ASSERT_FOR_INDEXING_INFO,
     crate::missing_assert_message::MISSING_ASSERT_MESSAGE_INFO,
     crate::missing_const_for_fn::MISSING_CONST_FOR_FN_INFO,
     crate::missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -198,6 +198,7 @@ mod minmax;
 mod misc;
 mod misc_early;
 mod mismatching_type_param_order;
+mod missing_assert_for_indexing;
 mod missing_assert_message;
 mod missing_const_for_fn;
 mod missing_doc;
@@ -970,6 +971,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(manual_slice_size_calculation::ManualSliceSizeCalculation));
     store.register_early_pass(|| Box::new(suspicious_doc_comments::SuspiciousDocComments));
     store.register_late_pass(|_| Box::new(items_after_test_module::ItemsAfterTestModule));
+    store.register_late_pass(|_| Box::new(missing_assert_for_indexing::MissingAssertsForIndexing));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/missing_assert_for_indexing.rs b/clippy_lints/src/missing_assert_for_indexing.rs
@@ -0,0 +1,356 @@
+use std::ops::ControlFlow;
+use std::{collections::hash_map::Entry, mem};
+
+use clippy_utils::source::snippet;
+use clippy_utils::{
+    comparisons::{normalize_comparison, Rel},
+    diagnostics::span_lint_and_then,
+    hash_expr, higher,
+    visitors::for_each_expr,
+};
+use rustc_ast::{LitKind, RangeLimits};
+use rustc_data_structures::fx::FxHashMap;
+use rustc_errors::{Applicability, Diagnostic};
+use rustc_hir::{BinOp, Block, Expr, ExprKind, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::source_map::Spanned;
+use rustc_span::{sym, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for repeated slice indexing without asserting beforehand that the length
+    /// is greater than the largest index used to index into the slice.
+    ///
+    /// ### Why is this bad?
+    /// In the general case where the compiler does not have a lot of information
+    /// about the length of a slice, indexing it repeatedly will generate a bounds check
+    /// for every single index.
+    ///
+    /// Asserting that the length of the slice is at least as large as the largest value
+    /// to index beforehand gives the compiler enough information to elide the bounds checks,
+    /// effectively reducing the number of bounds checks from however many times
+    /// the slice was indexed to just one (the assert).
+    ///
+    /// ### Drawbacks
+    /// False positives. It is, in general, very difficult to predict how well
+    /// the optimizer will be able to elide bounds checks and it very much depends on
+    /// the surrounding code. For example, indexing into the slice yielded by the
+    /// [`slice::chunks_exact`](https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunks_exact)
+    /// iterator will likely have all of the bounds checks elided even without an assert
+    /// if the `chunk_size` is a constant.
+    ///
+    /// Asserts are not tracked across function calls. Asserting the length of a slice
+    /// in a different function likely gives the optimizer enough information
+    /// about the length of a slice, but this lint will not detect that.
+    ///
+    /// ### Example
+    /// ```rust
+    /// fn sum(v: &[u8]) -> u8 {
+    ///     // 4 bounds checks
+    ///     v[0] + v[1] + v[2] + v[3]
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn sum(v: &[u8]) -> u8 {
+    ///     assert!(v.len() > 4);
+    ///     // no bounds checks
+    ///     v[0] + v[1] + v[2] + v[3]
+    /// }
+    /// ```
+    #[clippy::version = "1.70.0"]
+    pub MISSING_ASSERT_FOR_INDEXING,
+    nursery,
+    "indexing into a slice multiple times without an `assert`"
+}
+declare_lint_pass!(MissingAssertsForIndexing => [MISSING_ASSERT_FOR_INDEXING]);
+
+fn report_lint<F>(cx: &LateContext<'_>, full_span: Span, msg: &str, indexes: &[Span], f: F)
+where
+    F: FnOnce(&mut Diagnostic),
+{
+    span_lint_and_then(cx, MISSING_ASSERT_FOR_INDEXING, full_span, msg, |diag| {
+        f(diag);
+        for span in indexes {
+            diag.span_note(*span, "slice indexed here");
+        }
+        diag.note("asserting the length before indexing will elide bounds checks");
+    });
+}
+
+#[derive(Copy, Clone, Debug)]
+enum LengthComparison {
+    /// `v.len() < 5`
+    LengthLessThanInt,
+    /// `5 < v.len()`
+    IntLessThanLength,
+    /// `v.len() <= 5`
+    LengthLessThanOrEqualInt,
+    /// `5 <= v.len()`
+    IntLessThanOrEqualLength,
+}
+
+fn len_comparison<'hir>(
+    bin_op: BinOp,
+    left: &'hir Expr<'hir>,
+    right: &'hir Expr<'hir>,
+) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> {
+    macro_rules! int_lit_pat {
+        ($id:ident) => {
+            ExprKind::Lit(Spanned {
+                node: LitKind::Int($id, _),
+                ..
+            })
+        };
+    }
+
+    // normalize comparison, `v.len() > 4` becomes `4 <= v.len()`
+    // this simplifies the logic a bit
+    let (op, left, right) = normalize_comparison(bin_op.node, left, right)?;
+    match (op, &left.kind, &right.kind) {
+        (Rel::Lt, int_lit_pat!(left), _) => Some((LengthComparison::IntLessThanLength, *left as usize, right)),
+        (Rel::Lt, _, int_lit_pat!(right)) => Some((LengthComparison::LengthLessThanInt, *right as usize, left)),
+        (Rel::Le, int_lit_pat!(left), _) => Some((LengthComparison::IntLessThanOrEqualLength, *left as usize, right)),
+        (Rel::Le, _, int_lit_pat!(right)) => Some((LengthComparison::LengthLessThanOrEqualInt, *right as usize, left)),
+        _ => None,
+    }
+}
+
+fn assert_len_expr<'hir>(
+    cx: &LateContext<'_>,
+    expr: &'hir Expr<'hir>,
+) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> {
+    if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr)
+        && let ExprKind::Unary(UnOp::Not, condition) = &cond.kind
+        && let ExprKind::Binary(bin_op, left, right) = &condition.kind
+
+        && let Some((cmp, asserted_len, slice_len)) = len_comparison(*bin_op, left, right)
+        && let ExprKind::MethodCall(method, recv, ..) = &slice_len.kind
+        && cx.typeck_results().expr_ty(recv).peel_refs().is_slice()
+        && method.ident.name == sym::len
+
+        // check if `then` block has a never type expression
+        && let ExprKind::Block(Block { expr: Some(then_expr), .. }, _) = then.kind
+        && cx.typeck_results().expr_ty(then_expr).is_never()
+    {
+        Some((cmp, asserted_len, recv))
+    } else {
+        None
+    }
+}
+
+#[derive(Debug)]
+enum IndexEntry {
+    StrayAssert {
+        asserted_len: usize,
+        cmp: LengthComparison,
+        assert_span: Span,
+        slice_span: Span,
+    },
+    AssertWithIndex {
+        highest_index: usize,
+        asserted_len: usize,
+        assert_span: Span,
+        slice_span: Span,
+        indexes: Vec<Span>,
+        cmp: LengthComparison,
+    },
+    AssertWithoutIndex {
+        highest_index: usize,
+        indexes: Vec<Span>,
+        slice_span: Span,
+    },
+}
+
+impl IndexEntry {
+    pub fn spans(&self) -> Option<&[Span]> {
+        match self {
+            IndexEntry::StrayAssert { .. } => None,
+            IndexEntry::AssertWithIndex { indexes, .. } | IndexEntry::AssertWithoutIndex { indexes, .. } => {
+                Some(indexes)
+            },
+        }
+    }
+}
+
+fn upper_index_expr(expr: &Expr<'_>) -> Option<usize> {
+    if let ExprKind::Lit(lit) = &expr.kind && let LitKind::Int(index, _) = lit.node {
+        Some(index as usize)
+    } else if let Some(higher::Range { end: Some(end), limits, .. }) = higher::Range::hir(expr)
+        && let ExprKind::Lit(lit) = &end.kind
+        && let LitKind::Int(index @ 1.., _) = lit.node
+    {
+        match limits {
+            RangeLimits::HalfOpen => Some(index as usize - 1),
+            RangeLimits::Closed => Some(index as usize),
+        }
+    } else {
+        None
+    }
+}
+
+fn check_index(cx: &LateContext<'_>, expr: &Expr<'_>, indexes: &mut FxHashMap<u64, IndexEntry>) {
+    if let ExprKind::Index(target, index_lit) = expr.kind
+        && cx.typeck_results().expr_ty(target).peel_refs().is_slice()
+        && let Some(index) = upper_index_expr(index_lit)
+    {
+        let hash = hash_expr(cx, target);
+        let entry = indexes.entry(hash)
+            .or_insert_with(|| IndexEntry::AssertWithoutIndex {
+                highest_index: index,
+                slice_span: target.span,
+                indexes: Vec::new(),
+            });
+
+        match entry {
+            IndexEntry::StrayAssert { asserted_len, cmp, assert_span, slice_span: receiver_span } => {
+                let asserted_len = *asserted_len;
+                let cmp = *cmp;
+                let assert_span = *assert_span;
+                let receiver_span = *receiver_span;
+                *entry = IndexEntry::AssertWithIndex {
+                    highest_index: index,
+                    asserted_len,
+                    indexes: vec![expr.span],
+                    assert_span,
+                    cmp,
+                    slice_span: receiver_span
+                };
+            }
+            IndexEntry::AssertWithoutIndex { highest_index, indexes, .. }
+            | IndexEntry::AssertWithIndex { highest_index, indexes, .. } => {
+                indexes.push(expr.span);
+                *highest_index = (*highest_index).max(index);
+            }
+        }
+    }
+}
+
+fn check_assert(cx: &LateContext<'_>, expr: &Expr<'_>, indexes: &mut FxHashMap<u64, IndexEntry>) {
+    if let Some((cmp, asserted_len, slice)) = assert_len_expr(cx, expr) {
+        let hash = hash_expr(cx, slice);
+        match indexes.entry(hash) {
+            Entry::Vacant(entry) => {
+                entry.insert(IndexEntry::StrayAssert {
+                    asserted_len,
+                    cmp,
+                    assert_span: expr.span,
+                    slice_span: slice.span,
+                });
+            },
+            Entry::Occupied(mut entry) => {
+                if let IndexEntry::AssertWithoutIndex {
+                    highest_index, indexes, ..
+                } = entry.get_mut()
+                {
+                    let indexes = mem::take(indexes);
+                    let highest_index = *highest_index;
+
+                    *entry.get_mut() = IndexEntry::AssertWithIndex {
+                        highest_index,
+                        asserted_len,
+                        indexes,
+                        cmp,
+                        assert_span: expr.span,
+                        slice_span: slice.span,
+                    };
+                }
+            },
+        }
+    }
+}
+
+fn report_indexes(cx: &LateContext<'_>, indexes: &mut FxHashMap<u64, IndexEntry>) {
+    for entry in indexes.values() {
+        let Some(full_span) = entry
+            .spans()
+            .and_then(|spans| spans.first().zip(spans.last()))
+            .map(|(low, &high)| low.to(high)) else {
+                continue;
+            };
+
+        match entry {
+            IndexEntry::AssertWithIndex {
+                highest_index,
+                asserted_len,
+                indexes,
+                cmp,
+                assert_span,
+                slice_span: receiver_span,
+            } if indexes.len() > 1 => {
+                // if we have found an `assert!`, let's also check that it's actually right
+                // and convers the highest index and if not, suggest the correct length
+                let sugg = match cmp {
+                    // `v.len() < 5` and `v.len() <= 5` does nothing in terms of bounds checks.
+                    // The user probably meant `v.len() > 5`
+                    LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => Some(format!(
+                        "assert!({}.len() > {highest_index})",
+                        snippet(cx, *receiver_span, "..")
+                    )),
+                    // `5 < v.len()` == `v.len() > 5`
+                    LengthComparison::IntLessThanLength if asserted_len < highest_index => Some(format!(
+                        "assert!({}.len() > {highest_index})",
+                        snippet(cx, *receiver_span, "..")
+                    )),
+                    // `5 <= v.len() == `v.len() >= 5`
+                    LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => Some(format!(
+                        "assert!({}.len() > {highest_index})",
+                        snippet(cx, *receiver_span, "..")
+                    )),
+                    _ => None,
+                };
+
+                if let Some(sugg) = sugg {
+                    report_lint(
+                        cx,
+                        full_span,
+                        "indexing into a slice multiple times with an `assert` that does not cover the highest index",
+                        indexes,
+                        |diag| {
+                            diag.span_suggestion(
+                                *assert_span,
+                                "provide the highest index that is indexed with",
+                                sugg,
+                                Applicability::MachineApplicable,
+                            );
+                        },
+                    );
+                }
+            },
+            IndexEntry::AssertWithoutIndex {
+                indexes,
+                highest_index,
+                slice_span: receiver_span,
+            } if indexes.len() > 1 => {
+                report_lint(
+                    cx,
+                    full_span,
+                    "indexing into a slice multiple times without an `assert`",
+                    indexes,
+                    |diag| {
+                        diag.help(format!(
+                            "consider asserting the length before indexing: `assert!({}.len() > {highest_index});`",
+                            snippet(cx, *receiver_span, "..")
+                        ));
+                    },
+                );
+            },
+            _ => {},
+        }
+    }
+}
+
+impl LateLintPass<'_> for MissingAssertsForIndexing {
+    fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) {
+        let mut indexes = FxHashMap::default();
+
+        for_each_expr(block, |expr| {
+            check_index(cx, expr, &mut indexes);
+            check_assert(cx, expr, &mut indexes);
+            ControlFlow::<!, ()>::Continue(())
+        });
+
+        report_indexes(cx, &mut indexes);
+    }
+}
diff --git a/tests/ui/missing_assert_for_indexing.rs b/tests/ui/missing_assert_for_indexing.rs
diff --git a/tests/ui/missing_assert_for_indexing.stderr b/tests/ui/missing_assert_for_indexing.stderr
