diff --git a/README.md b/README.md index 05737f142..7fa0a1571 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 121 lints included in this crate: +There are 122 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -104,6 +104,7 @@ name [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value [should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait +[single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern) | warn | using a single-character str where a char could be used, e.g. `_.split("x")` [single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead [single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead [str_to_string](https://github.com/Manishearth/rust-clippy/wiki#str_to_string) | warn | using `to_string()` on a str, which should be `to_owned()` diff --git a/src/lib.rs b/src/lib.rs index bd13190f8..5f4f6e529 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -241,6 +241,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::OR_FUN_CALL, methods::SEARCH_IS_SOME, methods::SHOULD_IMPLEMENT_TRAIT, + methods::SINGLE_CHAR_PATTERN, methods::STR_TO_STRING, methods::STRING_TO_STRING, methods::WRONG_SELF_CONVENTION, diff --git a/src/methods.rs b/src/methods.rs index b71d2ffd7..39f69f63d 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -1,4 +1,6 @@ use rustc::lint::*; +use rustc::middle::const_eval::{ConstVal, eval_const_expr_partial}; +use rustc::middle::const_eval::EvalHint::ExprTypeChecked; use rustc::middle::subst::{Subst, TypeSpace}; use rustc::middle::ty; use rustc_front::hir::*; @@ -295,6 +297,20 @@ declare_lint! { pub NEW_RET_NO_SELF, Warn, "not returning `Self` in a `new` method" } +/// **What it does:** This lint checks for string methods that receive a single-character `str` as an argument, e.g. `_.split("x")`. +/// +/// **Why is this bad?** Performing these methods using a `char` is faster than using a `str`. +/// +/// **Known problems:** Does not catch multi-byte unicode characters. +/// +/// **Example:** `_.split("x")` could be `_.split('x')` +declare_lint! { + pub SINGLE_CHAR_PATTERN, + Warn, + "using a single-character str where a char could be used, e.g. \ + `_.split(\"x\")`" +} + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -312,7 +328,8 @@ impl LintPass for MethodsPass { CHARS_NEXT_CMP, CLONE_ON_COPY, CLONE_DOUBLE_REF, - NEW_RET_NO_SELF) + NEW_RET_NO_SELF, + SINGLE_CHAR_PATTERN) } } @@ -351,6 +368,11 @@ impl LateLintPass for MethodsPass { lint_clone_on_copy(cx, expr); lint_clone_double_ref(cx, expr, &args[0]); } + for &(method, pos) in &PATTERN_METHODS { + if name.node.as_str() == method && args.len() > pos { + lint_single_char_pattern(cx, expr, &args[pos]); + } + } } ExprBinary(op, ref lhs, ref rhs) if op.node == BiEq || op.node == BiNe => { if !lint_chars_next(cx, expr, lhs, rhs, op.node == BiEq) { @@ -819,6 +841,22 @@ fn lint_chars_next(cx: &LateContext, expr: &Expr, chain: &Expr, other: &Expr, eq false } +/// lint for length-1 `str`s for methods in `PATTERN_METHODS` +fn lint_single_char_pattern(cx: &LateContext, expr: &Expr, arg: &Expr) { + if let Ok(ConstVal::Str(r)) = eval_const_expr_partial(cx.tcx, arg, ExprTypeChecked, None) { + if r.len() == 1 { + let hint = snippet(cx, expr.span, "..").replace(&format!("\"{}\"", r), &format!("'{}'", r)); + span_lint_and_then(cx, + SINGLE_CHAR_PATTERN, + arg.span, + "single-character string constant used as pattern", + |db| { + db.span_suggestion(expr.span, "try using a char instead:", hint); + }); + } + } +} + /// Given a `Result` type, return its error type (`E`). fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { @@ -889,6 +927,28 @@ const TRAIT_METHODS: [(&'static str, usize, SelfKind, OutType, &'static str); 30 ("sub", 2, SelfKind::Value, OutType::Any, "std::ops::Sub"), ]; +#[cfg_attr(rustfmt, rustfmt_skip)] +const PATTERN_METHODS: [(&'static str, usize); 17] = [ + ("contains", 1), + ("starts_with", 1), + ("ends_with", 1), + ("find", 1), + ("rfind", 1), + ("split", 1), + ("rsplit", 1), + ("split_terminator", 1), + ("rsplit_terminator", 1), + ("splitn", 2), + ("rsplitn", 2), + ("matches", 1), + ("rmatches", 1), + ("match_indices", 1), + ("rmatch_indices", 1), + ("trim_left_matches", 1), + ("trim_right_matches", 1), +]; + + #[derive(Clone, Copy)] enum SelfKind { Value, diff --git a/src/misc_early.rs b/src/misc_early.rs index 59a0102aa..1999d9118 100644 --- a/src/misc_early.rs +++ b/src/misc_early.rs @@ -104,7 +104,7 @@ impl EarlyLintPass for MiscEarly { if let PatIdent(_, sp_ident, None) = arg.pat.node { let arg_name = sp_ident.node.to_string(); - if arg_name.starts_with("_") { + if arg_name.starts_with('_') { if let Some(correspondance) = registered_names.get(&arg_name[1..]) { span_lint(cx, DUPLICATE_UNDERSCORE_ARGUMENT, diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index afe056e3d..c450c9532 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(clippy, clippy_pedantic)] -#![allow(unused, print_stdout)] +#![allow(unused, print_stdout, non_ascii_literal)] use std::collections::BTreeMap; use std::collections::HashMap; @@ -355,3 +355,95 @@ fn clone_on_double_ref() { //~^^^ERROR using `clone` on a `Copy` type println!("{:p} {:p}",*y, z); } + +fn single_char_pattern() { + let x = "foo"; + x.split("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.split('x'); + + x.split("xx"); + + x.split('x'); + + let y = "x"; + x.split(y); + + // Not yet testing for multi-byte characters + // Changing `r.len() == 1` to `r.chars().count() == 1` in `lint_single_char_pattern` + // should have done this but produced an ICE + // + // We may not want to suggest changing these anyway + // See: https://github.com/Manishearth/rust-clippy/issues/650#issuecomment-184328984 + 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"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.contains('x'); + x.starts_with("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.starts_with('x'); + x.ends_with("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.ends_with('x'); + x.find("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.find('x'); + x.rfind("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rfind('x'); + x.rsplit("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rsplit('x'); + x.split_terminator("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.split_terminator('x'); + x.rsplit_terminator("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rsplit_terminator('x'); + x.splitn(0, "x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.splitn(0, 'x'); + x.rsplitn(0, "x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rsplitn(0, 'x'); + x.matches("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.matches('x'); + x.rmatches("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rmatches('x'); + x.match_indices("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.match_indices('x'); + x.rmatch_indices("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.rmatch_indices('x'); + x.trim_left_matches("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.trim_left_matches('x'); + x.trim_right_matches("x"); + //~^ ERROR single-character string constant used as pattern + //~| HELP try using a char instead: + //~| SUGGESTION x.trim_right_matches('x'); +}