From 2a28ea0beaca8b9de1d26ef4af9e264db1c143c4 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 16 Feb 2021 13:38:01 +0100 Subject: [PATCH 1/4] Add command line options option to lintcheck crates config --- clippy_dev/README.md | 58 +++++++++++++++++++++++-------------- clippy_dev/src/lintcheck.rs | 57 +++++++++++++++++++++++++++--------- 2 files changed, 79 insertions(+), 36 deletions(-) diff --git a/clippy_dev/README.md b/clippy_dev/README.md index 7c582b375357..7e794222a931 100644 --- a/clippy_dev/README.md +++ b/clippy_dev/README.md @@ -1,4 +1,4 @@ -# Clippy Dev Tool +# Clippy Dev Tool The Clippy Dev Tool is a tool to ease Clippy development, similar to `rustc`s `x.py`. @@ -28,25 +28,39 @@ The results will then be saved to `lintcheck-logs/custom_logs.toml`. ### Configuring the Crate Sources -The sources to check are saved in a `toml` file. -There are three types of sources. -A crates-io source: -````toml -bitflags = {name = "bitflags", versions = ['1.2.1']} -```` -Requires a "name" and one or multiple "versions" to be checked. +The sources to check are saved in a `toml` file. +There are three types of sources. -A git source: -````toml -puffin = {name = "puffin", git_url = "https://github.com/EmbarkStudios/puffin", git_hash = "02dd4a3"} -```` -Requires a name, the url to the repo and unique identifier of a commit, -branch or tag which is checked out before linting. -There is no way to always check `HEAD` because that would lead to changing lint-results as the repo would get updated. -If `git_url` or `git_hash` is missing, an error will be thrown. - -A local dependency: -````toml - clippy = {name = "clippy", path = "/home/user/clippy"} -```` -For when you want to add a repository that is not published yet. +1. Crates-io Source + + ````toml + bitflags = {name = "bitflags", versions = ['1.2.1']} + ```` + Requires a "name" and one or multiple "versions" to be checked. + +2. `git` Source + ````toml + puffin = {name = "puffin", git_url = "https://github.com/EmbarkStudios/puffin", git_hash = "02dd4a3"} + ```` + Requires a name, the url to the repo and unique identifier of a commit, + branch or tag which is checked out before linting. + There is no way to always check `HEAD` because that would lead to changing lint-results as the repo would get updated. + If `git_url` or `git_hash` is missing, an error will be thrown. + +3. Local Dependency + ````toml + clippy = {name = "clippy", path = "/home/user/clippy"} + ```` + For when you want to add a repository that is not published yet. + +#### Command Line Options (optional) + +```toml +bitflags = {name = "bitflags", versions = ['1.2.1'], options = ['-Wclippy::pedantic', '-Wclippy::cargo']} +``` + +It is possible to specify command line options for each crate. This makes it +possible to only check a crate for certain lint groups. If no options are +specified, the lint groups `clippy::all`, `clippy::pedantic`, and +`clippy::cargo` are checked. If an empty array is specified only `clippy::all` +is checked. diff --git a/clippy_dev/src/lintcheck.rs b/clippy_dev/src/lintcheck.rs index e96e1446c0f6..b62784da548d 100644 --- a/clippy_dev/src/lintcheck.rs +++ b/clippy_dev/src/lintcheck.rs @@ -32,15 +32,29 @@ struct TomlCrate { git_url: Option, git_hash: Option, path: Option, + options: Option>, } /// Represents an archive we download from crates.io, or a git repo, or a local repo/folder /// Once processed (downloaded/extracted/cloned/copied...), this will be translated into a `Crate` #[derive(Debug, Serialize, Deserialize, Eq, Hash, PartialEq)] enum CrateSource { - CratesIo { name: String, version: String }, - Git { name: String, url: String, commit: String }, - Path { name: String, path: PathBuf }, + CratesIo { + name: String, + version: String, + options: Option>, + }, + Git { + name: String, + url: String, + commit: String, + options: Option>, + }, + Path { + name: String, + path: PathBuf, + options: Option>, + }, } /// Represents the actual source code of a crate that we ran "cargo clippy" on @@ -50,6 +64,7 @@ struct Crate { name: String, // path to the extracted sources that clippy can check path: PathBuf, + options: Option>, } /// A single warning that clippy issued while checking a `Crate` @@ -81,7 +96,7 @@ impl CrateSource { /// copies a local folder fn download_and_extract(&self) -> Crate { match self { - CrateSource::CratesIo { name, version } => { + CrateSource::CratesIo { name, version, options } => { let extract_dir = PathBuf::from("target/lintcheck/crates"); let krate_download_dir = PathBuf::from("target/lintcheck/downloads"); @@ -113,9 +128,15 @@ impl CrateSource { version: version.clone(), name: name.clone(), path: extract_dir.join(format!("{}-{}/", name, version)), + options: options.clone(), } }, - CrateSource::Git { name, url, commit } => { + CrateSource::Git { + name, + url, + commit, + options, + } => { let repo_path = { let mut repo_path = PathBuf::from("target/lintcheck/crates"); // add a -git suffix in case we have the same crate from crates.io and a git repo @@ -152,9 +173,10 @@ impl CrateSource { version: commit.clone(), name: name.clone(), path: repo_path, + options: options.clone(), } }, - CrateSource::Path { name, path } => { + CrateSource::Path { name, path, options } => { use fs_extra::dir; // simply copy the entire directory into our target dir @@ -183,6 +205,7 @@ impl CrateSource { version: String::from("local"), name: name.clone(), path: crate_root, + options: options.clone(), } }, } @@ -198,18 +221,21 @@ impl Crate { let shared_target_dir = clippy_project_root().join("target/lintcheck/shared_target_dir/"); + let mut args = vec!["--", "--message-format=json", "--", "--cap-lints=warn"]; + + if let Some(options) = &self.options { + for opt in options { + args.push(opt); + } + } else { + args.extend(&["-Wclippy::pedantic", "-Wclippy::cargo"]) + } + let all_output = std::process::Command::new(&cargo_clippy_path) .env("CARGO_TARGET_DIR", shared_target_dir) // lint warnings will look like this: // src/cargo/ops/cargo_compile.rs:127:35: warning: usage of `FromIterator::from_iter` - .args(&[ - "--", - "--message-format=json", - "--", - "--cap-lints=warn", - "-Wclippy::pedantic", - "-Wclippy::cargo", - ]) + .args(&args) .current_dir(&self.path) .output() .unwrap_or_else(|error| { @@ -289,6 +315,7 @@ fn read_crates(toml_path: Option<&str>) -> (String, Vec) { crate_sources.push(CrateSource::Path { name: tk.name.clone(), path: PathBuf::from(path), + options: tk.options.clone(), }); } @@ -298,6 +325,7 @@ fn read_crates(toml_path: Option<&str>) -> (String, Vec) { crate_sources.push(CrateSource::CratesIo { name: tk.name.clone(), version: ver.to_string(), + options: tk.options.clone(), }); }) } @@ -307,6 +335,7 @@ fn read_crates(toml_path: Option<&str>) -> (String, Vec) { name: tk.name.clone(), url: tk.git_url.clone().unwrap(), commit: tk.git_hash.clone().unwrap(), + options: tk.options.clone(), }); } // if we have a version as well as a git data OR only one git data, something is funky From dd5c9b7ddaadfe8aaceade8b0e6ec3424e550371 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 16 Feb 2021 16:58:00 +0100 Subject: [PATCH 2/4] lintcheck: Slight improvements to the error reporting --- clippy_dev/src/lintcheck.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/clippy_dev/src/lintcheck.rs b/clippy_dev/src/lintcheck.rs index b62784da548d..cd3dd8143d55 100644 --- a/clippy_dev/src/lintcheck.rs +++ b/clippy_dev/src/lintcheck.rs @@ -283,10 +283,14 @@ fn filter_clippy_warnings(line: &str) -> bool { /// Builds clippy inside the repo to make sure we have a clippy executable we can use. fn build_clippy() { - Command::new("cargo") + let output = Command::new("cargo") .arg("build") .output() .expect("Failed to build clippy!"); + if !output.status.success() { + eprintln!("Failed to compile Clippy"); + eprintln!("stderr: {}", String::from_utf8_lossy(&output.stderr)) + } } /// Read a `toml` file and return a list of `CrateSources` that we want to check with clippy @@ -402,12 +406,14 @@ fn gather_stats(clippy_warnings: &[ClippyWarning]) -> String { /// lintchecks `main()` function pub fn run(clap_config: &ArgMatches) { - let cargo_clippy_path: PathBuf = PathBuf::from("target/debug/cargo-clippy"); - println!("Compiling clippy..."); build_clippy(); println!("Done compiling"); + let cargo_clippy_path: PathBuf = PathBuf::from("target/debug/cargo-clippy") + .canonicalize() + .expect("failed to canonicalize path to clippy binary"); + // assert that clippy is found assert!( cargo_clippy_path.is_file(), @@ -484,5 +490,6 @@ pub fn run(clap_config: &ArgMatches) { .for_each(|(cratename, msg)| text.push_str(&format!("{}: '{}'", cratename, msg))); let file = format!("lintcheck-logs/{}_logs.txt", filename); + println!("Writing logs to {}", file); write(file, text).unwrap(); } From e3f584665ac7a32c59af80801c7a68a42ead9423 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Tue, 16 Feb 2021 18:09:34 +0100 Subject: [PATCH 3/4] Reformat clippy_dev README --- clippy_dev/README.md | 50 +++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/clippy_dev/README.md b/clippy_dev/README.md index 7e794222a931..30f98489daf3 100644 --- a/clippy_dev/README.md +++ b/clippy_dev/README.md @@ -1,41 +1,48 @@ # Clippy Dev Tool -The Clippy Dev Tool is a tool to ease Clippy development, similar to `rustc`s `x.py`. +The Clippy Dev Tool is a tool to ease Clippy development, similar to `rustc`s +`x.py`. Functionalities (incomplete): ## `lintcheck` -Runs clippy on a fixed set of crates read from `clippy_dev/lintcheck_crates.toml` -and saves logs of the lint warnings into the repo. -We can then check the diff and spot new or disappearing warnings. + +Runs clippy on a fixed set of crates read from +`clippy_dev/lintcheck_crates.toml` and saves logs of the lint warnings into the +repo. We can then check the diff and spot new or disappearing warnings. From the repo root, run: -```` + +``` cargo run --target-dir clippy_dev/target --package clippy_dev \ --bin clippy_dev --manifest-path clippy_dev/Cargo.toml --features lintcheck -- lintcheck -```` +``` + or -```` + +``` cargo dev-lintcheck -```` +``` -By default the logs will be saved into `lintcheck-logs/lintcheck_crates_logs.txt`. +By default the logs will be saved into +`lintcheck-logs/lintcheck_crates_logs.txt`. -You can set a custom sources.toml by adding `--crates-toml custom.toml` or using `LINTCHECK_TOML="custom.toml"` -where `custom.toml` must be a relative path from the repo root. +You can set a custom sources.toml by adding `--crates-toml custom.toml` or using +`LINTCHECK_TOML="custom.toml"` where `custom.toml` must be a relative path from +the repo root. The results will then be saved to `lintcheck-logs/custom_logs.toml`. ### Configuring the Crate Sources -The sources to check are saved in a `toml` file. -There are three types of sources. +The sources to check are saved in a `toml` file. There are three types of +sources. 1. Crates-io Source - ````toml + ```toml bitflags = {name = "bitflags", versions = ['1.2.1']} - ```` + ``` Requires a "name" and one or multiple "versions" to be checked. 2. `git` Source @@ -43,14 +50,15 @@ There are three types of sources. puffin = {name = "puffin", git_url = "https://github.com/EmbarkStudios/puffin", git_hash = "02dd4a3"} ```` Requires a name, the url to the repo and unique identifier of a commit, - branch or tag which is checked out before linting. - There is no way to always check `HEAD` because that would lead to changing lint-results as the repo would get updated. - If `git_url` or `git_hash` is missing, an error will be thrown. + branch or tag which is checked out before linting. There is no way to always + check `HEAD` because that would lead to changing lint-results as the repo + would get updated. If `git_url` or `git_hash` is missing, an error will be + thrown. 3. Local Dependency - ````toml - clippy = {name = "clippy", path = "/home/user/clippy"} - ```` + ```toml + clippy = {name = "clippy", path = "/home/user/clippy"} + ``` For when you want to add a repository that is not published yet. #### Command Line Options (optional) From 79d7f4ccb355ac5d3f6d1c18d84c8e76092d0e97 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Wed, 17 Feb 2021 08:34:23 +0100 Subject: [PATCH 4/4] lintcheck: Add a note that -Wclippy::all is enabled by default --- clippy_dev/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clippy_dev/README.md b/clippy_dev/README.md index 30f98489daf3..a5ed9e27bd2b 100644 --- a/clippy_dev/README.md +++ b/clippy_dev/README.md @@ -72,3 +72,6 @@ possible to only check a crate for certain lint groups. If no options are specified, the lint groups `clippy::all`, `clippy::pedantic`, and `clippy::cargo` are checked. If an empty array is specified only `clippy::all` is checked. + +**Note:** `-Wclippy::all` is always enabled by default, unless `-Aclippy::all` +is explicitly specified in the options.