Auto merge of #3832 - HarrisonMc555:use_last, r=flip1995

Implement "Use last" lint

Closes #3673

This lint checks the use of `x.get(x.len() - 1)` and suggests `x.last()` (where `x` is a `Vec`).

There's at least one more thing that needs to happen here. I believe I am correctly checking most of the scenarios and avoiding false positives. However, if different `Vec`s are used for the `x.get` and `y.len`, then it will emit a false positive. In other words, I need to check to make sure the `Vec`s are the same.

Also, a lot of these commits were temporary and not helpful to the project history...am I supposed to squash on merge? If so, how do I do that?

changelog: New lint: `get_last_with_len`

CHANGELOG.md

@@ -936,6 +936,7 @@ All notable changes to this project will be documented in this file.
 [`for_loop_over_result`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loop_over_result
 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy
 [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref
+[`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
 [`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
 [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op

README.md

@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 303 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 304 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/get_last_with_len.rs

@@ -0,0 +1,105 @@
+//! lint on using `x.get(x.len() - 1)` instead of `x.last()`
+
+use crate::utils::{match_type, paths, snippet_with_applicability, span_lint_and_sugg, SpanlessEq};
+use if_chain::if_chain;
+use rustc::hir::{BinOpKind, Expr, ExprKind};
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::{declare_lint_pass, declare_tool_lint};
+use rustc_errors::Applicability;
+use syntax::ast::LitKind;
+use syntax::source_map::Spanned;
+use syntax::symbol::Symbol;
+
+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]`.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **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();
+    /// ```
+    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<'a, 'tcx> LateLintPass<'a, 'tcx> for GetLastWithLen {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        if_chain! {
+            // Is a method call
+            if let ExprKind::MethodCall(ref path, _, ref args) = expr.node;
+
+            // Method name is "get"
+            if path.ident.name == Symbol::intern("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.tables.expr_ty(struct_calling_on);
+            if match_type(cx, struct_ty, &paths::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.node;
+
+            // LHS of subtraction is "x.len()"
+            if let ExprKind::MethodCall(arg_lhs_path, _, lhs_args) = &lhs.node;
+            if arg_lhs_path.ident.name == Symbol::intern("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.node;
+            if let LitKind::Int(rhs_value, ..) = rhs_lit.node;
+            if rhs_value == 1;
+
+            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,
+                );
+            }
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -186,6 +186,7 @@ pub mod fallible_impl_from;
 pub mod format;
 pub mod formatting;
 pub mod functions;
+pub mod get_last_with_len;
 pub mod identity_conversion;
 pub mod identity_op;
 pub mod if_not_else;
@@ -492,6 +493,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box types::CharLitAsU8);
     reg.register_late_lint_pass(box vec::UselessVec);
     reg.register_late_lint_pass(box drop_bounds::DropBounds);
+    reg.register_late_lint_pass(box get_last_with_len::GetLastWithLen);
     reg.register_late_lint_pass(box drop_forget_ref::DropForgetRef);
     reg.register_late_lint_pass(box empty_enum::EmptyEnum);
     reg.register_late_lint_pass(box types::AbsurdExtremeComparisons);
@@ -710,6 +712,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         formatting::SUSPICIOUS_ELSE_FORMATTING,
         functions::NOT_UNSAFE_PTR_ARG_DEREF,
         functions::TOO_MANY_ARGUMENTS,
+        get_last_with_len::GET_LAST_WITH_LEN,
         identity_conversion::IDENTITY_CONVERSION,
         identity_op::IDENTITY_OP,
         indexing_slicing::OUT_OF_BOUNDS_INDEXING,
@@ -981,6 +984,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         explicit_write::EXPLICIT_WRITE,
         format::USELESS_FORMAT,
         functions::TOO_MANY_ARGUMENTS,
+        get_last_with_len::GET_LAST_WITH_LEN,
         identity_conversion::IDENTITY_CONVERSION,
         identity_op::IDENTITY_OP,
         int_plus_one::INT_PLUS_ONE,

tests/ui/get_last_with_len.fixed

@@ -0,0 +1,31 @@
+// run-rustfix
+
+#![warn(clippy::get_last_with_len)]
+
+fn dont_use_last() {
+    let x = vec![2, 3, 5];
+    let _ = x.last(); // ~ERROR Use x.last()
+}
+
+fn indexing_two_from_end() {
+    let x = vec![2, 3, 5];
+    let _ = x.get(x.len() - 2);
+}
+
+fn index_into_last() {
+    let x = vec![2, 3, 5];
+    let _ = x[x.len() - 1];
+}
+
+fn use_last_with_different_vec_length() {
+    let x = vec![2, 3, 5];
+    let y = vec!['a', 'b', 'c'];
+    let _ = x.get(y.len() - 1);
+}
+
+fn main() {
+    dont_use_last();
+    indexing_two_from_end();
+    index_into_last();
+    use_last_with_different_vec_length();
+}

tests/ui/get_last_with_len.rs

@@ -0,0 +1,31 @@
+// run-rustfix
+
+#![warn(clippy::get_last_with_len)]
+
+fn dont_use_last() {
+    let x = vec![2, 3, 5];
+    let _ = x.get(x.len() - 1); // ~ERROR Use x.last()
+}
+
+fn indexing_two_from_end() {
+    let x = vec![2, 3, 5];
+    let _ = x.get(x.len() - 2);
+}
+
+fn index_into_last() {
+    let x = vec![2, 3, 5];
+    let _ = x[x.len() - 1];
+}
+
+fn use_last_with_different_vec_length() {
+    let x = vec![2, 3, 5];
+    let y = vec!['a', 'b', 'c'];
+    let _ = x.get(y.len() - 1);
+}
+
+fn main() {
+    dont_use_last();
+    indexing_two_from_end();
+    index_into_last();
+    use_last_with_different_vec_length();
+}

tests/ui/get_last_with_len.stderr

@@ -0,0 +1,10 @@
+error: accessing last element with `x.get(x.len() - 1)`
+  --> $DIR/get_last_with_len.rs:7:13
+   |
+LL |     let _ = x.get(x.len() - 1); // ~ERROR Use x.last()
+   |             ^^^^^^^^^^^^^^^^^^ help: try: `x.last()`
+   |
+   = note: `-D clippy::get-last-with-len` implied by `-D warnings`
+
+error: aborting due to previous error
+