|
| 1 | +use clippy_utils::consts::{constant, Constant}; |
| 2 | +use clippy_utils::diagnostics::span_lint_and_then; |
| 3 | +use clippy_utils::higher::IfLet; |
| 4 | +use clippy_utils::ty::implements_trait; |
| 5 | +use clippy_utils::{in_macro, is_expn_of, is_lint_allowed, meets_msrv, msrvs, path_to_local}; |
| 6 | +use if_chain::if_chain; |
| 7 | +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; |
| 8 | +use rustc_errors::Applicability; |
| 9 | +use rustc_hir as hir; |
| 10 | +use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor}; |
| 11 | +use rustc_lint::{LateContext, LateLintPass, LintContext}; |
| 12 | +use rustc_middle::hir::map::Map; |
| 13 | +use rustc_middle::ty; |
| 14 | +use rustc_semver::RustcVersion; |
| 15 | +use rustc_session::{declare_tool_lint, impl_lint_pass}; |
| 16 | +use rustc_span::{symbol::Ident, Span}; |
| 17 | +use std::convert::TryInto; |
| 18 | + |
| 19 | +declare_clippy_lint! { |
| 20 | + /// ### What it does |
| 21 | + /// The lint checks for slice bindings in patterns that are only used to |
| 22 | + /// access individual slice values. |
| 23 | + /// |
| 24 | + /// ### Why is this bad? |
| 25 | + /// Accessing slice values using indices can lead to panics. Using a refutable |
| 26 | + /// pattern and binding to the individual values can avoid these. |
| 27 | + /// |
| 28 | + /// ### Limitations |
| 29 | + /// This lint currently only checks for immutable access inside `if let` |
| 30 | + /// patterns. |
| 31 | + /// |
| 32 | + /// ### Example |
| 33 | + /// ```rust |
| 34 | + /// let slice: Option<&[u32]> = Some(&[1, 2, 3]); |
| 35 | + /// |
| 36 | + /// if let Some(slice) = slice { |
| 37 | + /// println!("{}", slice[0]); |
| 38 | + /// } |
| 39 | + /// ``` |
| 40 | + /// Use instead: |
| 41 | + /// ```rust |
| 42 | + /// let slice: Option<&[u32]> = Some(&[1, 2, 3]); |
| 43 | + /// |
| 44 | + /// if let Some(&[first, ..]) = slice { |
| 45 | + /// println!("{}", first); |
| 46 | + /// } |
| 47 | + /// ``` |
| 48 | + pub INDEX_REFUTABLE_SLICE, |
| 49 | + nursery, |
| 50 | + "avoid indexing on slices which could be deconstructed" |
| 51 | +} |
| 52 | + |
| 53 | +#[derive(Copy, Clone)] |
| 54 | +pub struct IndexRefutableSlice { |
| 55 | + indexing_limit: u64, |
| 56 | + msrv: Option<RustcVersion>, |
| 57 | +} |
| 58 | + |
| 59 | +impl IndexRefutableSlice { |
| 60 | + pub fn new(max_suggested_slice_pattern_length: u64, msrv: Option<RustcVersion>) -> Self { |
| 61 | + Self { |
| 62 | + indexing_limit: max_suggested_slice_pattern_length, |
| 63 | + msrv, |
| 64 | + } |
| 65 | + } |
| 66 | +} |
| 67 | + |
| 68 | +impl_lint_pass!(IndexRefutableSlice => [INDEX_REFUTABLE_SLICE]); |
| 69 | + |
| 70 | +impl LateLintPass<'_> for IndexRefutableSlice { |
| 71 | + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { |
| 72 | + if_chain! { |
| 73 | + if !in_macro(expr.span) || is_expn_of(expr.span, "if_chain").is_some(); |
| 74 | + if let Some(IfLet {let_pat, if_then, ..}) = IfLet::hir(cx, expr); |
| 75 | + if !is_lint_allowed(cx, INDEX_REFUTABLE_SLICE, expr.hir_id); |
| 76 | + if meets_msrv(self.msrv.as_ref(), &msrvs::SLICE_PATTERNS); |
| 77 | + if let Some(found_slices) = find_slice_values(cx, let_pat); |
| 78 | + if let Some(filtered_slices) = filter_lintable_slices(cx, found_slices, self.indexing_limit, if_then); |
| 79 | + then { |
| 80 | + for slice in filtered_slices.values() { |
| 81 | + lint_slice(cx, slice); |
| 82 | + } |
| 83 | + } |
| 84 | + } |
| 85 | + } |
| 86 | + |
| 87 | + extract_msrv_attr!(LateContext); |
| 88 | +} |
| 89 | + |
| 90 | +fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> Option<FxHashMap<hir::HirId, SliceLintInformation>> { |
| 91 | + let mut removed_pat: FxHashSet<hir::HirId> = FxHashSet::default(); |
| 92 | + let mut slices: FxHashMap<hir::HirId, SliceLintInformation> = FxHashMap::default(); |
| 93 | + pat.walk_always(|pat| { |
| 94 | + if let hir::PatKind::Binding(binding, value_hir_id, ident, sub_pat) = pat.kind { |
| 95 | + // We'll just ignore mut and ref mut for simplicity sake right now |
| 96 | + if let hir::BindingAnnotation::Mutable | hir::BindingAnnotation::RefMut = binding { |
| 97 | + return; |
| 98 | + } |
| 99 | + |
| 100 | + // This block catches bindings with sub patterns. It would be hard to build a correct suggestion |
| 101 | + // for them and it's likely that the user knows what they are doing in such a case. |
| 102 | + if removed_pat.contains(&value_hir_id) { |
| 103 | + return; |
| 104 | + } |
| 105 | + if sub_pat.is_some() { |
| 106 | + removed_pat.insert(value_hir_id); |
| 107 | + slices.remove(&value_hir_id); |
| 108 | + return; |
| 109 | + } |
| 110 | + |
| 111 | + let bound_ty = cx.typeck_results().node_type(pat.hir_id); |
| 112 | + if let ty::Slice(inner_ty) | ty::Array(inner_ty, _) = bound_ty.peel_refs().kind() { |
| 113 | + // The values need to use the `ref` keyword if they can't be coppied. |
| 114 | + // This will need to be adjusted if the lint want to support multable access in the future |
| 115 | + let needs_ref = !bound_ty.is_ref() |
| 116 | + || !cx |
| 117 | + .tcx |
| 118 | + .lang_items() |
| 119 | + .copy_trait() |
| 120 | + .map_or(false, |trait_id| implements_trait(cx, inner_ty, trait_id, &[])); |
| 121 | + |
| 122 | + let slice_info = slices |
| 123 | + .entry(value_hir_id) |
| 124 | + .or_insert_with(|| SliceLintInformation::new(ident, needs_ref)); |
| 125 | + slice_info.pattern_spans.push(pat.span); |
| 126 | + } |
| 127 | + } |
| 128 | + }); |
| 129 | + |
| 130 | + (!slices.is_empty()).then(|| slices) |
| 131 | +} |
| 132 | + |
| 133 | +fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) { |
| 134 | + let used_indices = slice |
| 135 | + .index_use |
| 136 | + .iter() |
| 137 | + .map(|(index, _)| *index) |
| 138 | + .collect::<FxHashSet<_>>(); |
| 139 | + |
| 140 | + let value_name = |index| format!("{}_{}", slice.ident.name, index); |
| 141 | + |
| 142 | + if let Some(max_index) = used_indices.iter().max() { |
| 143 | + let opt_ref = if slice.needs_ref { "ref " } else { "" }; |
| 144 | + let pat_sugg_idents = (0..=*max_index) |
| 145 | + .map(|index| { |
| 146 | + if used_indices.contains(&index) { |
| 147 | + format!("{}{}", opt_ref, value_name(index)) |
| 148 | + } else { |
| 149 | + "_".to_string() |
| 150 | + } |
| 151 | + }) |
| 152 | + .collect::<Vec<_>>(); |
| 153 | + let pat_sugg = format!("[{}, ..]", pat_sugg_idents.join(", ")); |
| 154 | + |
| 155 | + span_lint_and_then( |
| 156 | + cx, |
| 157 | + INDEX_REFUTABLE_SLICE, |
| 158 | + slice.ident.span, |
| 159 | + "this binding can be a slice pattern to avoid indexing", |
| 160 | + |diag| { |
| 161 | + diag.multipart_suggestion( |
| 162 | + "try using a slice pattern here", |
| 163 | + slice |
| 164 | + .pattern_spans |
| 165 | + .iter() |
| 166 | + .map(|span| (*span, pat_sugg.clone())) |
| 167 | + .collect(), |
| 168 | + Applicability::MaybeIncorrect, |
| 169 | + ); |
| 170 | + |
| 171 | + diag.multipart_suggestion( |
| 172 | + "and replacing the index expressions here", |
| 173 | + slice |
| 174 | + .index_use |
| 175 | + .iter() |
| 176 | + .map(|(index, span)| (*span, value_name(*index))) |
| 177 | + .collect(), |
| 178 | + Applicability::MaybeIncorrect, |
| 179 | + ); |
| 180 | + |
| 181 | + // I thought about adding a note to the lint message to inform the user that these |
| 182 | + // refactorings will remove the index expression. However, I decided against this, |
| 183 | + // as `filter_lintable_slices` will only return slices where all access indices are |
| 184 | + // known at compile time. The removal should therefore not have any side effects. |
| 185 | + }, |
| 186 | + ); |
| 187 | + } |
| 188 | +} |
| 189 | + |
| 190 | +#[derive(Debug)] |
| 191 | +struct SliceLintInformation { |
| 192 | + ident: Ident, |
| 193 | + needs_ref: bool, |
| 194 | + pattern_spans: Vec<Span>, |
| 195 | + index_use: Vec<(u64, Span)>, |
| 196 | +} |
| 197 | + |
| 198 | +impl SliceLintInformation { |
| 199 | + fn new(ident: Ident, needs_ref: bool) -> Self { |
| 200 | + Self { |
| 201 | + ident, |
| 202 | + needs_ref, |
| 203 | + pattern_spans: Vec::new(), |
| 204 | + index_use: Vec::new(), |
| 205 | + } |
| 206 | + } |
| 207 | +} |
| 208 | + |
| 209 | +fn filter_lintable_slices<'a, 'tcx>( |
| 210 | + cx: &'a LateContext<'tcx>, |
| 211 | + slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>, |
| 212 | + index_limit: u64, |
| 213 | + scope: &'tcx hir::Expr<'tcx>, |
| 214 | +) -> Option<FxHashMap<hir::HirId, SliceLintInformation>> { |
| 215 | + let mut visitor = SliceIndexLintingDelegate { |
| 216 | + cx, |
| 217 | + slice_lint_info, |
| 218 | + index_limit, |
| 219 | + }; |
| 220 | + |
| 221 | + intravisit::walk_expr(&mut visitor, scope); |
| 222 | + |
| 223 | + let slices: FxHashMap<hir::HirId, SliceLintInformation> = visitor |
| 224 | + .slice_lint_info |
| 225 | + .drain() |
| 226 | + .filter(|(_key, value)| !value.index_use.is_empty()) |
| 227 | + .collect(); |
| 228 | + (!slices.is_empty()).then(|| slices) |
| 229 | +} |
| 230 | + |
| 231 | +struct SliceIndexLintingDelegate<'a, 'tcx> { |
| 232 | + cx: &'a LateContext<'tcx>, |
| 233 | + slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>, |
| 234 | + index_limit: u64, |
| 235 | +} |
| 236 | + |
| 237 | +impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingDelegate<'a, 'tcx> { |
| 238 | + type Map = Map<'tcx>; |
| 239 | + |
| 240 | + fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { |
| 241 | + NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) |
| 242 | + } |
| 243 | + |
| 244 | + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) { |
| 245 | + if let Some(local_id) = path_to_local(expr) { |
| 246 | + if_chain! { |
| 247 | + // Check if this is even a local we're interested in |
| 248 | + if self.slice_lint_info.contains_key(&local_id); |
| 249 | + |
| 250 | + let map = self.cx.tcx.hir(); |
| 251 | + |
| 252 | + // Checking for slice indexing |
| 253 | + let parent_id = map.get_parent_node(expr.hir_id); |
| 254 | + if let Some(hir::Node::Expr(parent_expr)) = map.find(parent_id); |
| 255 | + if let hir::ExprKind::Index(_, index_expr) = parent_expr.kind; |
| 256 | + if let Some((Constant::Int(index_value), _)) = constant(self.cx, self.cx.typeck_results(), index_expr); |
| 257 | + if let Ok(index_value) = index_value.try_into(); |
| 258 | + if index_value < self.index_limit; |
| 259 | + |
| 260 | + // Make sure that this slice index is read only |
| 261 | + let maybe_addrof_id = map.get_parent_node(parent_id); |
| 262 | + if let Some(hir::Node::Expr(maybe_addrof_expr)) = map.find(maybe_addrof_id); |
| 263 | + if let hir::ExprKind::AddrOf(_kind, hir::Mutability::Not, _inner_expr) = maybe_addrof_expr.kind; |
| 264 | + |
| 265 | + // Get info to later lint this exporession |
| 266 | + if let Some(use_info) = self.slice_lint_info.get_mut(&local_id); |
| 267 | + then { |
| 268 | + use_info.index_use.push((index_value, map.span(parent_expr.hir_id))); |
| 269 | + return; |
| 270 | + } |
| 271 | + } |
| 272 | + |
| 273 | + // The slice was used for something else than indexing |
| 274 | + self.slice_lint_info.remove(&local_id); |
| 275 | + } |
| 276 | + intravisit::walk_expr(self, expr); |
| 277 | + } |
| 278 | +} |
0 commit comments