diff --git a/clippy_lints/src/get_last_with_len.rs b/clippy_lints/src/get_last_with_len.rs deleted file mode 100644 index df29d9308e7..00000000000 --- a/clippy_lints/src/get_last_with_len.rs +++ /dev/null @@ -1,107 +0,0 @@ -//! lint on using `x.get(x.len() - 1)` instead of `x.last()` - -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::SpanlessEq; -use if_chain::if_chain; -use rustc_ast::ast::LitKind; -use rustc_errors::Applicability; -use rustc_hir::{BinOpKind, Expr, ExprKind}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::source_map::Spanned; -use rustc_span::sym; - -declare_clippy_lint! { - /// ### What it does - /// Checks for using `x.get(x.len() - 1)` instead of - /// `x.last()`. - /// - /// ### Why is this bad? - /// Using `x.last()` is easier to read and has the same - /// result. - /// - /// Note that using `x[x.len() - 1]` is semantically different from - /// `x.last()`. Indexing into the array will panic on out-of-bounds - /// accesses, while `x.get()` and `x.last()` will return `None`. - /// - /// There is another lint (get_unwrap) that covers the case of using - /// `x.get(index).unwrap()` instead of `x[index]`. - /// - /// ### Example - /// ```rust - /// // Bad - /// let x = vec![2, 3, 5]; - /// let last_element = x.get(x.len() - 1); - /// - /// // Good - /// let x = vec![2, 3, 5]; - /// let last_element = x.last(); - /// ``` - #[clippy::version = "1.37.0"] - pub GET_LAST_WITH_LEN, - complexity, - "Using `x.get(x.len() - 1)` when `x.last()` is correct and simpler" -} - -declare_lint_pass!(GetLastWithLen => [GET_LAST_WITH_LEN]); - -impl<'tcx> LateLintPass<'tcx> for GetLastWithLen { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if_chain! { - // Is a method call - if let ExprKind::MethodCall(path, args, _) = expr.kind; - - // Method name is "get" - if path.ident.name == sym!(get); - - // Argument 0 (the struct we're calling the method on) is a vector - if let Some(struct_calling_on) = args.get(0); - let struct_ty = cx.typeck_results().expr_ty(struct_calling_on); - if is_type_diagnostic_item(cx, struct_ty, sym::Vec); - - // Argument to "get" is a subtraction - if let Some(get_index_arg) = args.get(1); - if let ExprKind::Binary( - Spanned { - node: BinOpKind::Sub, - .. - }, - lhs, - rhs, - ) = &get_index_arg.kind; - - // LHS of subtraction is "x.len()" - if let ExprKind::MethodCall(arg_lhs_path, lhs_args, _) = &lhs.kind; - if arg_lhs_path.ident.name == sym::len; - if let Some(arg_lhs_struct) = lhs_args.get(0); - - // The two vectors referenced (x in x.get(...) and in x.len()) - if SpanlessEq::new(cx).eq_expr(struct_calling_on, arg_lhs_struct); - - // RHS of subtraction is 1 - if let ExprKind::Lit(rhs_lit) = &rhs.kind; - if let LitKind::Int(1, ..) = rhs_lit.node; - - then { - let mut applicability = Applicability::MachineApplicable; - let vec_name = snippet_with_applicability( - cx, - struct_calling_on.span, "vec", - &mut applicability, - ); - - span_lint_and_sugg( - cx, - GET_LAST_WITH_LEN, - expr.span, - &format!("accessing last element with `{0}.get({0}.len() - 1)`", vec_name), - "try", - format!("{}.last()", vec_name), - applicability, - ); - } - } - } -} diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index be5c478900f..87279de05c0 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -91,7 +91,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(functions::NOT_UNSAFE_PTR_ARG_DEREF), LintId::of(functions::RESULT_UNIT_ERR), LintId::of(functions::TOO_MANY_ARGUMENTS), - LintId::of(get_last_with_len::GET_LAST_WITH_LEN), LintId::of(identity_op::IDENTITY_OP), LintId::of(if_let_mutex::IF_LET_MUTEX), LintId::of(indexing_slicing::OUT_OF_BOUNDS_INDEXING), @@ -166,6 +165,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::FILTER_MAP_IDENTITY), LintId::of(methods::FILTER_NEXT), LintId::of(methods::FLAT_MAP_IDENTITY), + LintId::of(methods::GET_LAST_WITH_LEN), LintId::of(methods::INSPECT_FOR_EACH), LintId::of(methods::INTO_ITER_ON_REF), LintId::of(methods::IS_DIGIT_ASCII_RADIX), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index b15c979d0c7..d5dfcd10a66 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -15,7 +15,6 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(explicit_write::EXPLICIT_WRITE), LintId::of(format::USELESS_FORMAT), LintId::of(functions::TOO_MANY_ARGUMENTS), - LintId::of(get_last_with_len::GET_LAST_WITH_LEN), LintId::of(identity_op::IDENTITY_OP), LintId::of(int_plus_one::INT_PLUS_ONE), LintId::of(lifetimes::EXTRA_UNUSED_LIFETIMES), @@ -37,6 +36,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(methods::FILTER_MAP_IDENTITY), LintId::of(methods::FILTER_NEXT), LintId::of(methods::FLAT_MAP_IDENTITY), + LintId::of(methods::GET_LAST_WITH_LEN), LintId::of(methods::INSPECT_FOR_EACH), LintId::of(methods::ITER_COUNT), LintId::of(methods::MANUAL_FILTER_MAP), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 5552ea8aa80..527ff3485f2 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -183,7 +183,6 @@ store.register_lints(&[ functions::TOO_MANY_ARGUMENTS, functions::TOO_MANY_LINES, future_not_send::FUTURE_NOT_SEND, - get_last_with_len::GET_LAST_WITH_LEN, identity_op::IDENTITY_OP, if_let_mutex::IF_LET_MUTEX, if_not_else::IF_NOT_ELSE, @@ -302,6 +301,7 @@ store.register_lints(&[ methods::FLAT_MAP_IDENTITY, methods::FLAT_MAP_OPTION, methods::FROM_ITER_INSTEAD_OF_COLLECT, + methods::GET_LAST_WITH_LEN, methods::GET_UNWRAP, methods::IMPLICIT_CLONE, methods::INEFFICIENT_TO_STRING, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 4ac834f7240..2f8533723ef 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -242,7 +242,6 @@ mod from_over_into; mod from_str_radix_10; mod functions; mod future_not_send; -mod get_last_with_len; mod identity_op; mod if_let_mutex; mod if_not_else; @@ -652,7 +651,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(strings::StringLitAsBytes)); store.register_late_pass(|| Box::new(derive::Derive)); store.register_late_pass(|| Box::new(derivable_impls::DerivableImpls)); - store.register_late_pass(|| Box::new(get_last_with_len::GetLastWithLen)); store.register_late_pass(|| Box::new(drop_forget_ref::DropForgetRef)); store.register_late_pass(|| Box::new(empty_enum::EmptyEnum)); store.register_late_pass(|| Box::new(absurd_extreme_comparisons::AbsurdExtremeComparisons)); diff --git a/clippy_lints/src/methods/get_last_with_len.rs b/clippy_lints/src/methods/get_last_with_len.rs new file mode 100644 index 00000000000..23368238ef5 --- /dev/null +++ b/clippy_lints/src/methods/get_last_with_len.rs @@ -0,0 +1,55 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::SpanlessEq; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; +use rustc_span::source_map::Spanned; +use rustc_span::sym; + +use super::GET_LAST_WITH_LEN; + +pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, arg: &Expr<'_>) { + // Argument to "get" is a subtraction + if let ExprKind::Binary( + Spanned { + node: BinOpKind::Sub, .. + }, + lhs, + rhs, + ) = arg.kind + + // LHS of subtraction is "x.len()" + && let ExprKind::MethodCall(lhs_path, [lhs_recv], _) = &lhs.kind + && lhs_path.ident.name == sym::len + + // RHS of subtraction is 1 + && let ExprKind::Lit(rhs_lit) = &rhs.kind + && let LitKind::Int(1, ..) = rhs_lit.node + + // check that recv == lhs_recv `recv.get(lhs_recv.len() - 1)` + && SpanlessEq::new(cx).eq_expr(recv, lhs_recv) + && !recv.can_have_side_effects() + { + let method = match cx.typeck_results().expr_ty_adjusted(recv).peel_refs().kind() { + ty::Adt(def, _) if cx.tcx.is_diagnostic_item(sym::VecDeque, def.did()) => "back", + ty::Slice(_) => "last", + _ => return, + }; + + let mut applicability = Applicability::MachineApplicable; + let recv_snippet = snippet_with_applicability(cx, recv.span, "_", &mut applicability); + + span_lint_and_sugg( + cx, + GET_LAST_WITH_LEN, + expr.span, + &format!("accessing last element with `{recv_snippet}.get({recv_snippet}.len() - 1)`"), + "try", + format!("{recv_snippet}.{method}()"), + applicability, + ); + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 35fc452ed7c..8e89f43e207 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -21,6 +21,7 @@ mod filter_next; mod flat_map_identity; mod flat_map_option; mod from_iter_instead_of_collect; +mod get_last_with_len; mod get_unwrap; mod implicit_clone; mod inefficient_to_string; @@ -1209,6 +1210,38 @@ declare_clippy_lint! { "replace `.drain(..)` with `.into_iter()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for using `x.get(x.len() - 1)` instead of + /// `x.last()`. + /// + /// ### Why is this bad? + /// Using `x.last()` is easier to read and has the same + /// result. + /// + /// Note that using `x[x.len() - 1]` is semantically different from + /// `x.last()`. Indexing into the array will panic on out-of-bounds + /// accesses, while `x.get()` and `x.last()` will return `None`. + /// + /// There is another lint (get_unwrap) that covers the case of using + /// `x.get(index).unwrap()` instead of `x[index]`. + /// + /// ### Example + /// ```rust + /// // Bad + /// let x = vec![2, 3, 5]; + /// let last_element = x.get(x.len() - 1); + /// + /// // Good + /// let x = vec![2, 3, 5]; + /// let last_element = x.last(); + /// ``` + #[clippy::version = "1.37.0"] + pub GET_LAST_WITH_LEN, + complexity, + "Using `x.get(x.len() - 1)` when `x.last()` is correct and simpler" +} + declare_clippy_lint! { /// ### What it does /// Checks for use of `.get().unwrap()` (or @@ -2264,6 +2297,7 @@ impl_lint_pass!(Methods => [ BYTES_NTH, ITER_SKIP_NEXT, GET_UNWRAP, + GET_LAST_WITH_LEN, STRING_EXTEND_CHARS, ITER_CLONED_COLLECT, ITER_WITH_DRAIN, @@ -2590,6 +2624,7 @@ impl Methods { inspect_for_each::check(cx, expr, span2); } }, + ("get", [arg]) => get_last_with_len::check(cx, expr, recv, arg), ("get_or_insert_with", [arg]) => unnecessary_lazy_eval::check(cx, expr, recv, arg, "get_or_insert"), ("is_file", []) => filetype_is_file::check(cx, expr, recv), ("is_digit", [radix]) => is_digit_ascii_radix::check(cx, expr, recv, radix, self.msrv), diff --git a/tests/ui/get_last_with_len.fixed b/tests/ui/get_last_with_len.fixed index c8b363f9c38..1e90b37687a 100644 --- a/tests/ui/get_last_with_len.fixed +++ b/tests/ui/get_last_with_len.fixed @@ -1,10 +1,13 @@ // run-rustfix #![warn(clippy::get_last_with_len)] +#![allow(unused)] + +use std::collections::VecDeque; fn dont_use_last() { let x = vec![2, 3, 5]; - let _ = x.last(); // ~ERROR Use x.last() + let _ = x.last(); } fn indexing_two_from_end() { @@ -23,9 +26,24 @@ fn use_last_with_different_vec_length() { let _ = x.get(y.len() - 1); } -fn main() { - dont_use_last(); - indexing_two_from_end(); - index_into_last(); - use_last_with_different_vec_length(); +struct S { + field: Vec, +} + +fn in_field(s: &S) { + let _ = s.field.last(); +} + +fn main() { + let slice = &[1, 2, 3]; + let _ = slice.last(); + + let array = [4, 5, 6]; + let _ = array.last(); + + let deq = VecDeque::from([7, 8, 9]); + let _ = deq.back(); + + let nested = [[1]]; + let _ = nested[0].last(); } diff --git a/tests/ui/get_last_with_len.rs b/tests/ui/get_last_with_len.rs index bf9cb2d7e0c..d63a731bd52 100644 --- a/tests/ui/get_last_with_len.rs +++ b/tests/ui/get_last_with_len.rs @@ -1,10 +1,13 @@ // run-rustfix #![warn(clippy::get_last_with_len)] +#![allow(unused)] + +use std::collections::VecDeque; fn dont_use_last() { let x = vec![2, 3, 5]; - let _ = x.get(x.len() - 1); // ~ERROR Use x.last() + let _ = x.get(x.len() - 1); } fn indexing_two_from_end() { @@ -23,9 +26,24 @@ fn use_last_with_different_vec_length() { let _ = x.get(y.len() - 1); } -fn main() { - dont_use_last(); - indexing_two_from_end(); - index_into_last(); - use_last_with_different_vec_length(); +struct S { + field: Vec, +} + +fn in_field(s: &S) { + let _ = s.field.get(s.field.len() - 1); +} + +fn main() { + let slice = &[1, 2, 3]; + let _ = slice.get(slice.len() - 1); + + let array = [4, 5, 6]; + let _ = array.get(array.len() - 1); + + let deq = VecDeque::from([7, 8, 9]); + let _ = deq.get(deq.len() - 1); + + let nested = [[1]]; + let _ = nested[0].get(nested[0].len() - 1); } diff --git a/tests/ui/get_last_with_len.stderr b/tests/ui/get_last_with_len.stderr index 55baf87384a..ac8dd6c2e41 100644 --- a/tests/ui/get_last_with_len.stderr +++ b/tests/ui/get_last_with_len.stderr @@ -1,10 +1,40 @@ error: accessing last element with `x.get(x.len() - 1)` - --> $DIR/get_last_with_len.rs:7:13 + --> $DIR/get_last_with_len.rs:10:13 | -LL | let _ = x.get(x.len() - 1); // ~ERROR Use x.last() +LL | let _ = x.get(x.len() - 1); | ^^^^^^^^^^^^^^^^^^ help: try: `x.last()` | = note: `-D clippy::get-last-with-len` implied by `-D warnings` -error: aborting due to previous error +error: accessing last element with `s.field.get(s.field.len() - 1)` + --> $DIR/get_last_with_len.rs:34:13 + | +LL | let _ = s.field.get(s.field.len() - 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `s.field.last()` + +error: accessing last element with `slice.get(slice.len() - 1)` + --> $DIR/get_last_with_len.rs:39:13 + | +LL | let _ = slice.get(slice.len() - 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `slice.last()` + +error: accessing last element with `array.get(array.len() - 1)` + --> $DIR/get_last_with_len.rs:42:13 + | +LL | let _ = array.get(array.len() - 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `array.last()` + +error: accessing last element with `deq.get(deq.len() - 1)` + --> $DIR/get_last_with_len.rs:45:13 + | +LL | let _ = deq.get(deq.len() - 1); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `deq.back()` + +error: accessing last element with `nested[0].get(nested[0].len() - 1)` + --> $DIR/get_last_with_len.rs:48:13 + | +LL | let _ = nested[0].get(nested[0].len() - 1); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `nested[0].last()` + +error: aborting due to 6 previous errors