Skip to content

Commit 868dba9

Browse files
committed
Auto merge of #9295 - Guilherme-Vasconcelos:manual-empty-string-creation, r=dswij
Add `manual_empty_string_creations` lint Closes #2972 - [x] Followed [lint naming conventions][lint_naming] - [x] Added passing UI tests (including committed `.stderr` file) - [x] `cargo test` passes locally - [x] Executed `cargo dev update_lints` - [x] Added lint documentation - [x] Run `cargo dev fmt` changelog: [`manual_empty_string_creations`]: Add lint for empty String not being created with `String::new()`
2 parents eeaaba3 + 1bf8841 commit 868dba9

32 files changed

+371
-38
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3830,6 +3830,7 @@ Released 2018-09-13
38303830
[`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert
38313831
[`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn
38323832
[`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits
3833+
[`manual_empty_string_creations`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_empty_string_creations
38333834
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
38343835
[`manual_find`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find
38353836
[`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map

clippy_dev/src/new_lint.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ fn to_camel_case(name: &str) -> String {
155155
name.split('_')
156156
.map(|s| {
157157
if s.is_empty() {
158-
String::from("")
158+
String::new()
159159
} else {
160160
[&s[0..1].to_uppercase(), &s[1..]].concat()
161161
}

clippy_lints/src/lib.register_all.rs

+1
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
124124
LintId::of(main_recursion::MAIN_RECURSION),
125125
LintId::of(manual_async_fn::MANUAL_ASYNC_FN),
126126
LintId::of(manual_bits::MANUAL_BITS),
127+
LintId::of(manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS),
127128
LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
128129
LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID),
129130
LintId::of(manual_retain::MANUAL_RETAIN),

clippy_lints/src/lib.register_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,7 @@ store.register_lints(&[
244244
manual_assert::MANUAL_ASSERT,
245245
manual_async_fn::MANUAL_ASYNC_FN,
246246
manual_bits::MANUAL_BITS,
247+
manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS,
247248
manual_instant_elapsed::MANUAL_INSTANT_ELAPSED,
248249
manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
249250
manual_ok_or::MANUAL_OK_OR,

clippy_lints/src/lib.register_style.rs

+1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
4444
LintId::of(main_recursion::MAIN_RECURSION),
4545
LintId::of(manual_async_fn::MANUAL_ASYNC_FN),
4646
LintId::of(manual_bits::MANUAL_BITS),
47+
LintId::of(manual_empty_string_creations::MANUAL_EMPTY_STRING_CREATIONS),
4748
LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
4849
LintId::of(map_clone::MAP_CLONE),
4950
LintId::of(match_result_ok::MATCH_RESULT_OK),

clippy_lints/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,7 @@ mod main_recursion;
273273
mod manual_assert;
274274
mod manual_async_fn;
275275
mod manual_bits;
276+
mod manual_empty_string_creations;
276277
mod manual_instant_elapsed;
277278
mod manual_non_exhaustive;
278279
mod manual_ok_or;
@@ -933,6 +934,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
933934
store.register_late_pass(|| Box::new(std_instead_of_core::StdReexports::default()));
934935
store.register_late_pass(|| Box::new(manual_instant_elapsed::ManualInstantElapsed));
935936
store.register_late_pass(|| Box::new(partialeq_to_none::PartialeqToNone));
937+
store.register_late_pass(|| Box::new(manual_empty_string_creations::ManualEmptyStringCreations));
936938
// add lints here, do not remove this comment, it's used in `new_lint`
937939
}
938940

clippy_lints/src/manual_async_fn.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ fn suggested_ret(cx: &LateContext<'_>, output: &Ty<'_>) -> Option<(&'static str,
192192
match output.kind {
193193
TyKind::Tup(tys) if tys.is_empty() => {
194194
let sugg = "remove the return type";
195-
Some((sugg, "".into()))
195+
Some((sugg, String::new()))
196196
},
197197
_ => {
198198
let sugg = "return the output of the future directly";
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
use clippy_utils::diagnostics::span_lint_and_sugg;
2+
use rustc_ast::LitKind;
3+
use rustc_errors::Applicability::MachineApplicable;
4+
use rustc_hir::{Expr, ExprKind, PathSegment, QPath, TyKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty;
7+
use rustc_session::{declare_lint_pass, declare_tool_lint};
8+
use rustc_span::{sym, symbol, Span};
9+
10+
declare_clippy_lint! {
11+
/// ### What it does
12+
///
13+
/// Checks for usage of `""` to create a `String`, such as `"".to_string()`, `"".to_owned()`,
14+
/// `String::from("")` and others.
15+
///
16+
/// ### Why is this bad?
17+
///
18+
/// Different ways of creating an empty string makes your code less standardized, which can
19+
/// be confusing.
20+
///
21+
/// ### Example
22+
/// ```rust
23+
/// let a = "".to_string();
24+
/// let b: String = "".into();
25+
/// ```
26+
/// Use instead:
27+
/// ```rust
28+
/// let a = String::new();
29+
/// let b = String::new();
30+
/// ```
31+
#[clippy::version = "1.65.0"]
32+
pub MANUAL_EMPTY_STRING_CREATIONS,
33+
style,
34+
"empty String is being created manually"
35+
}
36+
declare_lint_pass!(ManualEmptyStringCreations => [MANUAL_EMPTY_STRING_CREATIONS]);
37+
38+
impl LateLintPass<'_> for ManualEmptyStringCreations {
39+
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
40+
if expr.span.from_expansion() {
41+
return;
42+
}
43+
44+
let ty = cx.typeck_results().expr_ty(expr);
45+
match ty.kind() {
46+
ty::Adt(adt_def, _) if adt_def.is_struct() => {
47+
if !cx.tcx.is_diagnostic_item(sym::String, adt_def.did()) {
48+
return;
49+
}
50+
},
51+
_ => return,
52+
}
53+
54+
match expr.kind {
55+
ExprKind::Call(func, args) => {
56+
parse_call(cx, expr.span, func, args);
57+
},
58+
ExprKind::MethodCall(path_segment, args, _) => {
59+
parse_method_call(cx, expr.span, path_segment, args);
60+
},
61+
_ => (),
62+
}
63+
}
64+
}
65+
66+
/// Checks if an expression's kind corresponds to an empty &str.
67+
fn is_expr_kind_empty_str(expr_kind: &ExprKind<'_>) -> bool {
68+
if let ExprKind::Lit(lit) = expr_kind &&
69+
let LitKind::Str(value, _) = lit.node &&
70+
value == symbol::kw::Empty
71+
{
72+
return true;
73+
}
74+
75+
false
76+
}
77+
78+
/// Emits the `MANUAL_EMPTY_STRING_CREATION` warning and suggests the appropriate fix.
79+
fn warn_then_suggest(cx: &LateContext<'_>, span: Span) {
80+
span_lint_and_sugg(
81+
cx,
82+
MANUAL_EMPTY_STRING_CREATIONS,
83+
span,
84+
"empty String is being created manually",
85+
"consider using",
86+
"String::new()".into(),
87+
MachineApplicable,
88+
);
89+
}
90+
91+
/// Tries to parse an expression as a method call, emiting the warning if necessary.
92+
fn parse_method_call(cx: &LateContext<'_>, span: Span, path_segment: &PathSegment<'_>, args: &[Expr<'_>]) {
93+
if args.is_empty() {
94+
// When parsing TryFrom::try_from(...).expect(...), we will have more than 1 arg.
95+
return;
96+
}
97+
98+
let ident = path_segment.ident.as_str();
99+
let method_arg_kind = &args[0].kind;
100+
if ["to_string", "to_owned", "into"].contains(&ident) && is_expr_kind_empty_str(method_arg_kind) {
101+
warn_then_suggest(cx, span);
102+
} else if let ExprKind::Call(func, args) = method_arg_kind {
103+
// If our first argument is a function call itself, it could be an `unwrap`-like function.
104+
// E.g. String::try_from("hello").unwrap(), TryFrom::try_from("").expect("hello"), etc.
105+
parse_call(cx, span, func, args);
106+
}
107+
}
108+
109+
/// Tries to parse an expression as a function call, emiting the warning if necessary.
110+
fn parse_call(cx: &LateContext<'_>, span: Span, func: &Expr<'_>, args: &[Expr<'_>]) {
111+
if args.len() != 1 {
112+
return;
113+
}
114+
115+
let arg_kind = &args[0].kind;
116+
if let ExprKind::Path(qpath) = &func.kind {
117+
if let QPath::TypeRelative(_, _) = qpath {
118+
// String::from(...) or String::try_from(...)
119+
if let QPath::TypeRelative(ty, path_seg) = qpath &&
120+
[sym::from, sym::try_from].contains(&path_seg.ident.name) &&
121+
let TyKind::Path(qpath) = &ty.kind &&
122+
let QPath::Resolved(_, path) = qpath &&
123+
let [path_seg] = path.segments &&
124+
path_seg.ident.name == sym::String &&
125+
is_expr_kind_empty_str(arg_kind)
126+
{
127+
warn_then_suggest(cx, span);
128+
}
129+
} else if let QPath::Resolved(_, path) = qpath {
130+
// From::from(...) or TryFrom::try_from(...)
131+
if let [path_seg1, path_seg2] = path.segments &&
132+
is_expr_kind_empty_str(arg_kind) && (
133+
(path_seg1.ident.name == sym::From && path_seg2.ident.name == sym::from) ||
134+
(path_seg1.ident.name == sym::TryFrom && path_seg2.ident.name == sym::try_from)
135+
)
136+
{
137+
warn_then_suggest(cx, span);
138+
}
139+
}
140+
}
141+
}

clippy_lints/src/methods/option_map_unwrap_or.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ pub(super) fn check<'tcx>(
7878
map_span,
7979
String::from(if unwrap_snippet_none { "and_then" } else { "map_or" }),
8080
),
81-
(expr.span.with_lo(unwrap_recv.span.hi()), String::from("")),
81+
(expr.span.with_lo(unwrap_recv.span.hi()), String::new()),
8282
];
8383

8484
if !unwrap_snippet_none {

clippy_lints/src/misc_early/unneeded_wildcard_pattern.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ fn span_lint(cx: &EarlyContext<'_>, span: Span, only_one: bool) {
4646
"these patterns are unneeded as the `..` pattern can match those elements"
4747
},
4848
if only_one { "remove it" } else { "remove them" },
49-
"".to_string(),
49+
String::new(),
5050
Applicability::MachineApplicable,
5151
);
5252
}

clippy_lints/src/unnecessary_wraps.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryWraps {
130130
(
131131
ret_expr.span,
132132
if inner_type.is_unit() {
133-
"".to_string()
133+
String::new()
134134
} else {
135135
snippet(cx, arg.span.source_callsite(), "..").to_string()
136136
}

clippy_utils/src/msrvs.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,8 @@ msrv_aliases! {
3232
1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
3333
1,28,0 { FROM_BOOL }
3434
1,26,0 { RANGE_INCLUSIVE, STRING_RETAIN }
35+
1,24,0 { IS_ASCII_DIGIT }
3536
1,18,0 { HASH_MAP_RETAIN, HASH_SET_RETAIN }
3637
1,17,0 { FIELD_INIT_SHORTHAND, STATIC_IN_CONST, EXPECT_ERR }
3738
1,16,0 { STR_REPEAT }
38-
1,24,0 { IS_ASCII_DIGIT }
3939
}

tests/ui/case_sensitive_file_extension_comparisons.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -14,31 +14,31 @@ fn is_rust_file(filename: &str) -> bool {
1414

1515
fn main() {
1616
// std::string::String and &str should trigger the lint failure with .ext12
17-
let _ = String::from("").ends_with(".ext12");
17+
let _ = String::new().ends_with(".ext12");
1818
let _ = "str".ends_with(".ext12");
1919

2020
// The test struct should not trigger the lint failure with .ext12
2121
TestStruct {}.ends_with(".ext12");
2222

2323
// std::string::String and &str should trigger the lint failure with .EXT12
24-
let _ = String::from("").ends_with(".EXT12");
24+
let _ = String::new().ends_with(".EXT12");
2525
let _ = "str".ends_with(".EXT12");
2626

2727
// The test struct should not trigger the lint failure with .EXT12
2828
TestStruct {}.ends_with(".EXT12");
2929

3030
// Should not trigger the lint failure with .eXT12
31-
let _ = String::from("").ends_with(".eXT12");
31+
let _ = String::new().ends_with(".eXT12");
3232
let _ = "str".ends_with(".eXT12");
3333
TestStruct {}.ends_with(".eXT12");
3434

3535
// Should not trigger the lint failure with .EXT123 (too long)
36-
let _ = String::from("").ends_with(".EXT123");
36+
let _ = String::new().ends_with(".EXT123");
3737
let _ = "str".ends_with(".EXT123");
3838
TestStruct {}.ends_with(".EXT123");
3939

4040
// Shouldn't fail if it doesn't start with a dot
41-
let _ = String::from("").ends_with("a.ext");
41+
let _ = String::new().ends_with("a.ext");
4242
let _ = "str".ends_with("a.extA");
4343
TestStruct {}.ends_with("a.ext");
4444
}

tests/ui/case_sensitive_file_extension_comparisons.stderr

+6-6
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,10 @@ LL | filename.ends_with(".rs")
88
= help: consider using a case-insensitive comparison instead
99

1010
error: case-sensitive file extension comparison
11-
--> $DIR/case_sensitive_file_extension_comparisons.rs:17:30
11+
--> $DIR/case_sensitive_file_extension_comparisons.rs:17:27
1212
|
13-
LL | let _ = String::from("").ends_with(".ext12");
14-
| ^^^^^^^^^^^^^^^^^^^
13+
LL | let _ = String::new().ends_with(".ext12");
14+
| ^^^^^^^^^^^^^^^^^^^
1515
|
1616
= help: consider using a case-insensitive comparison instead
1717

@@ -24,10 +24,10 @@ LL | let _ = "str".ends_with(".ext12");
2424
= help: consider using a case-insensitive comparison instead
2525

2626
error: case-sensitive file extension comparison
27-
--> $DIR/case_sensitive_file_extension_comparisons.rs:24:30
27+
--> $DIR/case_sensitive_file_extension_comparisons.rs:24:27
2828
|
29-
LL | let _ = String::from("").ends_with(".EXT12");
30-
| ^^^^^^^^^^^^^^^^^^^
29+
LL | let _ = String::new().ends_with(".EXT12");
30+
| ^^^^^^^^^^^^^^^^^^^
3131
|
3232
= help: consider using a case-insensitive comparison instead
3333

tests/ui/format.fixed

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ fn main() {
3333
format!("foo {}", "bar");
3434
format!("{} bar", "foo");
3535

36-
let arg: String = "".to_owned();
36+
let arg = String::new();
3737
arg.to_string();
3838
format!("{:?}", arg); // Don't warn about debug.
3939
format!("{:8}", arg);

tests/ui/format.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ fn main() {
3535
format!("foo {}", "bar");
3636
format!("{} bar", "foo");
3737

38-
let arg: String = "".to_owned();
38+
let arg = String::new();
3939
format!("{}", arg);
4040
format!("{:?}", arg); // Don't warn about debug.
4141
format!("{:8}", arg);

tests/ui/identity_op.fixed

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ fn main() {
6868
&x;
6969
x;
7070

71-
let mut a = A("".into());
71+
let mut a = A(String::new());
7272
let b = a << 0; // no error: non-integer
7373

7474
1 * Meter; // no error: non-integer

tests/ui/identity_op.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ fn main() {
6868
&x >> 0;
6969
x >> &0;
7070

71-
let mut a = A("".into());
71+
let mut a = A(String::new());
7272
let b = a << 0; // no error: non-integer
7373

7474
1 * Meter; // no error: non-integer

0 commit comments

Comments
 (0)