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, } impl IndexRefutableSlice { pub fn new(max_suggested_slice_pattern_length: u64, msrv: Option) -> 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 { let mut removed_pat: FxHashSet = FxHashSet::default(); let mut slices: FxHashMap = 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::>(); 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::>(); 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, 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, max_suggested_slice: u64, scope: &'tcx hir::Expr<'tcx>, ) -> FxHashMap { 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, max_suggested_slice: u64, } impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { 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); } }