diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7265d1b8323..b202fc4f281 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -115,7 +115,7 @@ To work around this, you need to have a copy of the [rustc-repo][rustc_repo] ava `git clone https://github.com/rust-lang/rust/`. Then you can run a `cargo dev` command to automatically make Clippy use the rustc-repo via path-dependencies which `IntelliJ Rust` will be able to understand. -Run `cargo dev ide_setup --repo-path ` where `` is a path to the rustc repo +Run `cargo dev setup intellij --repo-path ` where `` is a path to the rustc repo you just cloned. The command will add path-dependencies pointing towards rustc-crates inside the rustc repo to Clippys `Cargo.toml`s and should allow `IntelliJ Rust` to understand most of the types that Clippy uses. diff --git a/clippy_dev/src/fmt.rs b/clippy_dev/src/fmt.rs index edc8e6a629c..c81eb40d52f 100644 --- a/clippy_dev/src/fmt.rs +++ b/clippy_dev/src/fmt.rs @@ -86,7 +86,7 @@ pub fn run(check: bool, verbose: bool) { }, CliError::RaSetupActive => { eprintln!( - "error: a local rustc repo is enabled as path dependency via `cargo dev ide_setup`. + "error: a local rustc repo is enabled as path dependency via `cargo dev setup intellij`. Not formatting because that would format the local repo as well! Please revert the changes to Cargo.tomls first." ); diff --git a/clippy_dev/src/ide_setup.rs b/clippy_dev/src/ide_setup.rs deleted file mode 100644 index defb1133e44..00000000000 --- a/clippy_dev/src/ide_setup.rs +++ /dev/null @@ -1,103 +0,0 @@ -use std::fs; -use std::fs::File; -use std::io::prelude::*; -use std::path::{Path, PathBuf}; - -// This module takes an absolute path to a rustc repo and alters the dependencies to point towards -// the respective rustc subcrates instead of using extern crate xyz. -// This allows rust analyzer to analyze rustc internals and show proper information inside clippy -// code. See https://github.com/rust-analyzer/rust-analyzer/issues/3517 and https://github.com/rust-lang/rust-clippy/issues/5514 for details - -/// # Panics -/// -/// Panics if `rustc_path` does not lead to a rustc repo or the files could not be read -pub fn run(rustc_path: Option<&str>) { - // we can unwrap here because the arg is required by clap - let rustc_path = PathBuf::from(rustc_path.unwrap()) - .canonicalize() - .expect("failed to get the absolute repo path"); - assert!(rustc_path.is_dir(), "path is not a directory"); - let rustc_source_basedir = rustc_path.join("compiler"); - assert!( - rustc_source_basedir.is_dir(), - "are you sure the path leads to a rustc repo?" - ); - - let clippy_root_manifest = fs::read_to_string("Cargo.toml").expect("failed to read ./Cargo.toml"); - let clippy_root_lib_rs = fs::read_to_string("src/driver.rs").expect("failed to read ./src/driver.rs"); - inject_deps_into_manifest( - &rustc_source_basedir, - "Cargo.toml", - &clippy_root_manifest, - &clippy_root_lib_rs, - ) - .expect("Failed to inject deps into ./Cargo.toml"); - - let clippy_lints_manifest = - fs::read_to_string("clippy_lints/Cargo.toml").expect("failed to read ./clippy_lints/Cargo.toml"); - let clippy_lints_lib_rs = - fs::read_to_string("clippy_lints/src/lib.rs").expect("failed to read ./clippy_lints/src/lib.rs"); - inject_deps_into_manifest( - &rustc_source_basedir, - "clippy_lints/Cargo.toml", - &clippy_lints_manifest, - &clippy_lints_lib_rs, - ) - .expect("Failed to inject deps into ./clippy_lints/Cargo.toml"); -} - -fn inject_deps_into_manifest( - rustc_source_dir: &Path, - manifest_path: &str, - cargo_toml: &str, - lib_rs: &str, -) -> std::io::Result<()> { - // do not inject deps if we have aleady done so - if cargo_toml.contains("[target.'cfg(NOT_A_PLATFORM)'.dependencies]") { - eprintln!( - "cargo dev ide_setup: warning: deps already found inside {}, doing nothing.", - manifest_path - ); - return Ok(()); - } - - let extern_crates = lib_rs - .lines() - // get the deps - .filter(|line| line.starts_with("extern crate")) - // we have something like "extern crate foo;", we only care about the "foo" - // ↓ ↓ - // extern crate rustc_middle; - .map(|s| &s[13..(s.len() - 1)]); - - let new_deps = extern_crates.map(|dep| { - // format the dependencies that are going to be put inside the Cargo.toml - format!( - "{dep} = {{ path = \"{source_path}/{dep}\" }}\n", - dep = dep, - source_path = rustc_source_dir.display() - ) - }); - - // format a new [dependencies]-block with the new deps we need to inject - let mut all_deps = String::from("[target.'cfg(NOT_A_PLATFORM)'.dependencies]\n"); - new_deps.for_each(|dep_line| { - all_deps.push_str(&dep_line); - }); - all_deps.push_str("\n[dependencies]\n"); - - // replace "[dependencies]" with - // [dependencies] - // dep1 = { path = ... } - // dep2 = { path = ... } - // etc - let new_manifest = cargo_toml.replacen("[dependencies]\n", &all_deps, 1); - - // println!("{}", new_manifest); - let mut file = File::create(manifest_path)?; - file.write_all(new_manifest.as_bytes())?; - - println!("Dependency paths injected: {}", manifest_path); - - Ok(()) -} diff --git a/clippy_dev/src/lib.rs b/clippy_dev/src/lib.rs index 69f42aca8b6..72bdaf8d592 100644 --- a/clippy_dev/src/lib.rs +++ b/clippy_dev/src/lib.rs @@ -14,9 +14,9 @@ use walkdir::WalkDir; pub mod bless; pub mod fmt; -pub mod ide_setup; pub mod new_lint; pub mod serve; +pub mod setup; pub mod stderr_length_check; pub mod update_lints; diff --git a/clippy_dev/src/main.rs b/clippy_dev/src/main.rs index 7040c257c83..f5bd08657ea 100644 --- a/clippy_dev/src/main.rs +++ b/clippy_dev/src/main.rs @@ -2,8 +2,8 @@ // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] -use clap::{App, Arg, ArgMatches, SubCommand}; -use clippy_dev::{bless, fmt, ide_setup, new_lint, serve, stderr_length_check, update_lints}; +use clap::{App, AppSettings, Arg, ArgMatches, SubCommand}; +use clippy_dev::{bless, fmt, new_lint, serve, setup, stderr_length_check, update_lints}; fn main() { let matches = get_clap_config(); @@ -36,7 +36,20 @@ fn main() { ("limit_stderr_length", _) => { stderr_length_check::check(); }, - ("ide_setup", Some(matches)) => ide_setup::run(matches.value_of("rustc-repo-path")), + ("setup", Some(sub_command)) => match sub_command.subcommand() { + ("intellij", Some(matches)) => setup::intellij::setup_rustc_src( + matches + .value_of("rustc-repo-path") + .expect("this field is mandatory and therefore always valid"), + ), + ("git-hook", Some(matches)) => setup::git_hook::install_hook(matches.is_present("force-override")), + _ => {}, + }, + ("remove", Some(sub_command)) => match sub_command.subcommand() { + ("git-hook", Some(_)) => setup::git_hook::remove_hook(), + ("intellij", Some(_)) => setup::intellij::remove_rustc_src(), + _ => {}, + }, ("serve", Some(matches)) => { let port = matches.value_of("port").unwrap().parse().unwrap(); let lint = matches.value_of("lint"); @@ -48,6 +61,7 @@ fn main() { fn get_clap_config<'a>() -> ArgMatches<'a> { App::new("Clippy developer tooling") + .setting(AppSettings::ArgRequiredElseHelp) .subcommand( SubCommand::with_name("bless") .about("bless the test output changes") @@ -140,16 +154,42 @@ fn get_clap_config<'a>() -> ArgMatches<'a> { .about("Ensures that stderr files do not grow longer than a certain amount of lines."), ) .subcommand( - SubCommand::with_name("ide_setup") - .about("Alter dependencies so Intellij Rust can find rustc internals") - .arg( - Arg::with_name("rustc-repo-path") - .long("repo-path") - .short("r") - .help("The path to a rustc repo that will be used for setting the dependencies") - .takes_value(true) - .value_name("path") - .required(true), + SubCommand::with_name("setup") + .about("Support for setting up your personal development environment") + .setting(AppSettings::ArgRequiredElseHelp) + .subcommand( + SubCommand::with_name("intellij") + .about("Alter dependencies so Intellij Rust can find rustc internals") + .arg( + Arg::with_name("rustc-repo-path") + .long("repo-path") + .short("r") + .help("The path to a rustc repo that will be used for setting the dependencies") + .takes_value(true) + .value_name("path") + .required(true), + ), + ) + .subcommand( + SubCommand::with_name("git-hook") + .about("Add a pre-commit git hook that formats your code to make it look pretty") + .arg( + Arg::with_name("force-override") + .long("force-override") + .short("f") + .help("Forces the override of an existing git pre-commit hook") + .required(false), + ), + ), + ) + .subcommand( + SubCommand::with_name("remove") + .about("Support for undoing changes done by the setup command") + .setting(AppSettings::ArgRequiredElseHelp) + .subcommand(SubCommand::with_name("git-hook").about("Remove any existing pre-commit git hook")) + .subcommand( + SubCommand::with_name("intellij") + .about("Removes rustc source paths added via `cargo dev setup intellij`"), ), ) .subcommand( diff --git a/clippy_dev/src/setup/git_hook.rs b/clippy_dev/src/setup/git_hook.rs new file mode 100644 index 00000000000..f27b69a195b --- /dev/null +++ b/clippy_dev/src/setup/git_hook.rs @@ -0,0 +1,79 @@ +use std::fs; +use std::path::Path; + +/// Rusts setup uses `git rev-parse --git-common-dir` to get the root directory of the repo. +/// I've decided against this for the sake of simplicity and to make sure that it doesn't install +/// the hook if `clippy_dev` would be used in the rust tree. The hook also references this tool +/// for formatting and should therefor only be used in a normal clone of clippy +const REPO_GIT_DIR: &str = ".git"; +const HOOK_SOURCE_FILE: &str = "util/etc/pre-commit.sh"; +const HOOK_TARGET_FILE: &str = ".git/hooks/pre-commit"; + +pub fn install_hook(force_override: bool) { + if !check_precondition(force_override) { + return; + } + + // So a little bit of a funny story. Git on unix requires the pre-commit file + // to have the `execute` permission to be set. The Rust functions for modifying + // these flags doesn't seem to work when executed with normal user permissions. + // + // However, there is a little hack that is also being used by Rust itself in their + // setup script. Git saves the `execute` flag when syncing files. This means + // that we can check in a file with execution permissions and the sync it to create + // a file with the flag set. We then copy this file here. The copy function will also + // include the `execute` permission. + match fs::copy(HOOK_SOURCE_FILE, HOOK_TARGET_FILE) { + Ok(_) => { + println!("info: the hook can be removed with `cargo dev remove git-hook`"); + println!("git hook successfully installed"); + }, + Err(err) => eprintln!( + "error: unable to copy `{}` to `{}` ({})", + HOOK_SOURCE_FILE, HOOK_TARGET_FILE, err + ), + } +} + +fn check_precondition(force_override: bool) -> bool { + // Make sure that we can find the git repository + let git_path = Path::new(REPO_GIT_DIR); + if !git_path.exists() || !git_path.is_dir() { + eprintln!("error: clippy_dev was unable to find the `.git` directory"); + return false; + } + + // Make sure that we don't override an existing hook by accident + let path = Path::new(HOOK_TARGET_FILE); + if path.exists() { + if force_override { + return delete_git_hook_file(path); + } + + eprintln!("error: there is already a pre-commit hook installed"); + println!("info: use the `--force-override` flag to override the existing hook"); + return false; + } + + true +} + +pub fn remove_hook() { + let path = Path::new(HOOK_TARGET_FILE); + if path.exists() { + if delete_git_hook_file(path) { + println!("git hook successfully removed"); + } + } else { + println!("no pre-commit hook was found"); + } +} + +fn delete_git_hook_file(path: &Path) -> bool { + if let Err(err) = fs::remove_file(path) { + eprintln!("error: unable to delete existing pre-commit git hook ({})", err); + false + } else { + true + } +} diff --git a/clippy_dev/src/setup/intellij.rs b/clippy_dev/src/setup/intellij.rs new file mode 100644 index 00000000000..bf741e6d121 --- /dev/null +++ b/clippy_dev/src/setup/intellij.rs @@ -0,0 +1,223 @@ +use std::fs; +use std::fs::File; +use std::io::prelude::*; +use std::path::{Path, PathBuf}; + +// This module takes an absolute path to a rustc repo and alters the dependencies to point towards +// the respective rustc subcrates instead of using extern crate xyz. +// This allows IntelliJ to analyze rustc internals and show proper information inside Clippy +// code. See https://github.com/rust-lang/rust-clippy/issues/5514 for details + +const RUSTC_PATH_SECTION: &str = "[target.'cfg(NOT_A_PLATFORM)'.dependencies]"; +const DEPENDENCIES_SECTION: &str = "[dependencies]"; + +const CLIPPY_PROJECTS: &[ClippyProjectInfo] = &[ + ClippyProjectInfo::new("root", "Cargo.toml", "src/driver.rs"), + ClippyProjectInfo::new("clippy_lints", "clippy_lints/Cargo.toml", "clippy_lints/src/lib.rs"), + ClippyProjectInfo::new("clippy_utils", "clippy_utils/Cargo.toml", "clippy_utils/src/lib.rs"), +]; + +/// Used to store clippy project information to later inject the dependency into. +struct ClippyProjectInfo { + /// Only used to display information to the user + name: &'static str, + cargo_file: &'static str, + lib_rs_file: &'static str, +} + +impl ClippyProjectInfo { + const fn new(name: &'static str, cargo_file: &'static str, lib_rs_file: &'static str) -> Self { + Self { + name, + cargo_file, + lib_rs_file, + } + } +} + +pub fn setup_rustc_src(rustc_path: &str) { + let rustc_source_dir = match check_and_get_rustc_dir(rustc_path) { + Ok(path) => path, + Err(_) => return, + }; + + for project in CLIPPY_PROJECTS { + if inject_deps_into_project(&rustc_source_dir, project).is_err() { + return; + } + } + + println!("info: the source paths can be removed again with `cargo dev remove intellij`"); +} + +fn check_and_get_rustc_dir(rustc_path: &str) -> Result { + let mut path = PathBuf::from(rustc_path); + + if path.is_relative() { + match path.canonicalize() { + Ok(absolute_path) => { + println!("info: the rustc path was resolved to: `{}`", absolute_path.display()); + path = absolute_path; + }, + Err(err) => { + eprintln!("error: unable to get the absolute path of rustc ({})", err); + return Err(()); + }, + }; + } + + let path = path.join("compiler"); + println!("info: looking for compiler sources at: {}", path.display()); + + if !path.exists() { + eprintln!("error: the given path does not exist"); + return Err(()); + } + + if !path.is_dir() { + eprintln!("error: the given path is not a directory"); + return Err(()); + } + + Ok(path) +} + +fn inject_deps_into_project(rustc_source_dir: &Path, project: &ClippyProjectInfo) -> Result<(), ()> { + let cargo_content = read_project_file(project.cargo_file)?; + let lib_content = read_project_file(project.lib_rs_file)?; + + if inject_deps_into_manifest(rustc_source_dir, project.cargo_file, &cargo_content, &lib_content).is_err() { + eprintln!( + "error: unable to inject dependencies into {} with the Cargo file {}", + project.name, project.cargo_file + ); + Err(()) + } else { + Ok(()) + } +} + +/// `clippy_dev` expects to be executed in the root directory of Clippy. This function +/// loads the given file or returns an error. Having it in this extra function ensures +/// that the error message looks nice. +fn read_project_file(file_path: &str) -> Result { + let path = Path::new(file_path); + if !path.exists() { + eprintln!("error: unable to find the file `{}`", file_path); + return Err(()); + } + + match fs::read_to_string(path) { + Ok(content) => Ok(content), + Err(err) => { + eprintln!("error: the file `{}` could not be read ({})", file_path, err); + Err(()) + }, + } +} + +fn inject_deps_into_manifest( + rustc_source_dir: &Path, + manifest_path: &str, + cargo_toml: &str, + lib_rs: &str, +) -> std::io::Result<()> { + // do not inject deps if we have already done so + if cargo_toml.contains(RUSTC_PATH_SECTION) { + eprintln!( + "warn: dependencies are already setup inside {}, skipping file", + manifest_path + ); + return Ok(()); + } + + let extern_crates = lib_rs + .lines() + // only take dependencies starting with `rustc_` + .filter(|line| line.starts_with("extern crate rustc_")) + // we have something like "extern crate foo;", we only care about the "foo" + // extern crate rustc_middle; + // ^^^^^^^^^^^^ + .map(|s| &s[13..(s.len() - 1)]); + + let new_deps = extern_crates.map(|dep| { + // format the dependencies that are going to be put inside the Cargo.toml + format!( + "{dep} = {{ path = \"{source_path}/{dep}\" }}\n", + dep = dep, + source_path = rustc_source_dir.display() + ) + }); + + // format a new [dependencies]-block with the new deps we need to inject + let mut all_deps = String::from("[target.'cfg(NOT_A_PLATFORM)'.dependencies]\n"); + new_deps.for_each(|dep_line| { + all_deps.push_str(&dep_line); + }); + all_deps.push_str("\n[dependencies]\n"); + + // replace "[dependencies]" with + // [dependencies] + // dep1 = { path = ... } + // dep2 = { path = ... } + // etc + let new_manifest = cargo_toml.replacen("[dependencies]\n", &all_deps, 1); + + // println!("{}", new_manifest); + let mut file = File::create(manifest_path)?; + file.write_all(new_manifest.as_bytes())?; + + println!("info: successfully setup dependencies inside {}", manifest_path); + + Ok(()) +} + +pub fn remove_rustc_src() { + for project in CLIPPY_PROJECTS { + remove_rustc_src_from_project(project); + } +} + +fn remove_rustc_src_from_project(project: &ClippyProjectInfo) -> bool { + let mut cargo_content = if let Ok(content) = read_project_file(project.cargo_file) { + content + } else { + return false; + }; + let section_start = if let Some(section_start) = cargo_content.find(RUSTC_PATH_SECTION) { + section_start + } else { + println!( + "info: dependencies could not be found in `{}` for {}, skipping file", + project.cargo_file, project.name + ); + return true; + }; + + let end_point = if let Some(end_point) = cargo_content.find(DEPENDENCIES_SECTION) { + end_point + } else { + eprintln!( + "error: the end of the rustc dependencies section could not be found in `{}`", + project.cargo_file + ); + return false; + }; + + cargo_content.replace_range(section_start..end_point, ""); + + match File::create(project.cargo_file) { + Ok(mut file) => { + file.write_all(cargo_content.as_bytes()).unwrap(); + println!("info: successfully removed dependencies inside {}", project.cargo_file); + true + }, + Err(err) => { + eprintln!( + "error: unable to open file `{}` to remove rustc dependencies for {} ({})", + project.cargo_file, project.name, err + ); + false + }, + } +} diff --git a/clippy_dev/src/setup/mod.rs b/clippy_dev/src/setup/mod.rs new file mode 100644 index 00000000000..3834f5a1842 --- /dev/null +++ b/clippy_dev/src/setup/mod.rs @@ -0,0 +1,2 @@ +pub mod git_hook; +pub mod intellij; diff --git a/doc/basics.md b/doc/basics.md index e2e307ce4f6..89d572ad931 100644 --- a/doc/basics.md +++ b/doc/basics.md @@ -90,8 +90,10 @@ cargo dev fmt cargo dev update_lints # create a new lint and register it cargo dev new_lint +# automatically formatting all code before each commit +cargo dev setup git-hook # (experimental) Setup Clippy to work with IntelliJ-Rust -cargo dev ide_setup +cargo dev setup intellij ``` ## lintcheck diff --git a/util/etc/pre-commit.sh b/util/etc/pre-commit.sh new file mode 100755 index 00000000000..528f8953b25 --- /dev/null +++ b/util/etc/pre-commit.sh @@ -0,0 +1,21 @@ +#!/bin/sh + +# hide output +set -e + +# Update lints +cargo dev update_lints +git add clippy_lints/src/lib.rs + +# Formatting: +# Git will not automatically add the formatted code to the staged changes once +# fmt was executed. This collects all staged files rs files that are currently staged. +# They will later be added back. +# +# This was proudly stolen and adjusted from here: +# https://medium.com/@harshitbangar/automatic-code-formatting-with-git-66c3c5c26798 +files=$( (git diff --cached --name-only --diff-filter=ACMR | grep -Ei "\.rs$") || true) +if [ ! -z "${files}" ]; then + cargo dev fmt + git add $(echo "$files" | paste -s -d " " -) +fi