diff --git a/CHANGELOG.md b/CHANGELOG.md index 85a6a6be8b7f2..1059f0ac7cd65 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/index_refutable_slice.rs b/clippy_lints/src/index_refutable_slice.rs new file mode 100644 index 0000000000000..69f1c90beec5d --- /dev/null +++ b/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, +} + +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); + } +} diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index b32c9b060ae07..6d1d45f890006 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_nursery.rs b/clippy_lints/src/lib.register_nursery.rs index 44c75a11eec08..cc0eb71be695f 100644 --- a/clippy_lints/src/lib.register_nursery.rs +++ b/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), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6aa136371cfbc..2445a0aeed08a 100644 --- a/clippy_lints/src/lib.rs +++ b/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())); diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index 9e83cae79bc89..71771aae44b21 100644 --- a/clippy_lints/src/unwrap.rs +++ b/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() diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 3c4f114fe5978..1a17080798059 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/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 = 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. diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index f3913203f4b4f..66d07c9d0e859 100644 --- a/clippy_utils/src/msrvs.rs +++ b/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 } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 697158282709f..babab07ea9f6a 100644 --- a/clippy_utils/src/ty.rs +++ b/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) } diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/clippy.toml b/tests/ui-toml/max_suggested_slice_pattern_length/clippy.toml new file mode 100644 index 0000000000000..78c7e63b4107f --- /dev/null +++ b/tests/ui-toml/max_suggested_slice_pattern_length/clippy.toml @@ -0,0 +1 @@ +max-suggested-slice-pattern-length = 8 diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs new file mode 100644 index 0000000000000..21849a14fa95e --- /dev/null +++ b/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(); +} diff --git a/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr new file mode 100644 index 0000000000000..d319e65d06ce8 --- /dev/null +++ b/tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr @@ -0,0 +1,22 @@ +error: this binding can be a slice pattern to avoid indexing + --> $DIR/index_refutable_slice.rs:5:17 + | +LL | if let Some(slice) = slice { + | ^^^^^ + | +note: the lint level is defined here + --> $DIR/index_refutable_slice.rs:1:9 + | +LL | #![deny(clippy::index_refutable_slice)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: try using a slice pattern here + | +LL | if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice { + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{}", slice_7); + | ~~~~~~~ + +error: aborting due to previous error + diff --git a/tests/ui-toml/min_rust_version/min_rust_version.rs b/tests/ui-toml/min_rust_version/min_rust_version.rs index bc41efa42a17c..8e104926524e1 100644 --- a/tests/ui-toml/min_rust_version/min_rust_version.rs +++ b/tests/ui-toml/min_rust_version/min_rust_version.rs @@ -59,10 +59,20 @@ fn manual_strip_msrv() { } } +fn check_index_refutable_slice() { + // This shouldn't trigger `clippy::index_refutable_slice` as the suggestion + // would only be valid from 1.42.0 onward + let slice: Option<&[u32]> = Some(&[1]); + if let Some(slice) = slice { + println!("{}", slice[0]); + } +} + fn main() { option_as_ref_deref(); match_like_matches(); match_same_arms(); match_same_arms2(); manual_strip_msrv(); + check_index_refutable_slice(); } diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 97bab1308aa52..00ddbd608a7c7 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -1,4 +1,4 @@ -error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `avoid-breaking-exported-api`, `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `disallowed-types`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `standard-macro-braces`, `enforced-import-renames`, `allowed-scripts`, `enable-raw-pointer-heuristic-for-send`, `third-party` at line 5 column 1 +error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown field `foobar`, expected one of `avoid-breaking-exported-api`, `msrv`, `blacklisted-names`, `cognitive-complexity-threshold`, `cyclomatic-complexity-threshold`, `doc-valid-idents`, `too-many-arguments-threshold`, `type-complexity-threshold`, `single-char-binding-names-threshold`, `too-large-for-stack`, `enum-variant-name-threshold`, `enum-variant-size-threshold`, `verbose-bit-mask-threshold`, `literal-representation-threshold`, `trivial-copy-size-limit`, `pass-by-value-size-limit`, `too-many-lines-threshold`, `array-size-threshold`, `vec-box-size-threshold`, `max-trait-bounds`, `max-struct-bools`, `max-fn-params-bools`, `warn-on-all-wildcard-imports`, `disallowed-methods`, `disallowed-types`, `unreadable-literal-lint-fractions`, `upper-case-acronyms-aggressive`, `cargo-ignore-publish`, `standard-macro-braces`, `enforced-import-renames`, `allowed-scripts`, `enable-raw-pointer-heuristic-for-send`, `max-suggested-slice-pattern-length`, `third-party` at line 5 column 1 error: aborting due to previous error diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.rs b/tests/ui/index_refutable_slice/if_let_slice_binding.rs new file mode 100644 index 0000000000000..c2c0c520dc62d --- /dev/null +++ b/tests/ui/index_refutable_slice/if_let_slice_binding.rs @@ -0,0 +1,166 @@ +#![deny(clippy::index_refutable_slice)] + +enum SomeEnum { + One(T), + Two(T), + Three(T), + Four(T), +} + +fn lintable_examples() { + // Try with reference + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{}", slice[0]); + } + + // Try with copy + let slice: Option<[u32; 3]> = Some([1, 2, 3]); + if let Some(slice) = slice { + println!("{}", slice[0]); + } + + // Try with long slice and small indices + let slice: Option<[u32; 9]> = Some([1, 2, 3, 4, 5, 6, 7, 8, 9]); + if let Some(slice) = slice { + println!("{}", slice[2]); + println!("{}", slice[0]); + } + + // Multiple bindings + let slice_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([5, 6, 7]); + if let SomeEnum::One(slice) | SomeEnum::Three(slice) = slice_wrapped { + println!("{}", slice[0]); + } + + // Two lintable slices in one if let + let a_wrapped: SomeEnum<[u32; 3]> = SomeEnum::One([9, 5, 1]); + let b_wrapped: Option<[u32; 2]> = Some([4, 6]); + if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) { + println!("{} -> {}", a[2], b[1]); + } + + // This requires the slice values to be borrowed as the slice values can only be + // borrowed and `String` doesn't implement copy + let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]); + if let Some(ref slice) = slice { + println!("{:?}", slice[1]); + } + println!("{:?}", slice); + + // This should not suggest using the `ref` keyword as the scrutinee is already + // a reference + let slice: Option<[String; 2]> = Some([String::from("1"), String::from("2")]); + if let Some(slice) = &slice { + println!("{:?}", slice[0]); + } + println!("{:?}", slice); +} + +fn slice_index_above_limit() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + + if let Some(slice) = slice { + // Would cause a panic, IDK + println!("{}", slice[7]); + } +} + +fn slice_is_used() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{:?}", slice.len()); + } + + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{:?}", slice.to_vec()); + } + + let opt: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]); + if let Some(slice) = opt { + if !slice.is_empty() { + println!("first: {}", slice[0]); + } + } +} + +/// The slice is used by an external function and should therefore not be linted +fn check_slice_as_arg() { + fn is_interesting(slice: &[T; 2]) -> bool { + !slice.is_empty() + } + + let slice_wrapped: Option<[String; 2]> = Some([String::from("Hello"), String::from("world")]); + if let Some(slice) = &slice_wrapped { + if is_interesting(slice) { + println!("This is interesting {}", slice[0]); + } + } + println!("{:?}", slice_wrapped); +} + +fn check_slice_in_struct() { + #[derive(Debug)] + struct Wrapper<'a> { + inner: Option<&'a [String]>, + is_awesome: bool, + } + + impl<'a> Wrapper<'a> { + fn is_super_awesome(&self) -> bool { + self.is_awesome + } + } + + let inner = &[String::from("New"), String::from("World")]; + let wrap = Wrapper { + inner: Some(inner), + is_awesome: true, + }; + + // Test 1: Field access + if let Some(slice) = wrap.inner { + if wrap.is_awesome { + println!("This is awesome! {}", slice[0]); + } + } + + // Test 2: function access + if let Some(slice) = wrap.inner { + if wrap.is_super_awesome() { + println!("This is super awesome! {}", slice[0]); + } + } + println!("Complete wrap: {:?}", wrap); +} + +/// This would be a nice additional feature to have in the future, but adding it +/// now would make the PR too large. This is therefore only a test that we don't +/// lint cases we can't make a reasonable suggestion for +fn mutable_slice_index() { + // Mut access + let mut slice: Option<[String; 1]> = Some([String::from("Penguin")]); + if let Some(ref mut slice) = slice { + slice[0] = String::from("Mr. Penguin"); + } + println!("Use after modification: {:?}", slice); + + // Mut access on reference + let mut slice: Option<[String; 1]> = Some([String::from("Cat")]); + if let Some(slice) = &mut slice { + slice[0] = String::from("Lord Meow Meow"); + } + println!("Use after modification: {:?}", slice); +} + +/// The lint will ignore bindings with sub patterns as it would be hard +/// to build correct suggestions for these instances :) +fn binding_with_sub_pattern() { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice @ [_, _, _]) = slice { + println!("{:?}", slice[2]); + } +} + +fn main() {} diff --git a/tests/ui/index_refutable_slice/if_let_slice_binding.stderr b/tests/ui/index_refutable_slice/if_let_slice_binding.stderr new file mode 100644 index 0000000000000..a607df9b87660 --- /dev/null +++ b/tests/ui/index_refutable_slice/if_let_slice_binding.stderr @@ -0,0 +1,158 @@ +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:13:17 + | +LL | if let Some(slice) = slice { + | ^^^^^ + | +note: the lint level is defined here + --> $DIR/if_let_slice_binding.rs:1:9 + | +LL | #![deny(clippy::index_refutable_slice)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = slice { + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{}", slice_0); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:19:17 + | +LL | if let Some(slice) = slice { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = slice { + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{}", slice_0); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:25:17 + | +LL | if let Some(slice) = slice { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([slice_0, _, slice_2, ..]) = slice { + | ~~~~~~~~~~~~~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL ~ println!("{}", slice_2); +LL ~ println!("{}", slice_0); + | + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:32:26 + | +LL | if let SomeEnum::One(slice) | SomeEnum::Three(slice) = slice_wrapped { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let SomeEnum::One([slice_0, ..]) | SomeEnum::Three([slice_0, ..]) = slice_wrapped { + | ~~~~~~~~~~~~~ ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{}", slice_0); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:39:29 + | +LL | if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) { + | ^ + | +help: try using a slice pattern here + | +LL | if let (SomeEnum::Three([_, _, a_2, ..]), Some(b)) = (a_wrapped, b_wrapped) { + | ~~~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{} -> {}", a_2, b[1]); + | ~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:39:38 + | +LL | if let (SomeEnum::Three(a), Some(b)) = (a_wrapped, b_wrapped) { + | ^ + | +help: try using a slice pattern here + | +LL | if let (SomeEnum::Three(a), Some([_, b_1, ..])) = (a_wrapped, b_wrapped) { + | ~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{} -> {}", a[2], b_1); + | ~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:46:21 + | +LL | if let Some(ref slice) = slice { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([_, ref slice_1, ..]) = slice { + | ~~~~~~~~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{:?}", slice_1); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:54:17 + | +LL | if let Some(slice) = &slice { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = &slice { + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{:?}", slice_0); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:123:17 + | +LL | if let Some(slice) = wrap.inner { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = wrap.inner { + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("This is awesome! {}", slice_0); + | ~~~~~~~ + +error: this binding can be a slice pattern to avoid indexing + --> $DIR/if_let_slice_binding.rs:130:17 + | +LL | if let Some(slice) = wrap.inner { + | ^^^^^ + | +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = wrap.inner { + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("This is super awesome! {}", slice_0); + | ~~~~~~~ + +error: aborting due to 10 previous errors + diff --git a/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs b/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs new file mode 100644 index 0000000000000..406e82083f88f --- /dev/null +++ b/tests/ui/index_refutable_slice/slice_indexing_in_macro.rs @@ -0,0 +1,28 @@ +#![deny(clippy::index_refutable_slice)] + +extern crate if_chain; +use if_chain::if_chain; + +macro_rules! if_let_slice_macro { + () => { + // This would normally be linted + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice { + println!("{}", slice[0]); + } + }; +} + +fn main() { + // Don't lint this + if_let_slice_macro!(); + + // Do lint this + if_chain! { + let slice: Option<&[u32]> = Some(&[1, 2, 3]); + if let Some(slice) = slice; + then { + println!("{}", slice[0]); + } + } +} diff --git a/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr b/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr new file mode 100644 index 0000000000000..11b19428b4fdc --- /dev/null +++ b/tests/ui/index_refutable_slice/slice_indexing_in_macro.stderr @@ -0,0 +1,22 @@ +error: this binding can be a slice pattern to avoid indexing + --> $DIR/slice_indexing_in_macro.rs:23:21 + | +LL | if let Some(slice) = slice; + | ^^^^^ + | +note: the lint level is defined here + --> $DIR/slice_indexing_in_macro.rs:1:9 + | +LL | #![deny(clippy::index_refutable_slice)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: try using a slice pattern here + | +LL | if let Some([slice_0, ..]) = slice; + | ~~~~~~~~~~~~~ +help: and replace the index expressions here + | +LL | println!("{}", slice_0); + | ~~~~~~~ + +error: aborting due to previous error +