Teach avoidable_slice_indexing about HirIds and Visitors

Author: xFrednet

clippy_lints/src/avoidable_slice_indexing.rs

@@ -2,19 +2,18 @@ use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::IfLet;
 use clippy_utils::ty::implements_trait;
-use clippy_utils::{in_macro, is_expn_of, is_lint_allowed, meets_msrv, msrvs};
+use clippy_utils::{in_macro, 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::mir::FakeReadCause;
+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, Symbol};
-use rustc_trait_selection::infer::TyCtxtInferExt;
-use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, Place, PlaceBase, PlaceWithHirId};
+use rustc_span::{symbol::Ident, Span};
 use std::convert::TryInto;
 
 declare_clippy_lint! {
@@ -89,24 +88,23 @@ impl LateLintPass<'_> for AvoidableSliceIndexing {
 }
 
 fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> Option<FxHashMap<hir::HirId, SliceLintInformation>> {
-    let mut removed_pat: FxHashSet<Symbol> = FxHashSet::default();
-    let mut ident_to_id: FxHashMap<Symbol, hir::HirId> = FxHashMap::default();
+    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, _, ident, sub_pat) = pat.kind {
+        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(&ident.name) {
+            if removed_pat.contains(&value_hir_id) {
                 return;
             }
             if sub_pat.is_some() {
-                removed_pat.insert(ident.name);
-                ident_to_id.remove(&ident.name).map(|key| slices.remove(&key));
+                removed_pat.insert(value_hir_id);
+                slices.remove(&value_hir_id);
                 return;
             }
 
@@ -121,9 +119,8 @@ fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> Option<FxHashM
                         .copy_trait()
                         .map_or(false, |trait_id| implements_trait(cx, inner_ty, trait_id, &[]));
 
-                let key = *ident_to_id.entry(ident.name).or_insert(pat.hir_id);
                 let slice_info = slices
-                    .entry(key)
+                    .entry(value_hir_id)
                     .or_insert_with(|| SliceLintInformation::new(ident, needs_ref));
                 slice_info.pattern_spans.push(pat.span);
             }
@@ -209,23 +206,21 @@ impl SliceLintInformation {
     }
 }
 
-fn filter_lintable_slices(
-    cx: &LateContext<'_>,
+fn filter_lintable_slices<'a, 'tcx>(
+    cx: &'a LateContext<'tcx>,
     slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>,
     index_limit: u64,
-    scope: &hir::Expr<'_>,
+    scope: &'tcx hir::Expr<'tcx>,
 ) -> Option<FxHashMap<hir::HirId, SliceLintInformation>> {
-    let mut v = SliceIndexLintingDelegate {
+    let mut visitor = SliceIndexLintingDelegate {
         cx,
         slice_lint_info,
         index_limit,
     };
 
-    cx.tcx.infer_ctxt().enter(|infcx| {
-        ExprUseVisitor::new(&mut v, &infcx, scope.hir_id.owner, cx.param_env, cx.typeck_results()).consume_expr(scope);
-    });
+    intravisit::walk_expr(&mut visitor, scope);
 
-    let slices: FxHashMap<hir::HirId, SliceLintInformation> = v
+    let slices: FxHashMap<hir::HirId, SliceLintInformation> = visitor
         .slice_lint_info
         .drain()
         .filter(|(_key, value)| !value.index_use.is_empty())
@@ -239,54 +234,45 @@ struct SliceIndexLintingDelegate<'a, 'tcx> {
     index_limit: u64,
 }
 
-impl<'a, 'tcx> SliceIndexLintingDelegate<'a, 'tcx> {
-    fn check_place(&mut self, place: &PlaceWithHirId<'tcx>, hir_id: hir::HirId) {
-        if let PlaceBase::Local(slice_id) = place.place.base {
-            if !self.slice_lint_info.contains_key(&slice_id) {
-                return;
-            }
+impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingDelegate<'a, 'tcx> {
+    type Map = Map<'tcx>;
 
-            // Get the parent expression
-            let map = self.cx.tcx.hir();
-            let parent_id = map.get_parent_node(hir_id);
-            let parent_expr = if let Some(hir::Node::Expr(parent_expr)) = map.find(parent_id) {
-                parent_expr
-            } else {
-                return;
-            };
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::OnlyBodies(self.cx.tcx.hir())
+    }
 
-            // Checking for slice indexing
+    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) {
+        if let Some(local_id) = path_to_local(expr) {
             if_chain! {
-                if let hir::ExprKind::AddrOf(_kind, _mut, inner_expr) = parent_expr.kind;
-                if let hir::ExprKind::Index(_, index_expr) = inner_expr.kind;
+                // Check if this is even a local we're interested in
+                if self.slice_lint_info.contains_key(&local_id);
+
+                let map = self.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(self.cx, self.cx.typeck_results(), index_expr);
                 if let Ok(index_value) = index_value.try_into();
                 if index_value < self.index_limit;
-                if let Some(use_info) = self.slice_lint_info.get_mut(&slice_id);
+
+                // 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;
+
+                // Get info to later lint this exporession
+                if let Some(use_info) = self.slice_lint_info.get_mut(&local_id);
                 then {
                     use_info.index_use.push((index_value, map.span(parent_expr.hir_id)));
                     return;
                 }
             }
 
             // The slice was used for something else than indexing
-            self.slice_lint_info.remove(&slice_id);
+            self.slice_lint_info.remove(&local_id);
         }
+        intravisit::walk_expr(self, expr);
     }
 }
-
-impl<'a, 'tcx> Delegate<'tcx> for SliceIndexLintingDelegate<'a, 'tcx> {
-    fn consume(&mut self, place: &PlaceWithHirId<'tcx>, expr_id: hir::HirId) {
-        self.check_place(place, expr_id);
-    }
-
-    fn borrow(&mut self, place: &PlaceWithHirId<'tcx>, expr_id: hir::HirId, _bk: ty::BorrowKind) {
-        self.check_place(place, expr_id);
-    }
-
-    fn mutate(&mut self, place: &PlaceWithHirId<'tcx>, expr_id: hir::HirId) {
-        self.check_place(place, expr_id);
-    }
-
-    fn fake_read(&mut self, _place: Place<'tcx>, _cause: FakeReadCause, _diag_expr_id: hir::HirId) {}
-}