Skip to content

Commit cfddf09

Browse files
JarchoJarcho
committed
Fix type checks for manual_str_repeat 
1 parent 97311f0 commit cfddf09

File tree

7 files changed

+162
-44
lines changed

7 files changed

+162
-44
lines changed

‎clippy_lints/src/methods/manual_str_repeat.rs

+29-19
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,49 @@
11
use clippy_utils::diagnostics::span_lint_and_sugg;
2-
use clippy_utils::source::snippet_with_context;
2+
use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
3+
use clippy_utils::sugg::Sugg;
34
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item, match_type};
45
use clippy_utils::{is_expr_path_def_path, paths};
56
use if_chain::if_chain;
6-
use rustc_ast::util::parser::PREC_POSTFIX;
77
use rustc_ast::LitKind;
88
use rustc_errors::Applicability;
99
use rustc_hir::{Expr, ExprKind, LangItem};
1010
use rustc_lint::LateContext;
11-
use rustc_span::symbol::{sym, Symbol};
11+
use rustc_middle::ty::{self, Ty, TyS};
12+
use rustc_span::symbol::sym;
13+
use std::borrow::Cow;
1214

1315
use super::MANUAL_STR_REPEAT;
1416

1517
enum RepeatKind {
16-
Str,
1718
String,
18-
Char,
19+
Char(char),
20+
}
21+
22+
fn get_ty_param(ty: Ty<'_>) -> Option<Ty<'_>> {
23+
if let ty::Adt(_, subs) = ty.kind() {
24+
subs.types().next()
25+
} else {
26+
None
27+
}
1928
}
2029

2130
fn parse_repeat_arg(cx: &LateContext<'_>, e: &Expr<'_>) -> Option<RepeatKind> {
2231
if let ExprKind::Lit(lit) = &e.kind {
2332
match lit.node {
24-
LitKind::Str(..) => Some(RepeatKind::Str),
25-
LitKind::Char(_) => Some(RepeatKind::Char),
33+
LitKind::Str(..) => Some(RepeatKind::String),
34+
LitKind::Char(c) => Some(RepeatKind::Char(c)),
2635
_ => None,
2736
}
2837
} else {
2938
let ty = cx.typeck_results().expr_ty(e);
3039
if is_type_diagnostic_item(cx, ty, sym::string_type)
31-
|| is_type_lang_item(cx, ty, LangItem::OwnedBox)
32-
|| match_type(cx, ty, &paths::COW)
40+
|| (is_type_lang_item(cx, ty, LangItem::OwnedBox) && get_ty_param(ty).map_or(false, TyS::is_str))
41+
|| (match_type(cx, ty, &paths::COW) && get_ty_param(ty).map_or(false, TyS::is_str))
3342
{
3443
Some(RepeatKind::String)
3544
} else {
3645
let ty = ty.peel_refs();
37-
(ty.is_str() || is_type_diagnostic_item(cx, ty, sym::string_type)).then(|| RepeatKind::Str)
46+
(ty.is_str() || is_type_diagnostic_item(cx, ty, sym::string_type)).then(|| RepeatKind::String)
3847
}
3948
}
4049
}
@@ -61,18 +70,19 @@ pub(super) fn check(
6170
if ctxt == take_self_arg.span.ctxt();
6271
then {
6372
let mut app = Applicability::MachineApplicable;
64-
let (val_snip, val_is_mac) = snippet_with_context(cx, repeat_arg.span, ctxt, "..", &mut app);
6573
let count_snip = snippet_with_context(cx, take_arg.span, ctxt, "..", &mut app).0;
6674

6775
let val_str = match repeat_kind {
68-
RepeatKind::String => format!("(&{})", val_snip),
69-
RepeatKind::Str if !val_is_mac && repeat_arg.precedence().order() < PREC_POSTFIX => {
70-
format!("({})", val_snip)
71-
},
72-
RepeatKind::Str => val_snip.into(),
73-
RepeatKind::Char if val_snip == r#"'"'"# => r#""\"""#.into(),
74-
RepeatKind::Char if val_snip == r#"'\''"# => r#""'""#.into(),
75-
RepeatKind::Char => format!("\"{}\"", &val_snip[1..val_snip.len() - 1]),
76+
RepeatKind::Char(_) if repeat_arg.span.ctxt() != ctxt => return,
77+
RepeatKind::Char('\'') => r#""'""#.into(),
78+
RepeatKind::Char('"') => r#""\"""#.into(),
79+
RepeatKind::Char(_) =>
80+
match snippet_with_applicability(cx, repeat_arg.span, "..", &mut app) {
81+
Cow::Owned(s) => Cow::Owned(format!("\"{}\"", &s[1..s.len() - 1])),
82+
s @ Cow::Borrowed(_) => s,
83+
},
84+
RepeatKind::String =>
85+
Sugg::hir_with_context(cx, repeat_arg, ctxt, "..", &mut app).maybe_par().to_string().into(),
7686
};
7787

7888
span_lint_and_sugg(

‎clippy_lints/src/methods/mod.rs

+2-5
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,9 @@ mod wrong_self_convention;
6161
mod zst_offset;
6262

6363
use bind_instead_of_map::BindInsteadOfMap;
64+
use clippy_utils::diagnostics::{span_lint, span_lint_and_help};
6465
use clippy_utils::ty::{contains_adt_constructor, contains_ty, implements_trait, is_copy, is_type_diagnostic_item};
65-
use clippy_utils::{contains_return, get_trait_def_id, in_macro, iter_input_pats, paths, return_ty};
66-
use clippy_utils::{
67-
diagnostics::{span_lint, span_lint_and_help},
68-
meets_msrv, msrvs,
69-
};
66+
use clippy_utils::{contains_return, get_trait_def_id, in_macro, iter_input_pats, meets_msrv, msrvs, paths, return_ty};
7067
use if_chain::if_chain;
7168
use rustc_hir as hir;
7269
use rustc_hir::def::Res;

‎clippy_lints/src/utils/conf.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ macro_rules! define_Conf {
124124
define_Conf! {
125125
/// 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.
126126
(avoid_breaking_exported_api: bool = true),
127-
/// 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
127+
/// 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
128128
(msrv: Option<String> = None),
129129
/// Lint: BLACKLISTED_NAME. The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses
130130
(blacklisted_names: Vec<String> = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect()),

‎clippy_utils/src/sugg.rs

+25-2
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,15 @@
22
#![deny(clippy::missing_docs_in_private_items)]
33

44
use crate::higher;
5-
use crate::source::{snippet, snippet_opt, snippet_with_macro_callsite};
5+
use crate::source::{snippet, snippet_opt, snippet_with_context, snippet_with_macro_callsite};
66
use rustc_ast::util::parser::AssocOp;
77
use rustc_ast::{ast, token};
88
use rustc_ast_pretty::pprust::token_kind_to_string;
99
use rustc_errors::Applicability;
1010
use rustc_hir as hir;
1111
use rustc_lint::{EarlyContext, LateContext, LintContext};
1212
use rustc_span::source_map::{CharPos, Span};
13-
use rustc_span::{BytePos, Pos};
13+
use rustc_span::{BytePos, Pos, SyntaxContext};
1414
use std::borrow::Cow;
1515
use std::convert::TryInto;
1616
use std::fmt::Display;
@@ -90,6 +90,29 @@ impl<'a> Sugg<'a> {
9090
Self::hir_from_snippet(expr, snippet)
9191
}
9292

93+
/// Same as `hir`, but first walks the span up to the given context. This will result in the
94+
/// macro call, rather then the expansion, if the span is from a child context. If the span is
95+
/// not from a child context, it will be used directly instead.
96+
///
97+
/// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR
98+
/// node would result in `box []`. If given the context of the address of expression, this
99+
/// function will correctly get a snippet of `vec![]`.
100+
pub fn hir_with_context(
101+
cx: &LateContext<'_>,
102+
expr: &hir::Expr<'_>,
103+
ctxt: SyntaxContext,
104+
default: &'a str,
105+
applicability: &mut Applicability,
106+
) -> Self {
107+
let (snippet, in_macro) = snippet_with_context(cx, expr.span, ctxt, default, applicability);
108+
109+
if in_macro {
110+
Sugg::NonParen(snippet)
111+
} else {
112+
Self::hir_from_snippet(expr, snippet)
113+
}
114+
}
115+
93116
/// Generate a suggestion for an expression with the given snippet. This is used by the `hir_*`
94117
/// function variants of `Sugg`, since these use different snippet functions.
95118
fn hir_from_snippet(expr: &hir::Expr<'_>, snippet: Cow<'a, str>) -> Self {

‎tests/ui/manual_str_repeat.fixed

+38-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// run-rustfix
22

3+
#![feature(custom_inner_attributes)]
34
#![warn(clippy::manual_str_repeat)]
45

5-
use std::iter::repeat;
6+
use std::borrow::Cow;
7+
use std::iter::{repeat, FromIterator};
68

79
fn main() {
810
let _: String = "test".repeat(10);
@@ -17,8 +19,8 @@ fn main() {
1719
macro_rules! m {
1820
($e:expr) => {{ $e }};
1921
}
20-
21-
let _: String = m!("test").repeat(m!(count));
22+
// FIXME: macro args are fine
23+
let _: String = repeat(m!("test")).take(m!(count)).collect();
2224

2325
let x = &x;
2426
let _: String = (*x).repeat(count);
@@ -28,4 +30,37 @@ fn main() {
2830
}
2931
// Don't lint, repeat is from a macro.
3032
let _: String = repeat_m!("test").take(count).collect();
33+
34+
let x: Box<str> = Box::from("test");
35+
let _: String = x.repeat(count);
36+
37+
#[derive(Clone)]
38+
struct S;
39+
impl FromIterator<Box<S>> for String {
40+
fn from_iter<T: IntoIterator<Item = Box<S>>>(_: T) -> Self {
41+
Self::new()
42+
}
43+
}
44+
// Don't lint, wrong box type
45+
let _: String = repeat(Box::new(S)).take(count).collect();
46+
47+
let _: String = Cow::Borrowed("test").repeat(count);
48+
49+
let x = "x".to_owned();
50+
let _: String = x.repeat(count);
51+
52+
let x = 'x';
53+
// Don't lint, not char literal
54+
let _: String = repeat(x).take(count).collect();
55+
}
56+
57+
fn _msrv_1_15() {
58+
#![clippy::msrv = "1.15"]
59+
// `str::repeat` was stabilized in 1.16. Do not lint this
60+
let _: String = std::iter::repeat("test").take(10).collect();
61+
}
62+
63+
fn _msrv_1_16() {
64+
#![clippy::msrv = "1.16"]
65+
let _: String = "test".repeat(10);
3166
}

‎tests/ui/manual_str_repeat.rs

+37-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// run-rustfix
22

3+
#![feature(custom_inner_attributes)]
34
#![warn(clippy::manual_str_repeat)]
45

5-
use std::iter::repeat;
6+
use std::borrow::Cow;
7+
use std::iter::{repeat, FromIterator};
68

79
fn main() {
810
let _: String = std::iter::repeat("test").take(10).collect();
@@ -17,7 +19,7 @@ fn main() {
1719
macro_rules! m {
1820
($e:expr) => {{ $e }};
1921
}
20-
22+
// FIXME: macro args are fine
2123
let _: String = repeat(m!("test")).take(m!(count)).collect();
2224

2325
let x = &x;
@@ -28,4 +30,37 @@ fn main() {
2830
}
2931
// Don't lint, repeat is from a macro.
3032
let _: String = repeat_m!("test").take(count).collect();
33+
34+
let x: Box<str> = Box::from("test");
35+
let _: String = repeat(x).take(count).collect();
36+
37+
#[derive(Clone)]
38+
struct S;
39+
impl FromIterator<Box<S>> for String {
40+
fn from_iter<T: IntoIterator<Item = Box<S>>>(_: T) -> Self {
41+
Self::new()
42+
}
43+
}
44+
// Don't lint, wrong box type
45+
let _: String = repeat(Box::new(S)).take(count).collect();
46+
47+
let _: String = repeat(Cow::Borrowed("test")).take(count).collect();
48+
49+
let x = "x".to_owned();
50+
let _: String = repeat(x).take(count).collect();
51+
52+
let x = 'x';
53+
// Don't lint, not char literal
54+
let _: String = repeat(x).take(count).collect();
55+
}
56+
57+
fn _msrv_1_15() {
58+
#![clippy::msrv = "1.15"]
59+
// `str::repeat` was stabilized in 1.16. Do not lint this
60+
let _: String = std::iter::repeat("test").take(10).collect();
61+
}
62+
63+
fn _msrv_1_16() {
64+
#![clippy::msrv = "1.16"]
65+
let _: String = std::iter::repeat("test").take(10).collect();
3166
}

‎tests/ui/manual_str_repeat.stderr

+30-12
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,64 @@
11
error: manual implementation of `str::repeat` using iterators
2-
--> $DIR/manual_str_repeat.rs:8:21
2+
--> $DIR/manual_str_repeat.rs:10:21
33
|
44
LL | let _: String = std::iter::repeat("test").take(10).collect();
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"test".repeat(10)`
66
|
77
= note: `-D clippy::manual-str-repeat` implied by `-D warnings`
88

99
error: manual implementation of `str::repeat` using iterators
10-
--> $DIR/manual_str_repeat.rs:9:21
10+
--> $DIR/manual_str_repeat.rs:11:21
1111
|
1212
LL | let _: String = std::iter::repeat('x').take(10).collect();
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"x".repeat(10)`
1414

1515
error: manual implementation of `str::repeat` using iterators
16-
--> $DIR/manual_str_repeat.rs:10:21
16+
--> $DIR/manual_str_repeat.rs:12:21
1717
|
1818
LL | let _: String = std::iter::repeat('/'').take(10).collect();
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"'".repeat(10)`
2020

2121
error: manual implementation of `str::repeat` using iterators
22-
--> $DIR/manual_str_repeat.rs:11:21
22+
--> $DIR/manual_str_repeat.rs:13:21
2323
|
2424
LL | let _: String = std::iter::repeat('"').take(10).collect();
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"/"".repeat(10)`
2626

2727
error: manual implementation of `str::repeat` using iterators
28-
--> $DIR/manual_str_repeat.rs:15:13
28+
--> $DIR/manual_str_repeat.rs:17:13
2929
|
3030
LL | let _ = repeat(x).take(count + 2).collect::<String>();
3131
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count + 2)`
3232

3333
error: manual implementation of `str::repeat` using iterators
34-
--> $DIR/manual_str_repeat.rs:21:21
34+
--> $DIR/manual_str_repeat.rs:26:21
3535
|
36-
LL | let _: String = repeat(m!("test")).take(m!(count)).collect();
37-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `m!("test").repeat(m!(count))`
36+
LL | let _: String = repeat(*x).take(count).collect();
37+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)`
3838

3939
error: manual implementation of `str::repeat` using iterators
40-
--> $DIR/manual_str_repeat.rs:24:21
40+
--> $DIR/manual_str_repeat.rs:35:21
4141
|
42-
LL | let _: String = repeat(*x).take(count).collect();
43-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(*x).repeat(count)`
42+
LL | let _: String = repeat(x).take(count).collect();
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count)`
44+
45+
error: manual implementation of `str::repeat` using iterators
46+
--> $DIR/manual_str_repeat.rs:47:21
47+
|
48+
LL | let _: String = repeat(Cow::Borrowed("test")).take(count).collect();
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `Cow::Borrowed("test").repeat(count)`
50+
51+
error: manual implementation of `str::repeat` using iterators
52+
--> $DIR/manual_str_repeat.rs:50:21
53+
|
54+
LL | let _: String = repeat(x).take(count).collect();
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `x.repeat(count)`
56+
57+
error: manual implementation of `str::repeat` using iterators
58+
--> $DIR/manual_str_repeat.rs:65:21
59+
|
60+
LL | let _: String = std::iter::repeat("test").take(10).collect();
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"test".repeat(10)`
4462

45-
error: aborting due to 7 previous errors
63+
error: aborting due to 10 previous errors
4664

0 commit comments

Comments
 (0)