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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RIIR update lints: Add check mode (update_lints.py rewrite complete) #3408

Merged
merged 4 commits into from Nov 5, 2018
Jump to file or symbol
Failed to load files and symbols.
+128 鈭314
Diff settings

Always

Just for now

Copy path View file
@@ -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
Copy path View file
@@ -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...
Copy path View file
@@ -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));
}
Copy path View file
@@ -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);
}
}
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.