Merge pull request #622 from mcarton/regex

Lint about trivial regexes

Author: Manishearth

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 112 lints included in this crate:
+There are 113 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -104,6 +104,7 @@ name
 [string_to_string](https://github.com/Manishearth/rust-clippy/wiki#string_to_string)                           | warn    | calling `String::to_string` which is inefficient
 [temporary_assignment](https://github.com/Manishearth/rust-clippy/wiki#temporary_assignment)                   | warn    | assignments to temporaries
 [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg)                           | warn    | An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take references with `&`.
+[trivial_regex](https://github.com/Manishearth/rust-clippy/wiki#trivial_regex)                                 | warn    | finds trivial regular expressions in `Regex::new(_)` invocations
 [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity)                             | warn    | usage of very complex types; recommends factoring out parts into `type` definitions
 [unicode_not_nfc](https://github.com/Manishearth/rust-clippy/wiki#unicode_not_nfc)                             | allow   | using a unicode literal not in NFC normal form (see http://www.unicode.org/reports/tr15/ for further information)
 [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp)                                           | warn    | comparing unit values (which is always `true` or `false`, respectively)

src/cyclomatic_complexity.rs

@@ -62,8 +62,8 @@ impl CyclomaticComplexity {
                 span_help_and_lint(cx,
                                    CYCLOMATIC_COMPLEXITY,
                                    span,
-                                   &format!("The function has a cyclomatic complexity of {}", rust_cc),
-                                   "You could split it up into multiple smaller functions");
+                                   &format!("the function has a cyclomatic complexity of {}", rust_cc),
+                                   "you could split it up into multiple smaller functions");
             }
         }
     }

src/lib.rs

@@ -257,6 +257,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         ranges::RANGE_STEP_BY_ZERO,
         ranges::RANGE_ZIP_WITH_LEN,
         regex::INVALID_REGEX,
+        regex::TRIVIAL_REGEX,
         returns::LET_AND_RETURN,
         returns::NEEDLESS_RETURN,
         strings::STRING_LIT_AS_BYTES,

src/open_options.rs

@@ -120,7 +120,7 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
                     span_lint(cx,
                               NONSENSICAL_OPEN_OPTIONS,
                               span,
-                              "The method \"create\" is called more than once");
+                              "the method \"create\" is called more than once");
                 } else {
                     create = true
                 }
@@ -131,7 +131,7 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
                     span_lint(cx,
                               NONSENSICAL_OPEN_OPTIONS,
                               span,
-                              "The method \"append\" is called more than once");
+                              "the method \"append\" is called more than once");
                 } else {
                     append = true
                 }
@@ -142,7 +142,7 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
                     span_lint(cx,
                               NONSENSICAL_OPEN_OPTIONS,
                               span,
-                              "The method \"truncate\" is called more than once");
+                              "the method \"truncate\" is called more than once");
                 } else {
                     truncate = true
                 }
@@ -153,7 +153,7 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
                     span_lint(cx,
                               NONSENSICAL_OPEN_OPTIONS,
                               span,
-                              "The method \"read\" is called more than once");
+                              "the method \"read\" is called more than once");
                 } else {
                     read = true
                 }
@@ -164,7 +164,7 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
                     span_lint(cx,
                               NONSENSICAL_OPEN_OPTIONS,
                               span,
-                              "The method \"write\" is called more than once");
+                              "the method \"write\" is called more than once");
                 } else {
                     write = true
                 }
@@ -174,12 +174,12 @@ fn check_open_options(cx: &LateContext, options: &[(OpenOption, Argument)], span
     }
 
     if read && truncate && read_arg && truncate_arg {
-        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "File opened with \"truncate\" and \"read\"");
+        span_lint(cx, NONSENSICAL_OPEN_OPTIONS, span, "file opened with \"truncate\" and \"read\"");
     }
     if append && truncate && append_arg && truncate_arg {
         span_lint(cx,
                   NONSENSICAL_OPEN_OPTIONS,
                   span,
-                  "File opened with \"append\" and \"truncate\"");
+                  "file opened with \"append\" and \"truncate\"");
     }
 }

src/regex.rs

@@ -8,7 +8,7 @@ use rustc::middle::const_eval::{eval_const_expr_partial, ConstVal};
 use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
 use rustc::lint::*;
 
-use utils::{match_path, REGEX_NEW_PATH, span_lint};
+use utils::{match_path, REGEX_NEW_PATH, span_lint, span_help_and_lint};
 
 /// **What it does:** This lint checks `Regex::new(_)` invocations for correct regex syntax. It is `deny` by default.
 ///
@@ -23,12 +23,26 @@ declare_lint! {
     "finds invalid regular expressions in `Regex::new(_)` invocations"
 }
 
+/// **What it does:** This lint checks for `Regex::new(_)` invocations with trivial regex.
+///
+/// **Why is this bad?** This can likely be replaced by `==` or `str::starts_with`,
+/// `str::ends_with` or `std::contains` or other `str` methods.
+///
+/// **Known problems:** None.
+///
+/// **Example:** `Regex::new("^foobar")`
+declare_lint! {
+    pub TRIVIAL_REGEX,
+    Warn,
+    "finds trivial regular expressions in `Regex::new(_)` invocations"
+}
+
 #[derive(Copy,Clone)]
 pub struct RegexPass;
 
 impl LintPass for RegexPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(INVALID_REGEX)
+        lint_array!(INVALID_REGEX, TRIVIAL_REGEX)
     }
 }
 
@@ -45,22 +59,29 @@ impl LateLintPass for RegexPass {
                         span_lint(cx,
                                   INVALID_REGEX,
                                   str_span(args[0].span, &r, e.position()),
-                                  &format!("Regex syntax error: {}",
+                                  &format!("regex syntax error: {}",
                                            e.description()));
                     }
+                    else if let Some(repl) = is_trivial_regex(r) {
+                        span_help_and_lint(cx, TRIVIAL_REGEX, args[0].span,
+                                           &"trivial regex",
+                                           &format!("consider using {}", repl));
+                    }
                 }
-            } else {
-                if_let_chain!{[
-                    let Some(r) = const_str(cx, &*args[0]),
-                    let Err(e) = regex_syntax::Expr::parse(&r)
-                ], {
+            } else if let Some(r) = const_str(cx, &*args[0]) {
+                if let Err(e) = regex_syntax::Expr::parse(&r) {
                     span_lint(cx,
                               INVALID_REGEX,
                               args[0].span,
-                              &format!("Regex syntax error on position {}: {}",
+                              &format!("regex syntax error on position {}: {}",
                                        e.position(),
                                        e.description()));
-                }}
+                }
+                else if let Some(repl) = is_trivial_regex(&r) {
+                    span_help_and_lint(cx, TRIVIAL_REGEX, args[0].span,
+                                       &"trivial regex",
+                                       &format!("{}", repl));
+                }
             }
         }}
     }
@@ -81,3 +102,26 @@ fn const_str(cx: &LateContext, e: &Expr) -> Option<InternedString> {
         _ => None
     }
 }
+
+fn is_trivial_regex(s: &str) -> Option<&'static str> {
+    // some unlikely but valid corner cases
+    match s {
+        "" | "^" | "$" => return Some("the regex is unlikely to be useful as it is"),
+        "^$" => return Some("consider using `str::is_empty`"),
+        _ => (),
+    }
+
+    let (start, end, repl) = match (s.starts_with('^'), s.ends_with('$')) {
+        (true, true) => (1, s.len()-1, "consider using `==` on `str`s"),
+        (false, true) => (0, s.len()-1, "consider using `str::ends_with`"),
+        (true, false) => (1, s.len(), "consider using `str::starts_with`"),
+        (false, false) => (0, s.len(), "consider using `str::contains`"),
+    };
+
+    if !s.chars().take(end).skip(start).any(regex_syntax::is_punct) {
+        Some(repl)
+    }
+    else {
+        None
+    }
+}

tests/compile-fail/cyclomatic_complexity.rs

@@ -5,7 +5,7 @@
 #![allow(unused)]
 
 
-fn main() { //~ERROR The function has a cyclomatic complexity of 28
+fn main() { //~ERROR the function has a cyclomatic complexity of 28
     if true {
         println!("a");
     }
@@ -90,7 +90,7 @@ fn main() { //~ERROR The function has a cyclomatic complexity of 28
 }
 
 #[cyclomatic_complexity = "0"]
-fn kaboom() {  //~ ERROR: The function has a cyclomatic complexity of 8
+fn kaboom() {  //~ ERROR: the function has a cyclomatic complexity of 8
     let n = 0;
     'a: for i in 0..20 {
         'b: for j in i..20 {
@@ -136,7 +136,7 @@ fn bloo() {
 }
 
 #[cyclomatic_complexity = "0"]
-fn baa() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn baa() { //~ ERROR: the function has a cyclomatic complexity of 2
     let x = || match 99 {
         0 => true,
         1 => false,
@@ -154,15 +154,15 @@ fn baa() { //~ ERROR: The function has a cyclomatic complexity of 2
 }
 
 #[cyclomatic_complexity = "0"]
-fn bar() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn bar() { //~ ERROR: the function has a cyclomatic complexity of 2
     match 99 {
         0 => println!("hi"),
         _ => println!("bye"),
     }
 }
 
 #[cyclomatic_complexity = "0"]
-fn barr() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn barr() { //~ ERROR: the function has a cyclomatic complexity of 2
     match 99 {
         0 => println!("hi"),
         1 => println!("bla"),
@@ -172,7 +172,7 @@ fn barr() { //~ ERROR: The function has a cyclomatic complexity of 2
 }
 
 #[cyclomatic_complexity = "0"]
-fn barr2() { //~ ERROR: The function has a cyclomatic complexity of 3
+fn barr2() { //~ ERROR: the function has a cyclomatic complexity of 3
     match 99 {
         0 => println!("hi"),
         1 => println!("bla"),
@@ -188,7 +188,7 @@ fn barr2() { //~ ERROR: The function has a cyclomatic complexity of 3
 }
 
 #[cyclomatic_complexity = "0"]
-fn barrr() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn barrr() { //~ ERROR: the function has a cyclomatic complexity of 2
     match 99 {
         0 => println!("hi"),
         1 => panic!("bla"),
@@ -198,7 +198,7 @@ fn barrr() { //~ ERROR: The function has a cyclomatic complexity of 2
 }
 
 #[cyclomatic_complexity = "0"]
-fn barrr2() { //~ ERROR: The function has a cyclomatic complexity of 3
+fn barrr2() { //~ ERROR: the function has a cyclomatic complexity of 3
     match 99 {
         0 => println!("hi"),
         1 => panic!("bla"),
@@ -214,7 +214,7 @@ fn barrr2() { //~ ERROR: The function has a cyclomatic complexity of 3
 }
 
 #[cyclomatic_complexity = "0"]
-fn barrrr() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn barrrr() { //~ ERROR: the function has a cyclomatic complexity of 2
     match 99 {
         0 => println!("hi"),
         1 => println!("bla"),
@@ -224,7 +224,7 @@ fn barrrr() { //~ ERROR: The function has a cyclomatic complexity of 2
 }
 
 #[cyclomatic_complexity = "0"]
-fn barrrr2() { //~ ERROR: The function has a cyclomatic complexity of 3
+fn barrrr2() { //~ ERROR: the function has a cyclomatic complexity of 3
     match 99 {
         0 => println!("hi"),
         1 => println!("bla"),
@@ -240,7 +240,7 @@ fn barrrr2() { //~ ERROR: The function has a cyclomatic complexity of 3
 }
 
 #[cyclomatic_complexity = "0"]
-fn cake() { //~ ERROR: The function has a cyclomatic complexity of 2
+fn cake() { //~ ERROR: the function has a cyclomatic complexity of 2
     if 4 == 5 {
         println!("yea");
     } else {
@@ -251,7 +251,7 @@ fn cake() { //~ ERROR: The function has a cyclomatic complexity of 2
 
 
 #[cyclomatic_complexity = "0"]
-pub fn read_file(input_path: &str) -> String { //~ ERROR: The function has a cyclomatic complexity of 4
+pub fn read_file(input_path: &str) -> String { //~ ERROR: the function has a cyclomatic complexity of 4
     use std::fs::File;
     use std::io::{Read, Write};
     use std::path::Path;
@@ -282,7 +282,7 @@ pub fn read_file(input_path: &str) -> String { //~ ERROR: The function has a cyc
 enum Void {}
 
 #[cyclomatic_complexity = "0"]
-fn void(void: Void) { //~ ERROR: The function has a cyclomatic complexity of 1
+fn void(void: Void) { //~ ERROR: the function has a cyclomatic complexity of 1
     if true {
         match void {
         }

tests/compile-fail/open_options.rs

@@ -5,12 +5,12 @@ use std::fs::OpenOptions;
 #[allow(unused_must_use)]
 #[deny(nonsensical_open_options)]
 fn main() {
-	OpenOptions::new().read(true).truncate(true).open("foo.txt"); //~ERROR File opened with "truncate" and "read"
-	OpenOptions::new().append(true).truncate(true).open("foo.txt"); //~ERROR File opened with "append" and "truncate"
-    
-	OpenOptions::new().read(true).read(false).open("foo.txt"); //~ERROR The method "read" is called more than once
-	OpenOptions::new().create(true).create(false).open("foo.txt"); //~ERROR The method "create" is called more than once
-	OpenOptions::new().write(true).write(false).open("foo.txt"); //~ERROR The method "write" is called more than once
-	OpenOptions::new().append(true).append(false).open("foo.txt"); //~ERROR The method "append" is called more than once
-	OpenOptions::new().truncate(true).truncate(false).open("foo.txt"); //~ERROR The method "truncate" is called more than once
+    OpenOptions::new().read(true).truncate(true).open("foo.txt"); //~ERROR file opened with "truncate" and "read"
+    OpenOptions::new().append(true).truncate(true).open("foo.txt"); //~ERROR file opened with "append" and "truncate"
+
+    OpenOptions::new().read(true).read(false).open("foo.txt"); //~ERROR the method "read" is called more than once
+    OpenOptions::new().create(true).create(false).open("foo.txt"); //~ERROR the method "create" is called more than once
+    OpenOptions::new().write(true).write(false).open("foo.txt"); //~ERROR the method "write" is called more than once
+    OpenOptions::new().append(true).append(false).open("foo.txt"); //~ERROR the method "append" is called more than once
+    OpenOptions::new().truncate(true).truncate(false).open("foo.txt"); //~ERROR the method "truncate" is called more than once
 }