Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

single_char_insert_str: lint using insert_str() on single-char literals and suggest insert() #6037

Merged
merged 5 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Current beta, release 2020-11-19
* [`map_err_ignore`] [#5998](https://github.com/rust-lang/rust-clippy/pull/5998)
* [`rc_buffer`] [#6044](https://github.com/rust-lang/rust-clippy/pull/6044)
* [`to_string_in_display`] [#5831](https://github.com/rust-lang/rust-clippy/pull/5831)
* [`single_char_push_str`] [#5881](https://github.com/rust-lang/rust-clippy/pull/5881)
* `single_char_push_str` [#5881](https://github.com/rust-lang/rust-clippy/pull/5881)

### Moves and Deprecations

Expand Down Expand Up @@ -1939,8 +1939,8 @@ Released 2018-09-13
[`should_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_assert_eq
[`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait
[`similar_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#similar_names
[`single_char_add_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
[`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
[`single_char_push_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_push_str
[`single_component_path_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports
[`single_element_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop
[`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
Expand Down
7 changes: 4 additions & 3 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -713,8 +713,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::RESULT_MAP_OR_INTO_OPTION,
&methods::SEARCH_IS_SOME,
&methods::SHOULD_IMPLEMENT_TRAIT,
&methods::SINGLE_CHAR_ADD_STR,
&methods::SINGLE_CHAR_PATTERN,
&methods::SINGLE_CHAR_PUSH_STR,
&methods::SKIP_WHILE_NEXT,
&methods::STRING_EXTEND_CHARS,
&methods::SUSPICIOUS_MAP,
Expand Down Expand Up @@ -1438,8 +1438,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
LintId::of(&methods::SEARCH_IS_SOME),
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::SINGLE_CHAR_ADD_STR),
LintId::of(&methods::SINGLE_CHAR_PATTERN),
LintId::of(&methods::SINGLE_CHAR_PUSH_STR),
LintId::of(&methods::SKIP_WHILE_NEXT),
LintId::of(&methods::STRING_EXTEND_CHARS),
LintId::of(&methods::SUSPICIOUS_MAP),
Expand Down Expand Up @@ -1631,7 +1631,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::SINGLE_CHAR_PUSH_STR),
LintId::of(&methods::SINGLE_CHAR_ADD_STR),
ebroto marked this conversation as resolved.
Show resolved Hide resolved
LintId::of(&methods::STRING_EXTEND_CHARS),
LintId::of(&methods::UNNECESSARY_FOLD),
LintId::of(&methods::UNNECESSARY_LAZY_EVALUATIONS),
Expand Down Expand Up @@ -1958,6 +1958,7 @@ pub fn register_renamed(ls: &mut rustc_lint::LintStore) {
ls.register_renamed("clippy::for_loop_over_result", "clippy::for_loops_over_fallibles");
ls.register_renamed("clippy::identity_conversion", "clippy::useless_conversion");
ls.register_renamed("clippy::zero_width_space", "clippy::invisible_characters");
ls.register_renamed("clippy::single_char_push_str", "clippy::single_char_add_str");
}

// only exists to let the dogfood integration test works.
Expand Down
41 changes: 33 additions & 8 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1290,8 +1290,8 @@ declare_clippy_lint! {
}

declare_clippy_lint! {
/// **What it does:** Warns when using `push_str` with a single-character string literal,
/// and `push` with a `char` would work fine.
/// **What it does:** Warns when using `push_str`/`insert_str` with a single-character string literal
/// where `push`/`insert` with a `char` would work fine.
///
/// **Why is this bad?** It's less clear that we are pushing a single character.
///
Expand All @@ -1300,16 +1300,18 @@ declare_clippy_lint! {
/// **Example:**
/// ```rust
/// let mut string = String::new();
/// string.insert_str(0, "R");
/// string.push_str("R");
/// ```
/// Could be written as
/// ```rust
/// let mut string = String::new();
/// string.insert(0, 'R');
/// string.push('R');
/// ```
pub SINGLE_CHAR_PUSH_STR,
pub SINGLE_CHAR_ADD_STR,
style,
"`push_str()` used with a single-character string literal as parameter"
"`push_str()` or `insert_str()` used with a single-character string literal as parameter"
}

declare_clippy_lint! {
Expand Down Expand Up @@ -1390,7 +1392,7 @@ declare_lint_pass!(Methods => [
INEFFICIENT_TO_STRING,
NEW_RET_NO_SELF,
SINGLE_CHAR_PATTERN,
SINGLE_CHAR_PUSH_STR,
SINGLE_CHAR_ADD_STR,
SEARCH_IS_SOME,
FILTER_NEXT,
SKIP_WHILE_NEXT,
Expand Down Expand Up @@ -1521,6 +1523,8 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
if match_def_path(cx, fn_def_id, &paths::PUSH_STR) {
lint_single_char_push_string(cx, expr, args);
} else if match_def_path(cx, fn_def_id, &paths::INSERT_STR) {
lint_single_char_insert_string(cx, expr, args);
}
}

Expand Down Expand Up @@ -3202,7 +3206,7 @@ fn get_hint_if_single_char_arg(
if let hir::ExprKind::Lit(lit) = &arg.kind;
if let ast::LitKind::Str(r, style) = lit.node;
let string = r.as_str();
if string.len() == 1;
if string.chars().count() == 1;
then {
let snip = snippet_with_applicability(cx, arg.span, &string, applicability);
let ch = if let ast::StrStyle::Raw(nhash) = style {
Expand Down Expand Up @@ -3241,11 +3245,12 @@ fn lint_single_char_pattern(cx: &LateContext<'_>, _expr: &hir::Expr<'_>, arg: &h
fn lint_single_char_push_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
let mut applicability = Applicability::MachineApplicable;
if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[1], &mut applicability) {
let base_string_snippet = snippet_with_applicability(cx, args[0].span, "..", &mut applicability);
let base_string_snippet =
snippet_with_applicability(cx, args[0].span.source_callsite(), "..", &mut applicability);
let sugg = format!("{}.push({})", base_string_snippet, extension_string);
span_lint_and_sugg(
cx,
SINGLE_CHAR_PUSH_STR,
SINGLE_CHAR_ADD_STR,
expr.span,
"calling `push_str()` using a single-character string literal",
"consider using `push` with a character literal",
Expand All @@ -3255,6 +3260,26 @@ fn lint_single_char_push_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args
}
}

/// lint for length-1 `str`s as argument for `insert_str`
fn lint_single_char_insert_string(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
let mut applicability = Applicability::MachineApplicable;
if let Some(extension_string) = get_hint_if_single_char_arg(cx, &args[2], &mut applicability) {
let base_string_snippet =
snippet_with_applicability(cx, args[0].span.source_callsite(), "_", &mut applicability);
let pos_arg = snippet_with_applicability(cx, args[1].span, "..", &mut applicability);
let sugg = format!("{}.insert({}, {})", base_string_snippet, pos_arg, extension_string);
span_lint_and_sugg(
cx,
SINGLE_CHAR_ADD_STR,
expr.span,
"calling `insert_str()` using a single-character string literal",
"consider using `insert` with a character literal",
sugg,
applicability,
);
}
}

/// Checks for the `USELESS_ASREF` lint.
fn lint_asref(cx: &LateContext<'_>, expr: &hir::Expr<'_>, call_name: &str, as_ref_args: &[hir::Expr<'_>]) {
// when we get here, we've already checked that the call name is "as_ref" or "as_mut"
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/utils/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ pub const HASHMAP_ENTRY: [&str; 5] = ["std", "collections", "hash", "map", "Entr
pub const HASHSET: [&str; 5] = ["std", "collections", "hash", "set", "HashSet"];
pub const INDEX: [&str; 3] = ["core", "ops", "Index"];
pub const INDEX_MUT: [&str; 3] = ["core", "ops", "IndexMut"];
pub const INSERT_STR: [&str; 4] = ["alloc", "string", "String", "insert_str"];
pub const INTO: [&str; 3] = ["core", "convert", "Into"];
pub const INTO_ITERATOR: [&str; 5] = ["core", "iter", "traits", "collect", "IntoIterator"];
pub const IO_READ: [&str; 3] = ["std", "io", "Read"];
Expand Down
12 changes: 6 additions & 6 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2147,16 +2147,16 @@ vec![
module: "non_expressive_names",
},
Lint {
name: "single_char_pattern",
group: "perf",
desc: "using a single-character str where a char could be used, e.g., `_.split(\"x\")`",
name: "single_char_add_str",
group: "style",
desc: "`push_str()` or `insert_str()` used with a single-character string literal as parameter",
deprecation: None,
module: "methods",
},
Lint {
name: "single_char_push_str",
group: "style",
desc: "`push_str()` used with a single-character string literal as parameter",
name: "single_char_pattern",
group: "perf",
desc: "using a single-character str where a char could be used, e.g., `_.split(\"x\")`",
deprecation: None,
module: "methods",
},
Expand Down
45 changes: 45 additions & 0 deletions tests/ui/single_char_add_str.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// run-rustfix
#![warn(clippy::single_char_add_str)]

macro_rules! get_string {
() => {
String::from("Hello world!")
};
}

fn main() {
// `push_str` tests

let mut string = String::new();
string.push('R');
string.push('\'');

string.push('u');
string.push_str("st");
string.push_str("");
string.push('\x52');
string.push('\u{0052}');
string.push('a');

get_string!().push('ö');

// `insert_str` tests

let mut string = String::new();
string.insert(0, 'R');
string.insert(1, '\'');

string.insert(0, 'u');
string.insert_str(2, "st");
string.insert_str(0, "");
string.insert(0, '\x52');
string.insert(0, '\u{0052}');
let x: usize = 2;
string.insert(x, 'a');
const Y: usize = 1;
string.insert(Y, 'a');
string.insert(Y, '"');
string.insert(Y, '\'');

get_string!().insert(1, '?');
}
45 changes: 45 additions & 0 deletions tests/ui/single_char_add_str.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// run-rustfix
#![warn(clippy::single_char_add_str)]

macro_rules! get_string {
() => {
String::from("Hello world!")
};
}

fn main() {
// `push_str` tests

let mut string = String::new();
string.push_str("R");
string.push_str("'");

string.push('u');
string.push_str("st");
string.push_str("");
string.push_str("\x52");
string.push_str("\u{0052}");
string.push_str(r##"a"##);

get_string!().push_str("ö");

// `insert_str` tests

let mut string = String::new();
string.insert_str(0, "R");
string.insert_str(1, "'");

string.insert(0, 'u');
string.insert_str(2, "st");
string.insert_str(0, "");
string.insert_str(0, "\x52");
string.insert_str(0, "\u{0052}");
let x: usize = 2;
string.insert_str(x, r##"a"##);
const Y: usize = 1;
string.insert_str(Y, r##"a"##);
ebroto marked this conversation as resolved.
Show resolved Hide resolved
string.insert_str(Y, r##"""##);
string.insert_str(Y, r##"'"##);

get_string!().insert_str(1, "?");
}
94 changes: 94 additions & 0 deletions tests/ui/single_char_add_str.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:14:5
|
LL | string.push_str("R");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('R')`
|
= note: `-D clippy::single-char-add-str` implied by `-D warnings`

error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:15:5
|
LL | string.push_str("'");
| ^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/'')`

error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:20:5
|
LL | string.push_str("/x52");
| ^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/x52')`

error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:21:5
|
LL | string.push_str("/u{0052}");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('/u{0052}')`

error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:22:5
|
LL | string.push_str(r##"a"##);
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `string.push('a')`

error: calling `push_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:24:5
|
LL | get_string!().push_str("ö");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `push` with a character literal: `get_string!().push('ö')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:29:5
|
LL | string.insert_str(0, "R");
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(0, 'R')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:30:5
|
LL | string.insert_str(1, "'");
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(1, '/'')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:35:5
|
LL | string.insert_str(0, "/x52");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(0, '/x52')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:36:5
|
LL | string.insert_str(0, "/u{0052}");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(0, '/u{0052}')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:38:5
|
LL | string.insert_str(x, r##"a"##);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(x, 'a')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:40:5
|
LL | string.insert_str(Y, r##"a"##);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(Y, 'a')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:41:5
|
LL | string.insert_str(Y, r##"""##);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(Y, '"')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:42:5
|
LL | string.insert_str(Y, r##"'"##);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `string.insert(Y, '/'')`

error: calling `insert_str()` using a single-character string literal
--> $DIR/single_char_add_str.rs:44:5
|
LL | get_string!().insert_str(1, "?");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `insert` with a character literal: `get_string!().insert(1, '?')`

error: aborting due to 15 previous errors

12 changes: 3 additions & 9 deletions tests/ui/single_char_pattern.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,9 @@ fn main() {

let y = "x";
x.split(y);
// Not yet testing for multi-byte characters
// Changing `r.len() == 1` to `r.chars().count() == 1` in `lint_clippy::single_char_pattern`
// should have done this but produced an ICE
//
// We may not want to suggest changing these anyway
// See: https://github.com/rust-lang/rust-clippy/issues/650#issuecomment-184328984
x.split("ß");
x.split("ℝ");
x.split("💣");
x.split('ß');
x.split('ℝ');
x.split('💣');
// Can't use this lint for unicode code points which don't fit in a char
x.split("❤️");
x.contains('x');
Expand Down
Loading