Merge #3408

3408: RIIR update lints: Add check mode (update_lints.py rewrite complete) r=oli-obk a=phansch

This finishes up the rewrite of `update_lints.py` in Rust. More
specifically, this

* adds the `--check` flag and handling to clippy_dev
* tracks file changes over the different calls to `replace_region_in_file`
* only writes changes to files if the `--check` flag is *not* used
* runs `./util/dev update_lints --check` on CI instead of the old script
* replaces usage of the `update_lints.py` script with an error

`./util/dev update_lints` behaves 99% the same as the python script.
The only difference that I'm aware of is an ordering change to
`clippy_lints/src/lib.rs` because underscores seem to be sorted
differently in Rust and in Python.

:checkered_flag:

cc #2882 

Co-authored-by: Philipp Hansch <[email protected]>
Co-authored-by: Philipp Krones <[email protected]>

Author: 3 people

CONTRIBUTING.md

@@ -180,7 +180,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
 
 The [`rustc_plugin::PluginRegistry`][plugin_registry] provides two methods to register lints: [register_early_lint_pass][reg_early_lint_pass] and [register_late_lint_pass][reg_late_lint_pass].
 Both take an object that implements an [`EarlyLintPass`][early_lint_pass] or [`LateLintPass`][late_lint_pass] respectively. This is done in every single lint.
-It's worth noting that the majority of `clippy_lints/src/lib.rs` is autogenerated by `util/update_lints.py` and you don't have to add anything by hand. When you are writing your own lint, you can use that script to save you some time.
+It's worth noting that the majority of `clippy_lints/src/lib.rs` is autogenerated by `util/dev update_lints` and you don't have to add anything by hand. When you are writing your own lint, you can use that script to save you some time.
 
 ```rust
 // ./clippy_lints/src/else_if_without_else.rs

ci/base-tests.sh

@@ -23,8 +23,9 @@ cargo test --features debugging
 cd clippy_lints && cargo test && cd ..
 cd rustc_tools_util && cargo test && cd ..
 cd clippy_dev && cargo test && cd ..
-# check that the lint lists are up-to-date
-./util/update_lints.py -c
+
+# Perform various checks for lint registration
+./util/dev update_lints --check
 
 CLIPPY="`pwd`/target/debug/cargo-clippy clippy"
 # run clippy on its own codebase...

clippy_dev/src/lib.rs

@@ -114,19 +114,22 @@ pub fn gen_changelog_lint_list(lints: Vec<Lint>) -> Vec<String> {
 
 /// Generates the `register_removed` code in `./clippy_lints/src/lib.rs`.
 pub fn gen_deprecated(lints: &[Lint]) -> Vec<String> {
-    lints.iter()
-        .filter_map(|l| {
-            l.clone().deprecation.and_then(|depr_text| {
-                Some(
-                    format!(
-                        "    store.register_removed(\n        \"{}\",\n        \"{}\",\n    );",
-                        l.name,
-                        depr_text
+    itertools::flatten(
+        lints
+            .iter()
+            .filter_map(|l| {
+                l.clone().deprecation.and_then(|depr_text| {
+                    Some(
+                        vec![
+                            "    store.register_removed(".to_string(),
+                            format!("        \"{}\",", l.name),
+                            format!("        \"{}\",", depr_text),
+                            "    );".to_string()
+                        ]
                     )
-                )
+                })
             })
-        })
-        .collect()
+    ).collect()
 }
 
 /// Gathers all files in `src/clippy_lints` and gathers all lints inside
@@ -168,23 +171,33 @@ fn lint_files() -> impl Iterator<Item=walkdir::DirEntry> {
         .filter(|f| f.path().extension() == Some(OsStr::new("rs")))
 }
 
+/// Whether a file has had its text changed or not
+#[derive(PartialEq, Debug)]
+pub struct FileChange {
+    pub changed: bool,
+    pub new_lines: String,
+}
+
 /// Replace a region in a file delimited by two lines matching regexes.
 ///
 /// `path` is the relative path to the file on which you want to perform the replacement.
 ///
 /// See `replace_region_in_text` for documentation of the other options.
 #[allow(clippy::expect_fun_call)]
-pub fn replace_region_in_file<F>(path: &str, start: &str, end: &str, replace_start: bool, replacements: F) where F: Fn() -> Vec<String> {
+pub fn replace_region_in_file<F>(path: &str, start: &str, end: &str, replace_start: bool, write_back: bool, replacements: F) -> FileChange where F: Fn() -> Vec<String> {
     let mut f = fs::File::open(path).expect(&format!("File not found: {}", path));
     let mut contents = String::new();
     f.read_to_string(&mut contents).expect("Something went wrong reading the file");
-    let replaced = replace_region_in_text(&contents, start, end, replace_start, replacements);
-
-    let mut f = fs::File::create(path).expect(&format!("File not found: {}", path));
-    f.write_all(replaced.as_bytes()).expect("Unable to write file");
-    // Ensure we write the changes with a trailing newline so that
-    // the file has the proper line endings.
-    f.write_all(b"\n").expect("Unable to write file");
+    let file_change = replace_region_in_text(&contents, start, end, replace_start, replacements);
+
+    if write_back {
+        let mut f = fs::File::create(path).expect(&format!("File not found: {}", path));
+        f.write_all(file_change.new_lines.as_bytes()).expect("Unable to write file");
+        // Ensure we write the changes with a trailing newline so that
+        // the file has the proper line endings.
+        f.write_all(b"\n").expect("Unable to write file");
+    }
+    file_change
 }
 
 /// Replace a region in a text delimited by two lines matching regexes.
@@ -213,18 +226,18 @@ pub fn replace_region_in_file<F>(path: &str, start: &str, end: &str, replace_sta
 ///     || {
 ///         vec!["a different".to_string(), "text".to_string()]
 ///     }
-/// );
+/// ).new_lines;
 /// assert_eq!("replace_start\na different\ntext\nreplace_end", result);
 /// ```
-pub fn replace_region_in_text<F>(text: &str, start: &str, end: &str, replace_start: bool, replacements: F) -> String where F: Fn() -> Vec<String> {
+pub fn replace_region_in_text<F>(text: &str, start: &str, end: &str, replace_start: bool, replacements: F) -> FileChange where F: Fn() -> Vec<String> {
     let lines = text.lines();
     let mut in_old_region = false;
     let mut found = false;
     let mut new_lines = vec![];
     let start = Regex::new(start).unwrap();
     let end = Regex::new(end).unwrap();
 
-    for line in lines {
+    for line in lines.clone() {
         if in_old_region {
             if end.is_match(&line) {
                 in_old_region = false;
@@ -248,7 +261,11 @@ pub fn replace_region_in_text<F>(text: &str, start: &str, end: &str, replace_sta
         // is incorrect.
         eprintln!("error: regex `{:?}` not found. You may have to update it.", start);
     }
-    new_lines.join("\n")
+
+    FileChange {
+        changed: lines.ne(new_lines.clone()),
+        new_lines: new_lines.join("\n")
+    }
 }
 
 #[test]
@@ -292,17 +309,11 @@ declare_deprecated_lint! {
 
 #[test]
 fn test_replace_region() {
-    let text = r#"
-abc
-123
-789
-def
-ghi"#;
-    let expected = r#"
-abc
-hello world
-def
-ghi"#;
+    let text = "\nabc\n123\n789\ndef\nghi";
+    let expected = FileChange {
+        changed: true,
+        new_lines: "\nabc\nhello world\ndef\nghi".to_string()
+    };
     let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, false, || {
         vec!["hello world".to_string()]
     });
@@ -311,22 +322,30 @@ ghi"#;
 
 #[test]
 fn test_replace_region_with_start() {
-    let text = r#"
-abc
-123
-789
-def
-ghi"#;
-    let expected = r#"
-hello world
-def
-ghi"#;
+    let text = "\nabc\n123\n789\ndef\nghi";
+    let expected = FileChange {
+        changed: true,
+        new_lines: "\nhello world\ndef\nghi".to_string()
+    };
     let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, true, || {
         vec!["hello world".to_string()]
     });
     assert_eq!(expected, result);
 }
 
+#[test]
+fn test_replace_region_no_changes() {
+    let text = "123\n456\n789";
+    let expected = FileChange {
+        changed: false,
+        new_lines: "123\n456\n789".to_string()
+    };
+    let result = replace_region_in_text(text, r#"^\s*123$"#, r#"^\s*456"#, false, || {
+        vec![]
+    });
+    assert_eq!(expected, result);
+}
+
 #[test]
 fn test_usable_lints() {
     let lints = vec![
@@ -377,14 +396,19 @@ fn test_gen_changelog_lint_list() {
 fn test_gen_deprecated() {
     let lints = vec![
         Lint::new("should_assert_eq", "group1", "abc", Some("has been superseeded by should_assert_eq2"), "module_name"),
+        Lint::new("another_deprecated", "group2", "abc", Some("will be removed"), "module_name"),
         Lint::new("should_assert_eq2", "group2", "abc", None, "module_name")
     ];
     let expected: Vec<String> = vec![
-        r#"    store.register_removed(
-        "should_assert_eq",
-        "has been superseeded by should_assert_eq2",
-    );"#.to_string()
-    ];
+        "    store.register_removed(",
+        "        \"should_assert_eq\",",
+        "        \"has been superseeded by should_assert_eq2\",",
+        "    );",
+        "    store.register_removed(",
+        "        \"another_deprecated\",",
+        "        \"will be removed\",",
+        "    );"
+    ].into_iter().map(String::from).collect();
     assert_eq!(expected, gen_deprecated(&lints));
 }
 

clippy_dev/src/main.rs

@@ -15,6 +15,12 @@ extern crate regex;
 use clap::{App, Arg, SubCommand};
 use clippy_dev::*;
 
+#[derive(PartialEq)]
+enum UpdateMode {
+    Check,
+    Change
+}
+
 fn main() {
     let matches = App::new("Clippy developer tooling")
         .subcommand(
@@ -28,17 +34,23 @@ fn main() {
                 .arg(
                     Arg::with_name("print-only")
                         .long("print-only")
-                        .short("p")
-                        .help("Print a table of lints to STDOUT. This does not include deprecated and internal lints. (Does not modify any files)"),
+                        .help("Print a table of lints to STDOUT. This does not include deprecated and internal lints. (Does not modify any files)")
+                )
+                .arg(
+                    Arg::with_name("check")
+                        .long("check")
+                        .help("Checks that util/dev update_lints has been run. Used on CI."),
                 )
-        )
-        .get_matches();
+            )
+            .get_matches();
 
     if let Some(matches) = matches.subcommand_matches("update_lints") {
         if matches.is_present("print-only") {
             print_lints();
+        } else if matches.is_present("check") {
+            update_lints(&UpdateMode::Check);
         } else {
-            update_lints();
+            update_lints(&UpdateMode::Change);
         }
     }
 }
@@ -63,53 +75,58 @@ fn print_lints() {
     println!("there are {} lints", lint_count);
 }
 
-fn update_lints() {
+fn update_lints(update_mode: &UpdateMode) {
     let lint_list: Vec<Lint> = gather_all().collect();
     let usable_lints: Vec<Lint> = Lint::usable_lints(lint_list.clone().into_iter()).collect();
     let lint_count = usable_lints.len();
 
-    replace_region_in_file(
+    let mut file_change = replace_region_in_file(
         "../README.md",
         r#"\[There are \d+ lints included in this crate!\]\(https://rust-lang-nursery.github.io/rust-clippy/master/index.html\)"#,
         "",
         true,
+        update_mode == &UpdateMode::Change,
         || {
             vec![
                 format!("[There are {} lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)", lint_count)
             ]
         }
-    );
+    ).changed;
 
-    replace_region_in_file(
+    file_change |= replace_region_in_file(
         "../CHANGELOG.md",
         "<!-- begin autogenerated links to lint list -->",
         "<!-- end autogenerated links to lint list -->",
         false,
+        update_mode == &UpdateMode::Change,
         || { gen_changelog_lint_list(lint_list.clone()) }
-    );
+    ).changed;
 
-    replace_region_in_file(
+    file_change |= replace_region_in_file(
         "../clippy_lints/src/lib.rs",
         "begin deprecated lints",
         "end deprecated lints",
         false,
+        update_mode == &UpdateMode::Change,
         || { gen_deprecated(&lint_list) }
-    );
+    ).changed;
 
-    replace_region_in_file(
+    file_change |= replace_region_in_file(
         "../clippy_lints/src/lib.rs",
         "begin lints modules",
         "end lints modules",
         false,
+        update_mode == &UpdateMode::Change,
         || { gen_modules_list(lint_list.clone()) }
-    );
+    ).changed;
 
     // Generate lists of lints in the clippy::all lint group
-    replace_region_in_file(
+    file_change |= replace_region_in_file(
         "../clippy_lints/src/lib.rs",
         r#"reg.register_lint_group\("clippy::all""#,
         r#"\]\);"#,
         false,
+        update_mode == &UpdateMode::Change,
         || {
             // clippy::all should only include the following lint groups:
             let all_group_lints = usable_lints.clone().into_iter().filter(|l| {
@@ -121,16 +138,22 @@ fn update_lints() {
 
             gen_lint_group_list(all_group_lints)
         }
-    );
+    ).changed;
 
     // Generate the list of lints for all other lint groups
     for (lint_group, lints) in Lint::by_lint_group(&usable_lints) {
-        replace_region_in_file(
+        file_change |= replace_region_in_file(
             "../clippy_lints/src/lib.rs",
             &format!("reg.register_lint_group\\(\"clippy::{}\"", lint_group),
             r#"\]\);"#,
             false,
+            update_mode == &UpdateMode::Change,
             || { gen_lint_group_list(lints.clone()) }
-        );
+        ).changed;
+    }
+
+    if update_mode == &UpdateMode::Check && file_change {
+      println!("Not all lints defined properly. Please run `util/dev update_lints` to make sure all lints are defined properly.");
+      std::process::exit(1);
     }
 }