Auto merge of rust-lang#8862 - Alexendoo:get-last-with-len, r=Jarcho,xFrednet

`get_last_with_len`: lint `VecDeque` and any deref to slice

changelog: [`get_last_with_len`]: lint `VecDeque` and any deref to slice

Previously only `Vec`s were linted, this will now catch any usages on slices, arrays, etc. It also suggests `.back()` for `VecDeque`s

Also moves the lint into `methods/`

Author: bors

clippy_lints/src/get_last_with_len.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),

clippy_lints/src/lib.register_all.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),

clippy_lints/src/lib.register_complexity.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,

clippy_lints/src/lib.register_lints.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));

clippy_lints/src/lib.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,
+        );
+    }
+}

clippy_lints/src/methods/get_last_with_len.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;
@@ -1210,6 +1211,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
@@ -2283,6 +2316,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,
@@ -2610,6 +2644,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),

clippy_lints/src/methods/mod.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.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);
 }
 
+struct S {
+    field: Vec<usize>,
+}
+
+fn in_field(s: &S) {
+    let _ = s.field.last();
+}
+
 fn main() {
-    dont_use_last();
-    indexing_two_from_end();
-    index_into_last();
-    use_last_with_different_vec_length();
+    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();
 }

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.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);
 }
 
+struct S {
+    field: Vec<usize>,
+}
+
+fn in_field(s: &S) {
+    let _ = s.field.get(s.field.len() - 1);
+}
+
 fn main() {
-    dont_use_last();
-    indexing_two_from_end();
-    index_into_last();
-    use_last_with_different_vec_length();
+    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);
 }