From f32c2fcb7e1d65477d197f8bfb92cbdd50776e6a Mon Sep 17 00:00:00 2001 From: Harrison McCullough Date: Mon, 20 May 2019 18:01:21 -0600 Subject: [PATCH 1/2] Implement get_last_with_len lint --- CHANGELOG.md | 1 + README.md | 2 +- clippy_lints/src/get_last_with_len.rs | 98 +++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 4 ++ tests/ui/get_last_with_len.fixed | 31 +++++++++ tests/ui/get_last_with_len.rs | 31 +++++++++ tests/ui/get_last_with_len.stderr | 10 +++ 7 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/get_last_with_len.rs create mode 100644 tests/ui/get_last_with_len.fixed create mode 100644 tests/ui/get_last_with_len.rs create mode 100644 tests/ui/get_last_with_len.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 2287b2f9d433..b63111cab722 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index a779d0dd1f8a..c25c83d87520 100644 --- a/README.md +++ b/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: diff --git a/clippy_lints/src/get_last_with_len.rs b/clippy_lints/src/get_last_with_len.rs new file mode 100644 index 000000000000..4a26c788fb9d --- /dev/null +++ b/clippy_lints/src/get_last_with_len.rs @@ -0,0 +1,98 @@ +//! 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); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f74593662634..53143800f763 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/tests/ui/get_last_with_len.fixed b/tests/ui/get_last_with_len.fixed new file mode 100644 index 000000000000..c8b363f9c38e --- /dev/null +++ b/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(); +} diff --git a/tests/ui/get_last_with_len.rs b/tests/ui/get_last_with_len.rs new file mode 100644 index 000000000000..bf9cb2d7e0cc --- /dev/null +++ b/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(); +} diff --git a/tests/ui/get_last_with_len.stderr b/tests/ui/get_last_with_len.stderr new file mode 100644 index 000000000000..55baf87384a2 --- /dev/null +++ b/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 + From 42d849c1853d47d2b392d449077cea944b7861a8 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Tue, 21 May 2019 09:33:59 +0200 Subject: [PATCH 2/2] Formatting inside if_chain! macro --- clippy_lints/src/get_last_with_len.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/get_last_with_len.rs b/clippy_lints/src/get_last_with_len.rs index 4a26c788fb9d..d6b739e77909 100644 --- a/clippy_lints/src/get_last_with_len.rs +++ b/clippy_lints/src/get_last_with_len.rs @@ -60,8 +60,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for GetLastWithLen { // 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; + 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; @@ -91,7 +97,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for GetLastWithLen { &format!("accessing last element with `{0}.get({0}.len() - 1)`", vec_name), "try", format!("{}.last()", vec_name), - applicability); + applicability, + ); } } }