From 97311f0906ca89656f5942b326a665fe98d84c17 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 22 May 2021 17:08:17 -0400 Subject: [PATCH] Add lint `manual_str_repeat` --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/mem_discriminant.rs | 3 +- clippy_lints/src/methods/clone_on_copy.rs | 5 +- clippy_lints/src/methods/manual_str_repeat.rs | 89 +++++++++++++++++++ clippy_lints/src/methods/mod.rs | 35 +++++++- clippy_utils/src/msrvs.rs | 1 + tests/ui/manual_str_repeat.fixed | 31 +++++++ tests/ui/manual_str_repeat.rs | 31 +++++++ tests/ui/manual_str_repeat.stderr | 46 ++++++++++ 10 files changed, 238 insertions(+), 7 deletions(-) create mode 100644 clippy_lints/src/methods/manual_str_repeat.rs create mode 100644 tests/ui/manual_str_repeat.fixed create mode 100644 tests/ui/manual_str_repeat.rs create mode 100644 tests/ui/manual_str_repeat.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 59daa074282..41af8e190dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2493,6 +2493,7 @@ Released 2018-09-13 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic +[`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap [`manual_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0e815be9bd5..e7dd3952b3a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -762,6 +762,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: methods::MANUAL_FILTER_MAP, methods::MANUAL_FIND_MAP, methods::MANUAL_SATURATING_ARITHMETIC, + methods::MANUAL_STR_REPEAT, methods::MAP_COLLECT_RESULT_UNIT, methods::MAP_FLATTEN, methods::MAP_UNWRAP_OR, @@ -1298,6 +1299,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(methods::MANUAL_FILTER_MAP), LintId::of(methods::MANUAL_FIND_MAP), LintId::of(methods::MANUAL_SATURATING_ARITHMETIC), + LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::MAP_COLLECT_RESULT_UNIT), LintId::of(methods::NEW_RET_NO_SELF), LintId::of(methods::OK_EXPECT), @@ -1735,6 +1737,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(loops::NEEDLESS_COLLECT), LintId::of(methods::EXPECT_FUN_CALL), LintId::of(methods::ITER_NTH), + LintId::of(methods::MANUAL_STR_REPEAT), LintId::of(methods::OR_FUN_CALL), LintId::of(methods::SINGLE_CHAR_PATTERN), LintId::of(misc::CMP_OWNED), diff --git a/clippy_lints/src/mem_discriminant.rs b/clippy_lints/src/mem_discriminant.rs index a735c616f6e..aca96e06ef2 100644 --- a/clippy_lints/src/mem_discriminant.rs +++ b/clippy_lints/src/mem_discriminant.rs @@ -7,7 +7,6 @@ use rustc_errors::Applicability; use rustc_hir::{BorrowKind, Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use std::iter; declare_clippy_lint! { /// **What it does:** Checks for calls of `mem::discriminant()` on a non-enum type. @@ -67,7 +66,7 @@ impl<'tcx> LateLintPass<'tcx> for MemDiscriminant { } } - let derefs: String = iter::repeat('*').take(derefs_needed).collect(); + let derefs = "*".repeat(derefs_needed); diag.span_suggestion( param.span, "try dereferencing", diff --git a/clippy_lints/src/methods/clone_on_copy.rs b/clippy_lints/src/methods/clone_on_copy.rs index ce2e8fa8b10..1a32af5dc7a 100644 --- a/clippy_lints/src/methods/clone_on_copy.rs +++ b/clippy_lints/src/methods/clone_on_copy.rs @@ -8,7 +8,6 @@ use rustc_hir::{BindingAnnotation, Expr, ExprKind, MatchSource, Node, PatKind}; use rustc_lint::LateContext; use rustc_middle::ty::{self, adjustment::Adjust}; use rustc_span::symbol::{sym, Symbol}; -use std::iter; use super::CLONE_DOUBLE_REF; use super::CLONE_ON_COPY; @@ -54,8 +53,8 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symbol, ty = inner; n += 1; } - let refs: String = iter::repeat('&').take(n + 1).collect(); - let derefs: String = iter::repeat('*').take(n).collect(); + let refs = "&".repeat(n + 1); + let derefs = "*".repeat(n); let explicit = format!("<{}{}>::clone({})", refs, ty, snip); diag.span_suggestion( expr.span, diff --git a/clippy_lints/src/methods/manual_str_repeat.rs b/clippy_lints/src/methods/manual_str_repeat.rs new file mode 100644 index 00000000000..3f28412fbf7 --- /dev/null +++ b/clippy_lints/src/methods/manual_str_repeat.rs @@ -0,0 +1,89 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::source::snippet_with_context; +use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item, match_type}; +use clippy_utils::{is_expr_path_def_path, paths}; +use if_chain::if_chain; +use rustc_ast::util::parser::PREC_POSTFIX; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, LangItem}; +use rustc_lint::LateContext; +use rustc_span::symbol::{sym, Symbol}; + +use super::MANUAL_STR_REPEAT; + +enum RepeatKind { + Str, + String, + Char, +} + +fn parse_repeat_arg(cx: &LateContext<'_>, e: &Expr<'_>) -> Option { + if let ExprKind::Lit(lit) = &e.kind { + match lit.node { + LitKind::Str(..) => Some(RepeatKind::Str), + LitKind::Char(_) => Some(RepeatKind::Char), + _ => None, + } + } else { + let ty = cx.typeck_results().expr_ty(e); + if is_type_diagnostic_item(cx, ty, sym::string_type) + || is_type_lang_item(cx, ty, LangItem::OwnedBox) + || match_type(cx, ty, &paths::COW) + { + Some(RepeatKind::String) + } else { + let ty = ty.peel_refs(); + (ty.is_str() || is_type_diagnostic_item(cx, ty, sym::string_type)).then(|| RepeatKind::Str) + } + } +} + +pub(super) fn check( + cx: &LateContext<'_>, + collect_expr: &Expr<'_>, + take_expr: &Expr<'_>, + take_self_arg: &Expr<'_>, + take_arg: &Expr<'_>, +) { + if_chain! { + if let ExprKind::Call(repeat_fn, [repeat_arg]) = take_self_arg.kind; + if is_expr_path_def_path(cx, repeat_fn, &paths::ITER_REPEAT); + if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(collect_expr), sym::string_type); + if let Some(collect_id) = cx.typeck_results().type_dependent_def_id(collect_expr.hir_id); + if let Some(take_id) = cx.typeck_results().type_dependent_def_id(take_expr.hir_id); + if let Some(iter_trait_id) = cx.tcx.get_diagnostic_item(sym::Iterator); + if cx.tcx.trait_of_item(collect_id) == Some(iter_trait_id); + if cx.tcx.trait_of_item(take_id) == Some(iter_trait_id); + if let Some(repeat_kind) = parse_repeat_arg(cx, repeat_arg); + let ctxt = collect_expr.span.ctxt(); + if ctxt == take_expr.span.ctxt(); + if ctxt == take_self_arg.span.ctxt(); + then { + let mut app = Applicability::MachineApplicable; + let (val_snip, val_is_mac) = snippet_with_context(cx, repeat_arg.span, ctxt, "..", &mut app); + let count_snip = snippet_with_context(cx, take_arg.span, ctxt, "..", &mut app).0; + + let val_str = match repeat_kind { + RepeatKind::String => format!("(&{})", val_snip), + RepeatKind::Str if !val_is_mac && repeat_arg.precedence().order() < PREC_POSTFIX => { + format!("({})", val_snip) + }, + RepeatKind::Str => val_snip.into(), + RepeatKind::Char if val_snip == r#"'"'"# => r#""\"""#.into(), + RepeatKind::Char if val_snip == r#"'\''"# => r#""'""#.into(), + RepeatKind::Char => format!("\"{}\"", &val_snip[1..val_snip.len() - 1]), + }; + + span_lint_and_sugg( + cx, + MANUAL_STR_REPEAT, + collect_expr.span, + "manual implementation of `str::repeat` using iterators", + "try this", + format!("{}.repeat({})", val_str, count_snip), + app + ) + } + } +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a6e2e0baadb..62a56434b3c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -32,6 +32,7 @@ mod iter_nth_zero; mod iter_skip_next; mod iterator_step_by_zero; mod manual_saturating_arithmetic; +mod manual_str_repeat; mod map_collect_result_unit; mod map_flatten; mod map_unwrap_or; @@ -60,9 +61,12 @@ mod wrong_self_convention; mod zst_offset; use bind_instead_of_map::BindInsteadOfMap; -use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; use clippy_utils::ty::{contains_adt_constructor, contains_ty, implements_trait, is_copy, is_type_diagnostic_item}; use clippy_utils::{contains_return, get_trait_def_id, in_macro, iter_input_pats, paths, return_ty}; +use clippy_utils::{ + diagnostics::{span_lint, span_lint_and_help}, + meets_msrv, msrvs, +}; use if_chain::if_chain; use rustc_hir as hir; use rustc_hir::def::Res; @@ -1664,6 +1668,27 @@ declare_clippy_lint! { "checks for `.splitn(0, ..)` and `.splitn(1, ..)`" } +declare_clippy_lint! { + /// **What it does:** Checks for manual implementations of `str::repeat` + /// + /// **Why is this bad?** These are both harder to read, as well as less performant. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// // Bad + /// let x: String = std::iter::repeat('x').take(10).collect(); + /// + /// // Good + /// let x: String = "x".repeat(10); + /// ``` + pub MANUAL_STR_REPEAT, + perf, + "manual implementation of `str::repeat`" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -1737,7 +1762,8 @@ impl_lint_pass!(Methods => [ FROM_ITER_INSTEAD_OF_COLLECT, INSPECT_FOR_EACH, IMPLICIT_CLONE, - SUSPICIOUS_SPLITN + SUSPICIOUS_SPLITN, + MANUAL_STR_REPEAT ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -1981,6 +2007,11 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio Some(("map", [m_recv, m_arg], _)) => { map_collect_result_unit::check(cx, expr, m_recv, m_arg, recv); }, + Some(("take", [take_self_arg, take_arg], _)) => { + if meets_msrv(msrv, &msrvs::STR_REPEAT) { + manual_str_repeat::check(cx, expr, recv, take_self_arg, take_arg); + } + }, _ => {}, }, ("count", []) => match method_call!(recv) { diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 00df04c0144..4a9c4fd0276 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -26,4 +26,5 @@ msrv_aliases! { 1,34,0 { TRY_FROM } 1,30,0 { ITERATOR_FIND_MAP } 1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST } + 1,16,0 { STR_REPEAT } } diff --git a/tests/ui/manual_str_repeat.fixed b/tests/ui/manual_str_repeat.fixed new file mode 100644 index 00000000000..62225e7a7f8 --- /dev/null +++ b/tests/ui/manual_str_repeat.fixed @@ -0,0 +1,31 @@ +// run-rustfix + +#![warn(clippy::manual_str_repeat)] + +use std::iter::repeat; + +fn main() { + let _: String = "test".repeat(10); + let _: String = "x".repeat(10); + let _: String = "'".repeat(10); + let _: String = "\"".repeat(10); + + let x = "test"; + let count = 10; + let _ = x.repeat(count + 2); + + macro_rules! m { + ($e:expr) => {{ $e }}; + } + + let _: String = m!("test").repeat(m!(count)); + + let x = &x; + let _: String = (*x).repeat(count); + + macro_rules! repeat_m { + ($e:expr) => {{ repeat($e) }}; + } + // Don't lint, repeat is from a macro. + let _: String = repeat_m!("test").take(count).collect(); +} diff --git a/tests/ui/manual_str_repeat.rs b/tests/ui/manual_str_repeat.rs new file mode 100644 index 00000000000..1acd66da275 --- /dev/null +++ b/tests/ui/manual_str_repeat.rs @@ -0,0 +1,31 @@ +// run-rustfix + +#![warn(clippy::manual_str_repeat)] + +use std::iter::repeat; + +fn main() { + let _: String = std::iter::repeat("test").take(10).collect(); + let _: String = std::iter::repeat('x').take(10).collect(); + let _: String = std::iter::repeat('\'').take(10).collect(); + let _: String = std::iter::repeat('"').take(10).collect(); + + let x = "test"; + let count = 10; + let _ = repeat(x).take(count + 2).collect::(); + + macro_rules! m { + ($e:expr) => {{ $e }}; + } + + let _: String = repeat(m!("test")).take(m!(count)).collect(); + + let x = &x; + let _: String = repeat(*x).take(count).collect(); + + macro_rules! repeat_m { + ($e:expr) => {{ repeat($e) }}; + } + // Don't lint, repeat is from a macro. + let _: String = repeat_m!("test").take(count).collect(); +} diff --git a/tests/ui/manual_str_repeat.stderr b/tests/ui/manual_str_repeat.stderr new file mode 100644 index 00000000000..ef67ad2a1fd --- /dev/null +++ b/tests/ui/manual_str_repeat.stderr @@ -0,0 +1,46 @@ +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:8:21 + | +LL | let _: String = std::iter::repeat("test").take(10).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"test".repeat(10)` + | + = note: `-D clippy::manual-str-repeat` implied by `-D warnings` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:9:21 + | +LL | let _: String = std::iter::repeat('x').take(10).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"x".repeat(10)` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:10:21 + | +LL | let _: String = std::iter::repeat('/'').take(10).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"'".repeat(10)` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:11:21 + | +LL | let _: String = std::iter::repeat('"').take(10).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"/"".repeat(10)` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:15:13 + | +LL | let _ = repeat(x).take(count + 2).collect::(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count + 2)` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:21:21 + | +LL | let _: String = repeat(m!("test")).take(m!(count)).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `m!("test").repeat(m!(count))` + +error: manual implementation of `str::repeat` using iterators + --> $DIR/manual_str_repeat.rs:24:21 + | +LL | let _: String = repeat(*x).take(count).collect(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)` + +error: aborting due to 7 previous errors +