Fix type checks for `manual_str_repeat`

7 files changed, 162 insertions, 44 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<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::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<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()),
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<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);
 }
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<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();
 }
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::<String>();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count + 2)`
 
 error: manual implementation of `str::repeat` using iterators
-  --> $DIR/manual_str_repeat.rs:21:21
+  --> $DIR/manual_str_repeat.rs:26:21
    |
-LL |     let _: String = repeat(m!("test")).take(m!(count)).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `m!("test").repeat(m!(count))`
+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:24:21
+  --> $DIR/manual_str_repeat.rs:35:21
    |
-LL |     let _: String = repeat(*x).take(count).collect();
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)`
+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 7 previous errors
+error: aborting due to 10 previous errors