Auto merge of rust-lang#7643 - xFrednet:7569-splits-for-slices, r=camsteffen

New lint `index_refutable_slice` to avoid slice indexing

A new lint to check for slices that could be deconstructed to avoid indexing. This lint should hopefully prevent some panics in other projects and ICEs for us. See rust-lang#7569 for an example

The implementation specifically checks for immutable bindings in `if let` statements to slices and arrays. Then it checks if these bindings are only used for value access using indices and that these indices are lower than the configured limit. I did my best to keep the implementation small, however the check was sadly quite complex. Now it's around 300 lines for the implementation and the rest are test.

---

Optional future improvements:
* Check for these instances also in `match` statements
* Check for mutable slice bindings that could also be destructed

---

changelog: New lint [`index_refutable_slice`]

I've already fixed a bunch of lint triggers in rust-lang#7638 to make this PR smaller

Closes: rust-lang#7569

Author: bors

CHANGELOG.md

@@ -2904,6 +2904,7 @@ Released 2018-09-13
 [`imprecise_flops`]: https://rust-lang.github.io/rust-clippy/master/index.html#imprecise_flops
 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping
 [`inconsistent_struct_constructor`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor
+[`index_refutable_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#index_refutable_slice
 [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
 [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask
 [`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string

clippy_lints/src/index_refutable_slice.rs

@@ -0,0 +1,276 @@
+use clippy_utils::consts::{constant, Constant};
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::higher::IfLet;
+use clippy_utils::ty::is_copy;
+use clippy_utils::{is_expn_of, is_lint_allowed, meets_msrv, msrvs, path_to_local};
+use if_chain::if_chain;
+use rustc_data_structures::fx::{FxHashMap, FxHashSet};
+use rustc_errors::Applicability;
+use rustc_hir as hir;
+use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::hir::map::Map;
+use rustc_middle::ty;
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::{symbol::Ident, Span};
+use std::convert::TryInto;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// The lint checks for slice bindings in patterns that are only used to
+    /// access individual slice values.
+    ///
+    /// ### Why is this bad?
+    /// Accessing slice values using indices can lead to panics. Using refutable
+    /// patterns can avoid these. Binding to individual values also improves the
+    /// readability as they can be named.
+    ///
+    /// ### Limitations
+    /// This lint currently only checks for immutable access inside `if let`
+    /// patterns.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let slice: Option<&[u32]> = Some(&[1, 2, 3]);
+    ///
+    /// if let Some(slice) = slice {
+    ///     println!("{}", slice[0]);
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let slice: Option<&[u32]> = Some(&[1, 2, 3]);
+    ///
+    /// if let Some(&[first, ..]) = slice {
+    ///     println!("{}", first);
+    /// }
+    /// ```
+    #[clippy::version = "1.58.0"]
+    pub INDEX_REFUTABLE_SLICE,
+    nursery,
+    "avoid indexing on slices which could be destructed"
+}
+
+#[derive(Copy, Clone)]
+pub struct IndexRefutableSlice {
+    max_suggested_slice: u64,
+    msrv: Option<RustcVersion>,
+}
+
+impl IndexRefutableSlice {
+    pub fn new(max_suggested_slice_pattern_length: u64, msrv: Option<RustcVersion>) -> Self {
+        Self {
+            max_suggested_slice: max_suggested_slice_pattern_length,
+            msrv,
+        }
+    }
+}
+
+impl_lint_pass!(IndexRefutableSlice => [INDEX_REFUTABLE_SLICE]);
+
+impl LateLintPass<'_> for IndexRefutableSlice {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
+        if_chain! {
+            if !expr.span.from_expansion() || is_expn_of(expr.span, "if_chain").is_some();
+            if let Some(IfLet {let_pat, if_then, ..}) = IfLet::hir(cx, expr);
+            if !is_lint_allowed(cx, INDEX_REFUTABLE_SLICE, expr.hir_id);
+            if meets_msrv(self.msrv.as_ref(), &msrvs::SLICE_PATTERNS);
+
+            let found_slices = find_slice_values(cx, let_pat);
+            if !found_slices.is_empty();
+            let filtered_slices = filter_lintable_slices(cx, found_slices, self.max_suggested_slice, if_then);
+            if !filtered_slices.is_empty();
+            then {
+                for slice in filtered_slices.values() {
+                    lint_slice(cx, slice);
+                }
+            }
+        }
+    }
+
+    extract_msrv_attr!(LateContext);
+}
+
+fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> FxHashMap<hir::HirId, SliceLintInformation> {
+    let mut removed_pat: FxHashSet<hir::HirId> = FxHashSet::default();
+    let mut slices: FxHashMap<hir::HirId, SliceLintInformation> = FxHashMap::default();
+    pat.walk_always(|pat| {
+        if let hir::PatKind::Binding(binding, value_hir_id, ident, sub_pat) = pat.kind {
+            // We'll just ignore mut and ref mut for simplicity sake right now
+            if let hir::BindingAnnotation::Mutable | hir::BindingAnnotation::RefMut = binding {
+                return;
+            }
+
+            // This block catches bindings with sub patterns. It would be hard to build a correct suggestion
+            // for them and it's likely that the user knows what they are doing in such a case.
+            if removed_pat.contains(&value_hir_id) {
+                return;
+            }
+            if sub_pat.is_some() {
+                removed_pat.insert(value_hir_id);
+                slices.remove(&value_hir_id);
+                return;
+            }
+
+            let bound_ty = cx.typeck_results().node_type(pat.hir_id);
+            if let ty::Slice(inner_ty) | ty::Array(inner_ty, _) = bound_ty.peel_refs().kind() {
+                // The values need to use the `ref` keyword if they can't be copied.
+                // This will need to be adjusted if the lint want to support multable access in the future
+                let src_is_ref = bound_ty.is_ref() && binding != hir::BindingAnnotation::Ref;
+                let needs_ref = !(src_is_ref || is_copy(cx, inner_ty));
+
+                let slice_info = slices
+                    .entry(value_hir_id)
+                    .or_insert_with(|| SliceLintInformation::new(ident, needs_ref));
+                slice_info.pattern_spans.push(pat.span);
+            }
+        }
+    });
+
+    slices
+}
+
+fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) {
+    let used_indices = slice
+        .index_use
+        .iter()
+        .map(|(index, _)| *index)
+        .collect::<FxHashSet<_>>();
+
+    let value_name = |index| format!("{}_{}", slice.ident.name, index);
+
+    if let Some(max_index) = used_indices.iter().max() {
+        let opt_ref = if slice.needs_ref { "ref " } else { "" };
+        let pat_sugg_idents = (0..=*max_index)
+            .map(|index| {
+                if used_indices.contains(&index) {
+                    format!("{}{}", opt_ref, value_name(index))
+                } else {
+                    "_".to_string()
+                }
+            })
+            .collect::<Vec<_>>();
+        let pat_sugg = format!("[{}, ..]", pat_sugg_idents.join(", "));
+
+        span_lint_and_then(
+            cx,
+            INDEX_REFUTABLE_SLICE,
+            slice.ident.span,
+            "this binding can be a slice pattern to avoid indexing",
+            |diag| {
+                diag.multipart_suggestion(
+                    "try using a slice pattern here",
+                    slice
+                        .pattern_spans
+                        .iter()
+                        .map(|span| (*span, pat_sugg.clone()))
+                        .collect(),
+                    Applicability::MaybeIncorrect,
+                );
+
+                diag.multipart_suggestion(
+                    "and replace the index expressions here",
+                    slice
+                        .index_use
+                        .iter()
+                        .map(|(index, span)| (*span, value_name(*index)))
+                        .collect(),
+                    Applicability::MaybeIncorrect,
+                );
+
+                // The lint message doesn't contain a warning about the removed index expression,
+                // since `filter_lintable_slices` will only return slices where all access indices
+                // are known at compile time. Therefore, they can be removed without side effects.
+            },
+        );
+    }
+}
+
+#[derive(Debug)]
+struct SliceLintInformation {
+    ident: Ident,
+    needs_ref: bool,
+    pattern_spans: Vec<Span>,
+    index_use: Vec<(u64, Span)>,
+}
+
+impl SliceLintInformation {
+    fn new(ident: Ident, needs_ref: bool) -> Self {
+        Self {
+            ident,
+            needs_ref,
+            pattern_spans: Vec::new(),
+            index_use: Vec::new(),
+        }
+    }
+}
+
+fn filter_lintable_slices<'a, 'tcx>(
+    cx: &'a LateContext<'tcx>,
+    slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>,
+    max_suggested_slice: u64,
+    scope: &'tcx hir::Expr<'tcx>,
+) -> FxHashMap<hir::HirId, SliceLintInformation> {
+    let mut visitor = SliceIndexLintingVisitor {
+        cx,
+        slice_lint_info,
+        max_suggested_slice,
+    };
+
+    intravisit::walk_expr(&mut visitor, scope);
+
+    visitor.slice_lint_info
+}
+
+struct SliceIndexLintingVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>,
+    max_suggested_slice: u64,
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> {
+    type Map = Map<'tcx>;
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
+        if let Some(local_id) = path_to_local(expr) {
+            let Self {
+                cx,
+                ref mut slice_lint_info,
+                max_suggested_slice,
+            } = *self;
+
+            if_chain! {
+                // Check if this is even a local we're interested in
+                if let Some(use_info) = slice_lint_info.get_mut(&local_id);
+
+                let map = cx.tcx.hir();
+
+                // Checking for slice indexing
+                let parent_id = map.get_parent_node(expr.hir_id);
+                if let Some(hir::Node::Expr(parent_expr)) = map.find(parent_id);
+                if let hir::ExprKind::Index(_, index_expr) = parent_expr.kind;
+                if let Some((Constant::Int(index_value), _)) = constant(cx, cx.typeck_results(), index_expr);
+                if let Ok(index_value) = index_value.try_into();
+                if index_value < max_suggested_slice;
+
+                // Make sure that this slice index is read only
+                let maybe_addrof_id = map.get_parent_node(parent_id);
+                if let Some(hir::Node::Expr(maybe_addrof_expr)) = map.find(maybe_addrof_id);
+                if let hir::ExprKind::AddrOf(_kind, hir::Mutability::Not, _inner_expr) = maybe_addrof_expr.kind;
+                then {
+                    use_info.index_use.push((index_value, map.span(parent_expr.hir_id)));
+                    return;
+                }
+            }
+
+            // The slice was used for something other than indexing
+            self.slice_lint_info.remove(&local_id);
+        }
+        intravisit::walk_expr(self, expr);
+    }
+}

clippy_lints/src/lib.register_lints.rs

@@ -168,6 +168,7 @@ store.register_lints(&[
     implicit_return::IMPLICIT_RETURN,
     implicit_saturating_sub::IMPLICIT_SATURATING_SUB,
     inconsistent_struct_constructor::INCONSISTENT_STRUCT_CONSTRUCTOR,
+    index_refutable_slice::INDEX_REFUTABLE_SLICE,
     indexing_slicing::INDEXING_SLICING,
     indexing_slicing::OUT_OF_BOUNDS_INDEXING,
     infinite_iter::INFINITE_ITER,

clippy_lints/src/lib.register_nursery.rs

@@ -13,6 +13,7 @@ store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![
     LintId::of(floating_point_arithmetic::IMPRECISE_FLOPS),
     LintId::of(floating_point_arithmetic::SUBOPTIMAL_FLOPS),
     LintId::of(future_not_send::FUTURE_NOT_SEND),
+    LintId::of(index_refutable_slice::INDEX_REFUTABLE_SLICE),
     LintId::of(let_if_seq::USELESS_LET_IF_SEQ),
     LintId::of(missing_const_for_fn::MISSING_CONST_FOR_FN),
     LintId::of(mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL),

clippy_lints/src/lib.rs

@@ -238,6 +238,7 @@ mod implicit_hasher;
 mod implicit_return;
 mod implicit_saturating_sub;
 mod inconsistent_struct_constructor;
+mod index_refutable_slice;
 mod indexing_slicing;
 mod infinite_iter;
 mod inherent_impl;
@@ -580,6 +581,13 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
 
     store.register_late_pass(|| Box::new(size_of_in_element_count::SizeOfInElementCount));
     store.register_late_pass(|| Box::new(same_name_method::SameNameMethod));
+    let max_suggested_slice_pattern_length = conf.max_suggested_slice_pattern_length;
+    store.register_late_pass(move || {
+        Box::new(index_refutable_slice::IndexRefutableSlice::new(
+            max_suggested_slice_pattern_length,
+            msrv,
+        ))
+    });
     store.register_late_pass(|| Box::new(map_clone::MapClone));
     store.register_late_pass(|| Box::new(map_err_ignore::MapErrIgnore));
     store.register_late_pass(|| Box::new(shadow::Shadow::default()));

clippy_lints/src/unwrap.rs

@@ -231,8 +231,8 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
         } else {
             // find `unwrap[_err]()` calls:
             if_chain! {
-                if let ExprKind::MethodCall(method_name, _, args, _) = expr.kind;
-                if let Some(id) = path_to_local(&args[0]);
+                if let ExprKind::MethodCall(method_name, _, [self_arg, ..], _) = expr.kind;
+                if let Some(id) = path_to_local(self_arg);
                 if [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name);
                 let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name);
                 if let Some(unwrappable) = self.unwrappables.iter()

clippy_lints/src/utils/conf.rs

@@ -148,7 +148,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),
@@ -296,6 +296,12 @@ define_Conf! {
     ///
     /// Whether to apply the raw pointer heuristic to determine if a type is `Send`.
     (enable_raw_pointer_heuristic_for_send: bool = true),
+    /// Lint: INDEX_REFUTABLE_SLICE.
+    ///
+    /// When Clippy suggests using a slice pattern, this is the maximum number of elements allowed in
+    /// the slice pattern that is suggested. If more elements would be necessary, the lint is suppressed.
+    /// For example, `[_, _, _, e, ..]` is a slice pattern with 4 elements.
+    (max_suggested_slice_pattern_length: u64 = 3),
 }
 
 /// Search for the configuration file.

clippy_utils/src/msrvs.rs

@@ -19,7 +19,7 @@ msrv_aliases! {
     1,46,0 { CONST_IF_MATCH }
     1,45,0 { STR_STRIP_PREFIX }
     1,43,0 { LOG2_10, LOG10_2 }
-    1,42,0 { MATCHES_MACRO }
+    1,42,0 { MATCHES_MACRO, SLICE_PATTERNS }
     1,41,0 { RE_REBALANCING_COHERENCE, RESULT_MAP_OR_ELSE }
     1,40,0 { MEM_TAKE, NON_EXHAUSTIVE, OPTION_AS_DEREF }
     1,38,0 { POINTER_CAST }

clippy_utils/src/ty.rs

@@ -19,6 +19,7 @@ use rustc_trait_selection::traits::query::normalize::AtExt;
 
 use crate::{match_def_path, must_use_attr};
 
+// Checks if the given type implements copy.
 pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
     ty.is_copy_modulo_regions(cx.tcx.at(DUMMY_SP), cx.param_env)
 }

tests/ui-toml/max_suggested_slice_pattern_length/clippy.toml

@@ -0,0 +1 @@
+max-suggested-slice-pattern-length = 8

tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs

@@ -0,0 +1,23 @@
+#![deny(clippy::index_refutable_slice)]
+
+fn below_limit() {
+    let slice: Option<&[u32]> = Some(&[1, 2, 3]);
+    if let Some(slice) = slice {
+        // This would usually not be linted but is included now due to the
+        // index limit in the config file
+        println!("{}", slice[7]);
+    }
+}
+
+fn above_limit() {
+    let slice: Option<&[u32]> = Some(&[1, 2, 3]);
+    if let Some(slice) = slice {
+        // This will not be linted as 8 is above the limit
+        println!("{}", slice[8]);
+    }
+}
+
+fn main() {
+    below_limit();
+    above_limit();
+}