From df1ec91d952aa39646f38775a51834205fb75bef Mon Sep 17 00:00:00 2001 From: Chase Ruskin Date: Sun, 30 Jan 2022 19:16:10 -0500 Subject: [PATCH 1/2] adds lint logic and test for bytes_count_to_len formats code with fixes single match clippy error to replace with if let swaps ident.name.as_str to ident.name == sym for count fn --- CHANGELOG.md | 1 + clippy_lints/src/bytes_count_to_len.rs | 54 +++++++++++++++++++++ clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_complexity.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/bytes_count_to_len.rs | 15 ++++++ tests/ui/bytes_count_to_len.stderr | 19 ++++++++ 8 files changed, 94 insertions(+) create mode 100644 clippy_lints/src/bytes_count_to_len.rs create mode 100644 tests/ui/bytes_count_to_len.rs create mode 100644 tests/ui/bytes_count_to_len.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 811dce481d57..5de598a3204d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3299,6 +3299,7 @@ Released 2018-09-13 [`boxed_local`]: https://rust-lang.github.io/rust-clippy/master/index.html#boxed_local [`branches_sharing_code`]: https://rust-lang.github.io/rust-clippy/master/index.html#branches_sharing_code [`builtin_type_shadow`]: https://rust-lang.github.io/rust-clippy/master/index.html#builtin_type_shadow +[`bytes_count_to_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_count_to_len [`bytes_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#bytes_nth [`cargo_common_metadata`]: https://rust-lang.github.io/rust-clippy/master/index.html#cargo_common_metadata [`case_sensitive_file_extension_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#case_sensitive_file_extension_comparisons diff --git a/clippy_lints/src/bytes_count_to_len.rs b/clippy_lints/src/bytes_count_to_len.rs new file mode 100644 index 000000000000..799d3783f74f --- /dev/null +++ b/clippy_lints/src/bytes_count_to_len.rs @@ -0,0 +1,54 @@ +use clippy_utils::diagnostics::span_lint_and_note; +use if_chain::if_chain; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// It checks for `str::bytes().count()` and suggests replacing it with + /// `str::len()`. + /// + /// ### Why is this bad? + /// `str::bytes().count()` is longer and may not be as performant as using + /// `str::len()`. + /// + /// ### Example + /// ```rust + /// "hello".bytes().count(); + /// ``` + /// Use instead: + /// ```rust + /// "hello".len(); + /// ``` + #[clippy::version = "1.60.0"] + pub BYTES_COUNT_TO_LEN, + complexity, + "Using bytest().count() when len() performs the same functionality" +} + +declare_lint_pass!(BytesCountToLen => [BYTES_COUNT_TO_LEN]); + +impl<'tcx> LateLintPass<'tcx> for BytesCountToLen { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { + if_chain! { + //check for method call called "count" + if let hir::ExprKind::MethodCall(count_path, count_args, _) = &expr.kind; + if count_path.ident.name == rustc_span::sym::count; + if let [bytes_expr] = &**count_args; + //check for method call called "bytes" that was linked to "count" + if let hir::ExprKind::MethodCall(bytes_path, _, _) = &bytes_expr.kind; + if bytes_path.ident.name.as_str() == "bytes"; + then { + span_lint_and_note( + cx, + BYTES_COUNT_TO_LEN, + expr.span, + "using long and hard to read `.bytes().count()`", + None, + "`.len()` achieves same functionality" + ); + } + }; + } +} diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 097064ae26f8..0294b9866ac1 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -23,6 +23,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(bool_assert_comparison::BOOL_ASSERT_COMPARISON), LintId::of(booleans::LOGIC_BUG), LintId::of(booleans::NONMINIMAL_BOOL), + LintId::of(bytes_count_to_len::BYTES_COUNT_TO_LEN), LintId::of(casts::CAST_ABS_TO_UNSIGNED), LintId::of(casts::CAST_ENUM_CONSTRUCTOR), LintId::of(casts::CAST_ENUM_TRUNCATION), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 10369a855ae6..cd637b199fe2 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -5,6 +5,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec![ LintId::of(attrs::DEPRECATED_CFG_ATTR), LintId::of(booleans::NONMINIMAL_BOOL), + LintId::of(bytes_count_to_len::BYTES_COUNT_TO_LEN), LintId::of(casts::CHAR_LIT_AS_U8), LintId::of(casts::UNNECESSARY_CAST), LintId::of(derivable_impls::DERIVABLE_IMPLS), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index c608f634291f..590fc502738f 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -64,6 +64,7 @@ store.register_lints(&[ booleans::NONMINIMAL_BOOL, borrow_as_ptr::BORROW_AS_PTR, bytecount::NAIVE_BYTECOUNT, + bytes_count_to_len::BYTES_COUNT_TO_LEN, cargo::CARGO_COMMON_METADATA, cargo::MULTIPLE_CRATE_VERSIONS, cargo::NEGATIVE_FEATURE_NAMES, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 88e8a0cc2af0..9a9ba4f17790 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -181,6 +181,7 @@ mod bool_assert_comparison; mod booleans; mod borrow_as_ptr; mod bytecount; +mod bytes_count_to_len; mod cargo; mod case_sensitive_file_extension_comparisons; mod casts; @@ -875,6 +876,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(unnecessary_owned_empty_strings::UnnecessaryOwnedEmptyStrings)); store.register_early_pass(|| Box::new(pub_use::PubUse)); store.register_late_pass(|| Box::new(format_push_string::FormatPushString)); + store.register_late_pass(|| Box::new(bytes_count_to_len::BytesCountToLen)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/bytes_count_to_len.rs b/tests/ui/bytes_count_to_len.rs new file mode 100644 index 000000000000..b2dd2bfeeed7 --- /dev/null +++ b/tests/ui/bytes_count_to_len.rs @@ -0,0 +1,15 @@ +#![warn(clippy::bytes_count_to_len)] + +fn main() { + let s1 = String::from("world"); + + //test warning against a string literal + "hello".bytes().count(); + + //test warning against a string variable + s1.bytes().count(); + + //make sure using count() normally doesn't trigger warning + let vector = [0, 1, 2]; + let size = vector.iter().count(); +} diff --git a/tests/ui/bytes_count_to_len.stderr b/tests/ui/bytes_count_to_len.stderr new file mode 100644 index 000000000000..ddd08ee824c1 --- /dev/null +++ b/tests/ui/bytes_count_to_len.stderr @@ -0,0 +1,19 @@ +error: using long and hard to read `.bytes().count()` + --> $DIR/bytes_count_to_len.rs:7:5 + | +LL | "hello".bytes().count(); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::bytes-count-to-len` implied by `-D warnings` + = note: `.len()` achieves same functionality + +error: using long and hard to read `.bytes().count()` + --> $DIR/bytes_count_to_len.rs:10:5 + | +LL | s1.bytes().count(); + | ^^^^^^^^^^^^^^^^^^ + | + = note: `.len()` achieves same functionality + +error: aborting due to 2 previous errors + From f19387d2371de497552fbf465acf9438084fe77b Mon Sep 17 00:00:00 2001 From: kyoto7250 <50972773+kyoto7250@users.noreply.github.com> Date: Sat, 16 Apr 2022 17:53:22 +0900 Subject: [PATCH 2/2] add checking type adding test patterns cargo dev bless fix comment add ; delete : fix suggestion code and update stderr in tests. use match_def_path when checking method name --- clippy_lints/src/bytes_count_to_len.rs | 42 ++++++++++++++++++-------- clippy_utils/src/paths.rs | 2 ++ tests/ui/bytes_count_to_len.fixed | 34 +++++++++++++++++++++ tests/ui/bytes_count_to_len.rs | 33 +++++++++++++++----- tests/ui/bytes_count_to_len.stderr | 27 +++++++++++------ 5 files changed, 109 insertions(+), 29 deletions(-) create mode 100644 tests/ui/bytes_count_to_len.fixed diff --git a/clippy_lints/src/bytes_count_to_len.rs b/clippy_lints/src/bytes_count_to_len.rs index 799d3783f74f..d70dbf5b2390 100644 --- a/clippy_lints/src/bytes_count_to_len.rs +++ b/clippy_lints/src/bytes_count_to_len.rs @@ -1,8 +1,14 @@ -use clippy_utils::diagnostics::span_lint_and_note; +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::{match_def_path, paths}; use if_chain::if_chain; +use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; declare_clippy_lint! { /// ### What it does @@ -16,15 +22,17 @@ declare_clippy_lint! { /// ### Example /// ```rust /// "hello".bytes().count(); + /// String::from("hello").bytes().count(); /// ``` /// Use instead: /// ```rust /// "hello".len(); + /// String::from("hello").len(); /// ``` - #[clippy::version = "1.60.0"] + #[clippy::version = "1.62.0"] pub BYTES_COUNT_TO_LEN, complexity, - "Using bytest().count() when len() performs the same functionality" + "Using `bytes().count()` when `len()` performs the same functionality" } declare_lint_pass!(BytesCountToLen => [BYTES_COUNT_TO_LEN]); @@ -32,21 +40,29 @@ declare_lint_pass!(BytesCountToLen => [BYTES_COUNT_TO_LEN]); impl<'tcx> LateLintPass<'tcx> for BytesCountToLen { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { if_chain! { - //check for method call called "count" - if let hir::ExprKind::MethodCall(count_path, count_args, _) = &expr.kind; - if count_path.ident.name == rustc_span::sym::count; - if let [bytes_expr] = &**count_args; - //check for method call called "bytes" that was linked to "count" - if let hir::ExprKind::MethodCall(bytes_path, _, _) = &bytes_expr.kind; - if bytes_path.ident.name.as_str() == "bytes"; + if let hir::ExprKind::MethodCall(_, expr_args, _) = &expr.kind; + if let Some(expr_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id); + if match_def_path(cx, expr_def_id, &paths::ITER_COUNT); + + if let [bytes_expr] = &**expr_args; + if let hir::ExprKind::MethodCall(_, bytes_args, _) = &bytes_expr.kind; + if let Some(bytes_def_id) = cx.typeck_results().type_dependent_def_id(bytes_expr.hir_id); + if match_def_path(cx, bytes_def_id, &paths::STR_BYTES); + + if let [str_expr] = &**bytes_args; + let ty = cx.typeck_results().expr_ty(str_expr).peel_refs(); + + if is_type_diagnostic_item(cx, ty, sym::String) || ty.kind() == &ty::Str; then { - span_lint_and_note( + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( cx, BYTES_COUNT_TO_LEN, expr.span, "using long and hard to read `.bytes().count()`", - None, - "`.len()` achieves same functionality" + "consider calling `.len()` instead", + format!("{}.len()", snippet_with_applicability(cx, str_expr.span, "..", &mut applicability)), + applicability ); } }; diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index deb548daf2d0..4291a5e2299c 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -61,6 +61,7 @@ pub const IO_READ: [&str; 3] = ["std", "io", "Read"]; pub const IO_WRITE: [&str; 3] = ["std", "io", "Write"]; pub const IPADDR_V4: [&str; 5] = ["std", "net", "ip", "IpAddr", "V4"]; pub const IPADDR_V6: [&str; 5] = ["std", "net", "ip", "IpAddr", "V6"]; +pub const ITER_COUNT: [&str; 6] = ["core", "iter", "traits", "iterator", "Iterator", "count"]; pub const ITER_REPEAT: [&str; 5] = ["core", "iter", "sources", "repeat", "repeat"]; #[allow(clippy::invalid_paths)] // internal lints do not know about all external crates pub const ITERTOOLS_NEXT_TUPLE: [&str; 3] = ["itertools", "Itertools", "next_tuple"]; @@ -149,6 +150,7 @@ pub const STD_FS_CREATE_DIR: [&str; 3] = ["std", "fs", "create_dir"]; pub const STRING_AS_MUT_STR: [&str; 4] = ["alloc", "string", "String", "as_mut_str"]; pub const STRING_AS_STR: [&str; 4] = ["alloc", "string", "String", "as_str"]; pub const STRING_NEW: [&str; 4] = ["alloc", "string", "String", "new"]; +pub const STR_BYTES: [&str; 4] = ["core", "str", "", "bytes"]; pub const STR_ENDS_WITH: [&str; 4] = ["core", "str", "", "ends_with"]; pub const STR_FROM_UTF8: [&str; 4] = ["core", "str", "converts", "from_utf8"]; pub const STR_LEN: [&str; 4] = ["core", "str", "", "len"]; diff --git a/tests/ui/bytes_count_to_len.fixed b/tests/ui/bytes_count_to_len.fixed new file mode 100644 index 000000000000..860642363b5f --- /dev/null +++ b/tests/ui/bytes_count_to_len.fixed @@ -0,0 +1,34 @@ +// run-rustfix +#![warn(clippy::bytes_count_to_len)] +use std::fs::File; +use std::io::Read; + +fn main() { + // should fix, because type is String + let _ = String::from("foo").len(); + + let s1 = String::from("foo"); + let _ = s1.len(); + + // should fix, because type is &str + let _ = "foo".len(); + + let s2 = "foo"; + let _ = s2.len(); + + // make sure using count() normally doesn't trigger warning + let vector = [0, 1, 2]; + let _ = vector.iter().count(); + + // The type is slice, so should not fix + let _ = &[1, 2, 3].bytes().count(); + + let bytes: &[u8] = &[1, 2, 3]; + bytes.bytes().count(); + + // The type is File, so should not fix + let _ = File::open("foobar").unwrap().bytes().count(); + + let f = File::open("foobar").unwrap(); + let _ = f.bytes().count(); +} diff --git a/tests/ui/bytes_count_to_len.rs b/tests/ui/bytes_count_to_len.rs index b2dd2bfeeed7..162730c2842a 100644 --- a/tests/ui/bytes_count_to_len.rs +++ b/tests/ui/bytes_count_to_len.rs @@ -1,15 +1,34 @@ +// run-rustfix #![warn(clippy::bytes_count_to_len)] +use std::fs::File; +use std::io::Read; fn main() { - let s1 = String::from("world"); + // should fix, because type is String + let _ = String::from("foo").bytes().count(); - //test warning against a string literal - "hello".bytes().count(); + let s1 = String::from("foo"); + let _ = s1.bytes().count(); - //test warning against a string variable - s1.bytes().count(); + // should fix, because type is &str + let _ = "foo".bytes().count(); - //make sure using count() normally doesn't trigger warning + let s2 = "foo"; + let _ = s2.bytes().count(); + + // make sure using count() normally doesn't trigger warning let vector = [0, 1, 2]; - let size = vector.iter().count(); + let _ = vector.iter().count(); + + // The type is slice, so should not fix + let _ = &[1, 2, 3].bytes().count(); + + let bytes: &[u8] = &[1, 2, 3]; + bytes.bytes().count(); + + // The type is File, so should not fix + let _ = File::open("foobar").unwrap().bytes().count(); + + let f = File::open("foobar").unwrap(); + let _ = f.bytes().count(); } diff --git a/tests/ui/bytes_count_to_len.stderr b/tests/ui/bytes_count_to_len.stderr index ddd08ee824c1..224deb779871 100644 --- a/tests/ui/bytes_count_to_len.stderr +++ b/tests/ui/bytes_count_to_len.stderr @@ -1,19 +1,28 @@ error: using long and hard to read `.bytes().count()` - --> $DIR/bytes_count_to_len.rs:7:5 + --> $DIR/bytes_count_to_len.rs:8:13 | -LL | "hello".bytes().count(); - | ^^^^^^^^^^^^^^^^^^^^^^^ +LL | let _ = String::from("foo").bytes().count(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling `.len()` instead: `String::from("foo").len()` | = note: `-D clippy::bytes-count-to-len` implied by `-D warnings` - = note: `.len()` achieves same functionality error: using long and hard to read `.bytes().count()` - --> $DIR/bytes_count_to_len.rs:10:5 + --> $DIR/bytes_count_to_len.rs:11:13 | -LL | s1.bytes().count(); - | ^^^^^^^^^^^^^^^^^^ +LL | let _ = s1.bytes().count(); + | ^^^^^^^^^^^^^^^^^^ help: consider calling `.len()` instead: `s1.len()` + +error: using long and hard to read `.bytes().count()` + --> $DIR/bytes_count_to_len.rs:14:13 + | +LL | let _ = "foo".bytes().count(); + | ^^^^^^^^^^^^^^^^^^^^^ help: consider calling `.len()` instead: `"foo".len()` + +error: using long and hard to read `.bytes().count()` + --> $DIR/bytes_count_to_len.rs:17:13 | - = note: `.len()` achieves same functionality +LL | let _ = s2.bytes().count(); + | ^^^^^^^^^^^^^^^^^^ help: consider calling `.len()` instead: `s2.len()` -error: aborting due to 2 previous errors +error: aborting due to 4 previous errors