From 97311f0906ca89656f5942b326a665fe98d84c17 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 22 May 2021 17:08:17 -0400 Subject: [PATCH 1/2] 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 + From cfddf0927bd71b859bc1e749ec159285433a3849 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sun, 23 May 2021 13:16:09 -0400 Subject: [PATCH 2/2] Fix type checks for `manual_str_repeat` --- clippy_lints/src/methods/manual_str_repeat.rs | 48 +++++++++++-------- clippy_lints/src/methods/mod.rs | 7 +-- clippy_lints/src/utils/conf.rs | 2 +- clippy_utils/src/sugg.rs | 27 ++++++++++- tests/ui/manual_str_repeat.fixed | 41 ++++++++++++++-- tests/ui/manual_str_repeat.rs | 39 ++++++++++++++- tests/ui/manual_str_repeat.stderr | 44 ++++++++++++----- 7 files changed, 163 insertions(+), 45 deletions(-) diff --git a/clippy_lints/src/methods/manual_str_repeat.rs b/clippy_lints/src/methods/manual_str_repeat.rs index 3f28412fbf7..919e2628c52 100644 --- a/clippy_lints/src/methods/manual_str_repeat.rs +++ b/clippy_lints/src/methods/manual_str_repeat.rs @@ -1,40 +1,49 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_context; +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::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 rustc_middle::ty::{self, Ty, TyS}; +use rustc_span::symbol::sym; +use std::borrow::Cow; use super::MANUAL_STR_REPEAT; enum RepeatKind { - Str, String, - Char, + Char(char), +} + +fn get_ty_param(ty: Ty<'_>) -> Option> { + if let ty::Adt(_, subs) = ty.kind() { + subs.types().next() + } else { + None + } } 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), + 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) - || match_type(cx, ty, &paths::COW) + || (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::Str) + (ty.is_str() || is_type_diagnostic_item(cx, ty, sym::string_type)).then(|| RepeatKind::String) } } } @@ -61,18 +70,19 @@ pub(super) fn check( 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]), + 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( diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 62a56434b3c..c8ae972f18c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -61,12 +61,9 @@ 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 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; diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 1bd38dc042c..0e33ae740d9 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/clippy_lints/src/utils/conf.rs @@ -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 = 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 = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()), diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index e5a70f0beac..efc0ec50fdc 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -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 { diff --git a/tests/ui/manual_str_repeat.fixed b/tests/ui/manual_str_repeat.fixed index 62225e7a7f8..dc140257f32 100644 --- a/tests/ui/manual_str_repeat.fixed +++ b/tests/ui/manual_str_repeat.fixed @@ -1,8 +1,10 @@ // run-rustfix +#![feature(custom_inner_attributes)] #![warn(clippy::manual_str_repeat)] -use std::iter::repeat; +use std::borrow::Cow; +use std::iter::{repeat, FromIterator}; fn main() { let _: String = "test".repeat(10); @@ -17,8 +19,8 @@ fn main() { macro_rules! m { ($e:expr) => {{ $e }}; } - - let _: String = m!("test").repeat(m!(count)); + // FIXME: macro args are fine + let _: String = repeat(m!("test")).take(m!(count)).collect(); let x = &x; let _: String = (*x).repeat(count); @@ -28,4 +30,37 @@ fn main() { } // Don't lint, repeat is from a macro. let _: String = repeat_m!("test").take(count).collect(); + + let x: Box = Box::from("test"); + let _: String = x.repeat(count); + + #[derive(Clone)] + struct S; + impl FromIterator> for String { + fn from_iter>>(_: 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); } diff --git a/tests/ui/manual_str_repeat.rs b/tests/ui/manual_str_repeat.rs index 1acd66da275..0d69c989b2e 100644 --- a/tests/ui/manual_str_repeat.rs +++ b/tests/ui/manual_str_repeat.rs @@ -1,8 +1,10 @@ // run-rustfix +#![feature(custom_inner_attributes)] #![warn(clippy::manual_str_repeat)] -use std::iter::repeat; +use std::borrow::Cow; +use std::iter::{repeat, FromIterator}; fn main() { let _: String = std::iter::repeat("test").take(10).collect(); @@ -17,7 +19,7 @@ fn main() { macro_rules! m { ($e:expr) => {{ $e }}; } - + // FIXME: macro args are fine let _: String = repeat(m!("test")).take(m!(count)).collect(); let x = &x; @@ -28,4 +30,37 @@ fn main() { } // Don't lint, repeat is from a macro. let _: String = repeat_m!("test").take(count).collect(); + + let x: Box = Box::from("test"); + let _: String = repeat(x).take(count).collect(); + + #[derive(Clone)] + struct S; + impl FromIterator> for String { + fn from_iter>>(_: 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(); } diff --git a/tests/ui/manual_str_repeat.stderr b/tests/ui/manual_str_repeat.stderr index ef67ad2a1fd..c6511689716 100644 --- a/tests/ui/manual_str_repeat.stderr +++ b/tests/ui/manual_str_repeat.stderr @@ -1,5 +1,5 @@ error: manual implementation of `str::repeat` using iterators - --> $DIR/manual_str_repeat.rs:8:21 + --> $DIR/manual_str_repeat.rs:10:21 | LL | let _: String = std::iter::repeat("test").take(10).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"test".repeat(10)` @@ -7,40 +7,58 @@ LL | let _: String = std::iter::repeat("test").take(10).collect(); = 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 + --> $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:10:21 + --> $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:11:21 + --> $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:15:13 + --> $DIR/manual_str_repeat.rs:17: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 + --> $DIR/manual_str_repeat.rs:26:21 | LL | let _: String = repeat(*x).take(count).collect(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)` -error: aborting due to 7 previous errors +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