mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #7265 - Jarcho:manual_str_repeat, r=giraffate

Browse Source 
Add lint `manual_str_repeat`

fixes: #7260

There's a similar function for slices. Should this be renamed to include it, or should that be a separate lint? If we are going to have them as one lint a better name will be needed. `manual_repeat` isn't exactly clear as it's replacing a call to `iter::repeat`.

changelog: Add new lint `manual_str_repeat`
This commit is contained in:
bors 2021-06-01 00:47:05 +00:00
commit ca570f9b84
12 changed files with 359 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -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

3
clippy_lints/src/lib.rs
View File

@ -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),

3
clippy_lints/src/mem_discriminant.rs
View File

@ -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",

5
clippy_lints/src/methods/clone_on_copy.rs
View File

@ -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,

99
clippy_lints/src/methods/manual_str_repeat.rs Normal file
View File

@ -0,0 +1,99 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
use clippy_utils::sugg::Sugg;
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::LitKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, LangItem};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, Ty, TyS};
use rustc_span::symbol::sym;
use std::borrow::Cow;
use super::MANUAL_STR_REPEAT;
enum RepeatKind {
String,
Char(char),
}
fn get_ty_param(ty: Ty<'_>) -> Option<Ty<'_>> {
if let ty::Adt(_, subs) = ty.kind() {
subs.types().next()
} else {
None
}
}
fn parse_repeat_arg(cx: &LateContext<'_>, e: &Expr<'_>) -> Option<RepeatKind> {
if let ExprKind::Lit(lit) = &e.kind {
match lit.node {
LitKind::Str(..) => Some(RepeatKind::String),
LitKind::Char(c) => Some(RepeatKind::Char(c)),
_ => 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) && get_ty_param(ty).map_or(false, TyS::is_str))
|| (match_type(cx, ty, &paths::COW) && get_ty_param(ty).map_or(false, TyS::is_str))
{
Some(RepeatKind::String)
} else {
let ty = ty.peel_refs();
(ty.is_str() || is_type_diagnostic_item(cx, ty, sym::string_type)).then(|| RepeatKind::String)
}
}
}
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 count_snip = snippet_with_context(cx, take_arg.span, ctxt, "..", &mut app).0;
let val_str = match repeat_kind {
RepeatKind::Char(_) if repeat_arg.span.ctxt() != ctxt => return,
RepeatKind::Char('\'') => r#""'""#.into(),
RepeatKind::Char('"') => r#""\"""#.into(),
RepeatKind::Char(_) =>
match snippet_with_applicability(cx, repeat_arg.span, "..", &mut app) {
Cow::Owned(s) => Cow::Owned(format!("\"{}\"", &s[1..s.len() - 1])),
s @ Cow::Borrowed(_) => s,
},
RepeatKind::String =>
Sugg::hir_with_context(cx, repeat_arg, ctxt, "..", &mut app).maybe_par().to_string().into(),
};
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
)
}
}
}

32
clippy_lints/src/methods/mod.rs
View File

@ -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;
@ -62,7 +63,7 @@ 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::{contains_return, get_trait_def_id, in_macro, iter_input_pats, meets_msrv, msrvs, paths, return_ty};
use if_chain::if_chain;
use rustc_hir as hir;
use rustc_hir::def::Res;
@ -1664,6 +1665,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<RustcVersion>,
@ -1737,7 +1759,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 +2004,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) {

2
clippy_lints/src/utils/conf.rs
View File

@ -124,7 +124,7 @@ macro_rules! define_Conf {
define_Conf! {
/// Lint: ENUM_VARIANT_NAMES, LARGE_TYPES_PASSED_BY_VALUE, TRIVIALLY_COPY_PASS_BY_REF, UNNECESSARY_WRAPS, UPPER_CASE_ACRONYMS, WRONG_SELF_CONVENTION. Suppress lints whenever the suggested change would cause breakage for other crates.
(avoid_breaking_exported_api: bool = true),
/// Lint: CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE. The minimum rust version that the project supports
/// Lint: MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE. The minimum rust version that the project supports
(msrv: Option<String> = None),
/// Lint: BLACKLISTED_NAME. The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses
(blacklisted_names: Vec<String> = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()),

1
clippy_utils/src/msrvs.rs
View File

@ -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 }
}

27
clippy_utils/src/sugg.rs
View File

@ -2,7 +2,7 @@
#![deny(clippy::missing_docs_in_private_items)]
use crate::higher;
use crate::source::{snippet, snippet_opt, snippet_with_macro_callsite};
use crate::source::{snippet, snippet_opt, snippet_with_context, snippet_with_macro_callsite};
use rustc_ast::util::parser::AssocOp;
use rustc_ast::{ast, token};
use rustc_ast_pretty::pprust::token_kind_to_string;
@ -10,7 +10,7 @@ use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::{EarlyContext, LateContext, LintContext};
use rustc_span::source_map::{CharPos, Span};
use rustc_span::{BytePos, Pos};
use rustc_span::{BytePos, Pos, SyntaxContext};
use std::borrow::Cow;
use std::convert::TryInto;
use std::fmt::Display;
@ -90,6 +90,29 @@ impl<'a> Sugg<'a> {
Self::hir_from_snippet(expr, snippet)
}
/// Same as `hir`, but first walks the span up to the given context. This will result in the
/// macro call, rather then the expansion, if the span is from a child context. If the span is
/// not from a child context, it will be used directly instead.
///
/// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR
/// node would result in `box []`. If given the context of the address of expression, this
/// function will correctly get a snippet of `vec![]`.
pub fn hir_with_context(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
ctxt: SyntaxContext,
default: &'a str,
applicability: &mut Applicability,
) -> Self {
let (snippet, in_macro) = snippet_with_context(cx, expr.span, ctxt, default, applicability);
if in_macro {
Sugg::NonParen(snippet)
} else {
Self::hir_from_snippet(expr, snippet)
}
}
/// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*`
/// function variants of `Sugg`, since these use different snippet functions.
fn hir_from_snippet(expr: &hir::Expr<'_>, snippet: Cow<'a, str>) -> Self {

66
tests/ui/manual_str_repeat.fixed Normal file
View File

@ -0,0 +1,66 @@
// run-rustfix
#![feature(custom_inner_attributes)]
#![warn(clippy::manual_str_repeat)]
use std::borrow::Cow;
use std::iter::{repeat, FromIterator};
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 }};
}
// FIXME: macro args are fine
let _: String = repeat(m!("test")).take(m!(count)).collect();
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();
let x: Box<str> = Box::from("test");
let _: String = x.repeat(count);
#[derive(Clone)]
struct S;
impl FromIterator<Box<S>> for String {
fn from_iter<T: IntoIterator<Item = Box<S>>>(_: T) -> Self {
Self::new()
}
}
// Don't lint, wrong box type
let _: String = repeat(Box::new(S)).take(count).collect();
let _: String = Cow::Borrowed("test").repeat(count);
let x = "x".to_owned();
let _: String = x.repeat(count);
let x = 'x';
// Don't lint, not char literal
let _: String = repeat(x).take(count).collect();
}
fn _msrv_1_15() {
#![clippy::msrv = "1.15"]
// `str::repeat` was stabilized in 1.16. Do not lint this
let _: String = std::iter::repeat("test").take(10).collect();
}
fn _msrv_1_16() {
#![clippy::msrv = "1.16"]
let _: String = "test".repeat(10);
}

66
tests/ui/manual_str_repeat.rs Normal file
View File

@ -0,0 +1,66 @@
// run-rustfix
#![feature(custom_inner_attributes)]
#![warn(clippy::manual_str_repeat)]
use std::borrow::Cow;
use std::iter::{repeat, FromIterator};
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 }};
}
// FIXME: macro args are fine
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();
let x: Box<str> = Box::from("test");
let _: String = repeat(x).take(count).collect();
#[derive(Clone)]
struct S;
impl FromIterator<Box<S>> for String {
fn from_iter<T: IntoIterator<Item = Box<S>>>(_: T) -> Self {
Self::new()
}
}
// Don't lint, wrong box type
let _: String = repeat(Box::new(S)).take(count).collect();
let _: String = repeat(Cow::Borrowed("test")).take(count).collect();
let x = "x".to_owned();
let _: String = repeat(x).take(count).collect();
let x = 'x';
// Don't lint, not char literal
let _: String = repeat(x).take(count).collect();
}
fn _msrv_1_15() {
#![clippy::msrv = "1.15"]
// `str::repeat` was stabilized in 1.16. Do not lint this
let _: String = std::iter::repeat("test").take(10).collect();
}
fn _msrv_1_16() {
#![clippy::msrv = "1.16"]
let _: String = std::iter::repeat("test").take(10).collect();
}

64
tests/ui/manual_str_repeat.stderr Normal file
View File

@ -0,0 +1,64 @@
error: manual implementation of `str::repeat` using iterators
--> $DIR/manual_str_repeat.rs:10: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:11: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:12: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:13: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:17: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:26:21
|
LL | let _: String = repeat(*x).take(count).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)`
error: manual implementation of `str::repeat` using iterators
--> $DIR/manual_str_repeat.rs:35:21
|
LL | let _: String = repeat(x).take(count).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count)`
error: manual implementation of `str::repeat` using iterators
--> $DIR/manual_str_repeat.rs:47:21
|
LL | let _: String = repeat(Cow::Borrowed("test")).take(count).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `Cow::Borrowed("test").repeat(count)`
error: manual implementation of `str::repeat` using iterators
--> $DIR/manual_str_repeat.rs:50:21
|
LL | let _: String = repeat(x).take(count).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count)`
error: manual implementation of `str::repeat` using iterators
--> $DIR/manual_str_repeat.rs:65:21
|
LL | let _: String = std::iter::repeat("test").take(10).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"test".repeat(10)`
error: aborting due to 10 previous errors