From 90f31e21ab1e3b0b3cd3887094a705217f67bdfa Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Sun, 4 Nov 2018 09:41:28 +0100 Subject: [PATCH] RIIR update lints: Add check mode (update_lints.py rewrite complete) 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: --- CONTRIBUTING.md | 2 +- ci/base-tests.sh | 5 +- clippy_dev/src/lib.rs | 39 ++++--- clippy_dev/src/main.rs | 59 +++++++--- clippy_lints/src/lib.rs | 12 +- util/update_lints.py | 242 +--------------------------------------- 6 files changed, 81 insertions(+), 278 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c4080a5fa9c..50f61eb1e67 100644 --- a/CONTRIBUTING.md +++ b/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 diff --git a/ci/base-tests.sh b/ci/base-tests.sh index 0ed1494be82..88cc20842e8 100755 --- a/ci/base-tests.sh +++ b/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... diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 2ae8423381d..d1ba13bbe9d 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -168,23 +168,33 @@ fn lint_files() -> impl Iterator { .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(path: &str, start: &str, end: &str, replace_start: bool, replacements: F) where F: Fn() -> Vec { +pub fn replace_region_in_file(path: &str, start: &str, end: &str, replace_start: bool, write_back: bool, replacements: F) -> FileChange where F: Fn() -> Vec { 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 file_change = 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"); + 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,10 +223,10 @@ pub fn replace_region_in_file(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(text: &str, start: &str, end: &str, replace_start: bool, replacements: F) -> String where F: Fn() -> Vec { +pub fn replace_region_in_text(text: &str, start: &str, end: &str, replace_start: bool, replacements: F) -> FileChange where F: Fn() -> Vec { let lines = text.lines(); let mut in_old_region = false; let mut found = false; @@ -224,7 +234,7 @@ pub fn replace_region_in_text(text: &str, start: &str, end: &str, replace_sta 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 +258,10 @@ pub fn replace_region_in_text(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] @@ -305,7 +318,7 @@ def ghi"#; let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, false, || { vec!["hello world".to_string()] - }); + }).new_lines; assert_eq!(expected, result); } @@ -323,7 +336,7 @@ def ghi"#; let result = replace_region_in_text(text, r#"^\s*abc$"#, r#"^\s*def"#, true, || { vec!["hello world".to_string()] - }); + }).new_lines; assert_eq!(expected, result); } diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 288fb7c58b4..c9b498f5d5a 100644 --- a/clippy_dev/src/main.rs +++ b/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)") ) - ) - .get_matches(); + .arg( + Arg::with_name("check") + .long("check") + .help("Checks that util/dev update_lints has been run. Used on CI."), + ) + ) + .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 = gather_all().collect(); let usable_lints: Vec = 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", "", "", 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); } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 06ae78a6509..c93e9d57d67 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -548,8 +548,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { bytecount::NAIVE_BYTECOUNT, collapsible_if::COLLAPSIBLE_IF, const_static_lifetime::CONST_STATIC_LIFETIME, - copies::IF_SAME_THEN_ELSE, copies::IFS_SAME_COND, + copies::IF_SAME_THEN_ELSE, cyclomatic_complexity::CYCLOMATIC_COMPLEXITY, derive::DERIVE_HASH_XOR_EQ, double_comparison::DOUBLE_COMPARISONS, @@ -743,12 +743,12 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { unused_io_amount::UNUSED_IO_AMOUNT, unused_label::UNUSED_LABEL, vec::USELESS_VEC, + write::PRINTLN_EMPTY_STRING, write::PRINT_LITERAL, write::PRINT_WITH_NEWLINE, - write::PRINTLN_EMPTY_STRING, + write::WRITELN_EMPTY_STRING, write::WRITE_LITERAL, write::WRITE_WITH_NEWLINE, - write::WRITELN_EMPTY_STRING, zero_div_zero::ZERO_DIVIDED_BY_ZERO, ]); @@ -831,12 +831,12 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { types::IMPLICIT_HASHER, types::LET_UNIT_VALUE, unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME, + write::PRINTLN_EMPTY_STRING, write::PRINT_LITERAL, write::PRINT_WITH_NEWLINE, - write::PRINTLN_EMPTY_STRING, + write::WRITELN_EMPTY_STRING, write::WRITE_LITERAL, write::WRITE_WITH_NEWLINE, - write::WRITELN_EMPTY_STRING, ]); reg.register_lint_group("clippy::complexity", Some("clippy_complexity"), vec![ @@ -916,8 +916,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { bit_mask::BAD_BIT_MASK, bit_mask::INEFFECTIVE_BIT_MASK, booleans::LOGIC_BUG, - copies::IF_SAME_THEN_ELSE, copies::IFS_SAME_COND, + copies::IF_SAME_THEN_ELSE, derive::DERIVE_HASH_XOR_EQ, drop_forget_ref::DROP_COPY, drop_forget_ref::DROP_REF, diff --git a/util/update_lints.py b/util/update_lints.py index 221069d353c..4467b5c0cf7 100755 --- a/util/update_lints.py +++ b/util/update_lints.py @@ -10,245 +10,11 @@ # option. This file may not be copied, modified, or distributed # except according to those terms. - -# Generate a Markdown table of all lints, and put it in README.md. -# With -n option, only print the new table to stdout. -# With -c option, print a warning and set exit status to 1 if a file would be -# changed. - -import os -import re import sys -from subprocess import call - -declare_deprecated_lint_re = re.compile(r''' - declare_deprecated_lint! \s* [{(] \s* - pub \s+ (?P[A-Z_][A-Z_0-9]*) \s*,\s* - " (?P(?:[^"\\]+|\\.)*) " \s* [})] -''', re.VERBOSE | re.DOTALL) - -declare_clippy_lint_re = re.compile(r''' - declare_clippy_lint! \s* [{(] \s* - pub \s+ (?P[A-Z_][A-Z_0-9]*) \s*,\s* - (?P[a-z_]+) \s*,\s* - " (?P(?:[^"\\]+|\\.)*) " \s* [})] -''', re.VERBOSE | re.DOTALL) - -nl_escape_re = re.compile(r'\\\n\s*') - -docs_link = 'https://rust-lang-nursery.github.io/rust-clippy/master/index.html' - - -def collect(deprecated_lints, clippy_lints, fn): - """Collect all lints from a file. - - Adds entries to the lints list as `(module, name, level, desc)`. - """ - with open(fn) as fp: - code = fp.read() - - for match in declare_deprecated_lint_re.finditer(code): - # remove \-newline escapes from description string - desc = nl_escape_re.sub('', match.group('desc')) - deprecated_lints.append((os.path.splitext(os.path.basename(fn))[0], - match.group('name').lower(), - desc.replace('\\"', '"'))) - - for match in declare_clippy_lint_re.finditer(code): - # remove \-newline escapes from description string - desc = nl_escape_re.sub('', match.group('desc')) - cat = match.group('cat') - if cat in ('internal', 'internal_warn'): - continue - module_name = os.path.splitext(os.path.basename(fn))[0] - if module_name == 'mod': - module_name = os.path.basename(os.path.dirname(fn)) - clippy_lints[cat].append((module_name, - match.group('name').lower(), - "allow", - desc.replace('\\"', '"'))) - - -def gen_group(lints): - """Write lint group (list of all lints in the form module::NAME).""" - for (module, name, _, _) in sorted(lints): - yield ' %s::%s,\n' % (module, name.upper()) - - -def gen_mods(lints): - """Declare modules""" - - for module in sorted(set(lint[0] for lint in lints)): - yield 'pub mod %s;\n' % module - - -def gen_deprecated(lints): - """Declare deprecated lints""" - - for lint in lints: - yield ' store.register_removed(\n' - yield ' "%s",\n' % lint[1] - yield ' "%s",\n' % lint[2] - yield ' );\n' - - -def replace_region(fn, region_start, region_end, callback, - replace_start=True, write_back=True): - """Replace a region in a file delimited by two lines matching regexes. - - A callback is called to write the new region. If `replace_start` is true, - the start delimiter line is replaced as well. The end delimiter line is - never replaced. - """ - # read current content - with open(fn) as fp: - lines = list(fp) - - found = False - - # replace old region with new region - new_lines = [] - in_old_region = False - for line in lines: - if in_old_region: - if re.search(region_end, line): - in_old_region = False - new_lines.extend(callback()) - new_lines.append(line) - elif re.search(region_start, line): - if not replace_start: - new_lines.append(line) - # old region starts here - in_old_region = True - found = True - else: - new_lines.append(line) - - if not found: - print("regex " + region_start + " not found") - - # write back to file - if write_back: - with open(fn, 'w') as fp: - fp.writelines(new_lines) - - # if something changed, return true - return lines != new_lines - - -def main(print_only=False, check=False): - deprecated_lints = [] - clippy_lints = { - "correctness": [], - "style": [], - "complexity": [], - "perf": [], - "restriction": [], - "pedantic": [], - "cargo": [], - "nursery": [], - } - - # check directory - if not os.path.isfile('clippy_lints/src/lib.rs'): - print('Error: call this script from clippy checkout directory!') - return - - # collect all lints from source files - for root, dirs, files in os.walk('clippy_lints/src'): - for fn in files: - if fn.endswith('.rs'): - collect(deprecated_lints, clippy_lints, - os.path.join(root, fn)) - - # determine version - with open('Cargo.toml') as fp: - for line in fp: - if line.startswith('version ='): - clippy_version = line.split()[2].strip('"') - break - else: - print('Error: version not found in Cargo.toml!') - return - - all_lints = [] - clippy_lint_groups = [ - "correctness", - "style", - "complexity", - "perf", - ] - clippy_lint_list = [] - for x in clippy_lint_groups: - clippy_lint_list += clippy_lints[x] - for _, value in clippy_lints.iteritems(): - all_lints += value - - if print_only: - call(["./util/dev", "update_lints", "--print-only"]) - return - - # update the lint counter in README.md - changed = replace_region( - 'README.md', - r'^\[There are \d+ lints included in this crate!\]\(https://rust-lang-nursery.github.io/rust-clippy/master/index.html\)$', "", - lambda: ['[There are %d lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)\n' % - (len(all_lints))], - write_back=not check) - - # update the links in the CHANGELOG - changed |= replace_region( - 'CHANGELOG.md', - "", - "", - lambda: ["[`{0}`]: {1}#{0}\n".format(l[1], docs_link) for l in - sorted(all_lints + deprecated_lints, - key=lambda l: l[1])], - replace_start=False, write_back=not check) - - # update version of clippy_lints in Cargo.toml - changed |= replace_region( - 'Cargo.toml', r'# begin automatic update', '# end automatic update', - lambda: ['clippy_lints = { version = "%s", path = "clippy_lints" }\n' % - clippy_version], - replace_start=False, write_back=not check) - - # update version of clippy_lints in Cargo.toml - changed |= replace_region( - 'clippy_lints/Cargo.toml', r'# begin automatic update', '# end automatic update', - lambda: ['version = "%s"\n' % clippy_version], - replace_start=False, write_back=not check) - - # update the `pub mod` list - changed |= replace_region( - 'clippy_lints/src/lib.rs', r'begin lints modules', r'end lints modules', - lambda: gen_mods(all_lints), - replace_start=False, write_back=not check) - - # same for "clippy::*" lint collections - changed |= replace_region( - 'clippy_lints/src/lib.rs', r'reg.register_lint_group\("clippy::all"', r'\]\);', - lambda: gen_group(clippy_lint_list), - replace_start=False, write_back=not check) - - for key, value in clippy_lints.iteritems(): - # same for "clippy::*" lint collections - changed |= replace_region( - 'clippy_lints/src/lib.rs', r'reg.register_lint_group\("clippy::' + key + r'"', r'\]\);', - lambda: gen_group(value), - replace_start=False, write_back=not check) - - # same for "deprecated" lint collection - changed |= replace_region( - 'clippy_lints/src/lib.rs', r'begin deprecated lints', r'end deprecated lints', - lambda: gen_deprecated(deprecated_lints), - replace_start=False, - write_back=not check) - - if check and changed: - print('Please run util/update_lints.py to regenerate lints lists.') - return 1 +def main(): + print('Error: Please use `util/dev` to update lints') + return 1 if __name__ == '__main__': - sys.exit(main(print_only='-n' in sys.argv, check='-c' in sys.argv)) + sys.exit(main())