Skip to content

Commit

Permalink
Add lint manual_str_repeat
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarcho committed May 22, 2021
1 parent 029c326 commit 2d894e8
Show file tree
Hide file tree
Showing 10 changed files with 234 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,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,
Expand Down Expand Up @@ -1317,6 +1318,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),
Expand Down Expand Up @@ -1752,6 +1754,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),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/mem_discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,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",
Expand Down
4 changes: 2 additions & 2 deletions clippy_lints/src/methods/clone_on_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,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,
Expand Down
87 changes: 87 additions & 0 deletions clippy_lints/src/methods/manual_str_repeat.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
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<RepeatKind> {
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<'_>, expr: &Expr<'_>, method_name: Symbol, args: &[Expr<'_>]) {
let ctxt = expr.span.ctxt();
if_chain! {
if let [take_expr] = args;
if method_name.as_str() == "collect";
if let ExprKind::MethodCall(take_name, _, [repeat_expr, take_arg], _) = take_expr.kind;
if take_name.ident.as_str() == "take";
if let ExprKind::Call(repeat_fn, [repeat_arg]) = repeat_expr.kind;
if is_expr_path_def_path(cx, repeat_fn, &paths::ITER_REPEAT);
if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::string_type);
if let Some(collect_id) = cx.typeck_results().type_dependent_def_id(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);
if ctxt == take_expr.span.ctxt();
if ctxt == repeat_expr.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,
expr.span,
"manual implementation of `str::repeat` using iterators",
"try this",
format!("{}.repeat({})", val_str, count_snip),
app
)
}
}
}
33 changes: 31 additions & 2 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -59,9 +60,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;
Expand Down Expand Up @@ -1657,6 +1661,27 @@ declare_clippy_lint! {
"replace `.iter().count()` with `.len()`"
}

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
/// let x: String = std::iter::repeat('x').take(10).collect();
/// ```
/// Use instead:
/// ```rust
/// let x: String = "x".repeat(10);
/// ```
pub MANUAL_STR_REPEAT,
perf,
"manual implementation of `str::repeat`"
}

pub struct Methods {
msrv: Option<RustcVersion>,
}
Expand Down Expand Up @@ -1726,7 +1751,8 @@ impl_lint_pass!(Methods => [
MAP_COLLECT_RESULT_UNIT,
FROM_ITER_INSTEAD_OF_COLLECT,
INSPECT_FOR_EACH,
IMPLICIT_CLONE
IMPLICIT_CLONE,
MANUAL_STR_REPEAT
]);

/// Extracts a method call name, args, and `Span` of the method name.
Expand Down Expand Up @@ -1769,6 +1795,9 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
single_char_add_str::check(cx, expr, args);
into_iter_on_ref::check(cx, expr, *method_span, method_call.ident.name, args);
single_char_pattern::check(cx, expr, method_call.ident.name, args);
if meets_msrv(self.msrv.as_ref(), &msrvs::STR_REPEAT) {
manual_str_repeat::check(cx, expr, method_call.ident.name, args);
}
},
hir::ExprKind::Binary(op, lhs, rhs) if op.node == hir::BinOpKind::Eq || op.node == hir::BinOpKind::Ne => {
let mut info = BinaryExprInfo {
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
}
31 changes: 31 additions & 0 deletions tests/ui/manual_str_repeat.fixed
Original file line number Diff line number Diff line change
@@ -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();
}
31 changes: 31 additions & 0 deletions tests/ui/manual_str_repeat.rs
Original file line number Diff line number Diff line change
@@ -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::<String>();

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();
}
46 changes: 46 additions & 0 deletions tests/ui/manual_str_repeat.stderr
Original file line number Diff line number Diff line change
@@ -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::<String>();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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

0 comments on commit 2d894e8

Please sign in to comment.