Auto merge of #6450 - matthiaskrgr:dont_format_local_repo, r=ebroto

bors · bors · commit a642b42a46e1 · 2020-12-13T18:07:57.000Z
cargo dev fmt: don't format entire rustc repo if we ran ra_setup previously

It turns out that rustfmt sees a rustc repo that we pulled in as path dependency via `cargo dev ra-setup` as part of the tree and would try to format it :D

Of course we don't want this, so skip formatting if we see that we ran `ra-setup` previously.

changelog: none
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -19,10 +19,10 @@ All contributors are expected to follow the [Rust Code of Conduct].
   - [Writing code](#writing-code)
   - [Getting code-completion for rustc internals to work](#getting-code-completion-for-rustc-internals-to-work)
   - [How Clippy works](#how-clippy-works)
-  - [Fixing build failures caused by Rust](#fixing-build-failures-caused-by-rust)
+  - [Syncing changes between Clippy and [`rust-lang/rust`]](#syncing-changes-between-clippy-and-rust-langrust)
     - [Patching git-subtree to work with big repos](#patching-git-subtree-to-work-with-big-repos)
-    - [Performing the sync](#performing-the-sync)
-    - [Syncing back changes in Clippy to [`rust-lang/rust`]](#syncing-back-changes-in-clippy-to-rust-langrust)
+    - [Performing the sync from [`rust-lang/rust`] to Clippy](#performing-the-sync-from-rust-langrust-to-clippy)
+    - [Performing the sync from Clippy to [`rust-lang/rust`]](#performing-the-sync-from-clippy-to-rust-langrust)
     - [Defining remotes](#defining-remotes)
   - [Issue and PR triage](#issue-and-pr-triage)
   - [Bors and Homu](#bors-and-homu)
@@ -111,7 +111,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 rust-analyzer will be able to understand.
-Run `cargo dev ra-setup --repo-path <repo-path>` where `<repo-path>` is an absolute path to the rustc repo
+Run `cargo dev ra_setup --repo-path <repo-path>` where `<repo-path>` is an absolute 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 rust-analyzer to understand most of the types that Clippy uses.
diff --git a/clippy_dev/src/fmt.rs b/clippy_dev/src/fmt.rs
@@ -1,9 +1,9 @@
 use crate::clippy_project_root;
 use shell_escape::escape;
 use std::ffi::OsStr;
-use std::io;
 use std::path::Path;
 use std::process::{self, Command};
+use std::{fs, io};
 use walkdir::WalkDir;
 
 #[derive(Debug)]
@@ -12,6 +12,7 @@ pub enum CliError {
     IoError(io::Error),
     RustfmtNotInstalled,
     WalkDirError(walkdir::Error),
+    RaSetupActive,
 }
 
 impl From<io::Error> for CliError {
@@ -31,12 +32,23 @@ struct FmtContext {
     verbose: bool,
 }
 
+// the "main" function of cargo dev fmt
 pub fn run(check: bool, verbose: bool) {
     fn try_run(context: &FmtContext) -> Result<bool, CliError> {
         let mut success = true;
 
         let project_root = clippy_project_root();
 
+        // if we added a local rustc repo as path dependency to clippy for rust analyzer, we do NOT want to
+        // format because rustfmt would also format the entire rustc repo as it is a local
+        // dependency
+        if fs::read_to_string(project_root.join("Cargo.toml"))
+            .expect("Failed to read clippy Cargo.toml")
+            .contains(&"[target.'cfg(NOT_A_PLATFORM)'.dependencies]")
+        {
+            return Err(CliError::RaSetupActive);
+        }
+
         rustfmt_test(context)?;
 
         success &= cargo_fmt(context, project_root.as_path())?;
@@ -75,6 +87,13 @@ pub fn run(check: bool, verbose: bool) {
             CliError::WalkDirError(err) => {
                 eprintln!("error: {}", err);
             },
+            CliError::RaSetupActive => {
+                eprintln!(
+                    "error: a local rustc repo is enabled as path dependency via `cargo dev ra_setup`.
+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/main.rs b/clippy_dev/src/main.rs
@@ -1,10 +1,52 @@
 #![cfg_attr(feature = "deny-warnings", deny(warnings))]
 
-use clap::{App, Arg, SubCommand};
+use clap::{App, Arg, ArgMatches, SubCommand};
 use clippy_dev::{bless, fmt, new_lint, ra_setup, serve, stderr_length_check, update_lints};
 
 fn main() {
-    let matches = App::new("Clippy developer tooling")
+    let matches = get_clap_config();
+
+    match matches.subcommand() {
+        ("bless", Some(_)) => {
+            bless::bless();
+        },
+        ("fmt", Some(matches)) => {
+            fmt::run(matches.is_present("check"), matches.is_present("verbose"));
+        },
+        ("update_lints", Some(matches)) => {
+            if matches.is_present("print-only") {
+                update_lints::print_lints();
+            } else if matches.is_present("check") {
+                update_lints::run(update_lints::UpdateMode::Check);
+            } else {
+                update_lints::run(update_lints::UpdateMode::Change);
+            }
+        },
+        ("new_lint", Some(matches)) => {
+            match new_lint::create(
+                matches.value_of("pass"),
+                matches.value_of("name"),
+                matches.value_of("category"),
+            ) {
+                Ok(_) => update_lints::run(update_lints::UpdateMode::Change),
+                Err(e) => eprintln!("Unable to create lint: {}", e),
+            }
+        },
+        ("limit_stderr_length", _) => {
+            stderr_length_check::check();
+        },
+        ("ra_setup", Some(matches)) => ra_setup::run(matches.value_of("rustc-repo-path")),
+        ("serve", Some(matches)) => {
+            let port = matches.value_of("port").unwrap().parse().unwrap();
+            let lint = matches.value_of("lint");
+            serve::run(port, lint);
+        },
+        _ => {},
+    }
+}
+
+fn get_clap_config<'a>() -> ArgMatches<'a> {
+    App::new("Clippy developer tooling")
         .subcommand(SubCommand::with_name("bless").about("bless the test output changes"))
         .subcommand(
             SubCommand::with_name("fmt")
@@ -26,16 +68,16 @@ fn main() {
                 .about("Updates lint registration and information from the source code")
                 .long_about(
                     "Makes sure that:\n \
-                     * the lint count in README.md is correct\n \
-                     * the changelog contains markdown link references at the bottom\n \
-                     * all lint groups include the correct lints\n \
-                     * lint modules in `clippy_lints/*` are visible in `src/lib.rs` via `pub mod`\n \
-                     * all lints are registered in the lint store",
+                 * the lint count in README.md is correct\n \
+                 * the changelog contains markdown link references at the bottom\n \
+                 * all lint groups include the correct lints\n \
+                 * lint modules in `clippy_lints/*` are visible in `src/lifb.rs` via `pub mod`\n \
+                 * all lints are registered in the lint store",
                 )
                 .arg(Arg::with_name("print-only").long("print-only").help(
                     "Print a table of lints to STDOUT. \
-                     This does not include deprecated and internal lints. \
-                     (Does not modify any files)",
+                 This does not include deprecated and internal lints. \
+                 (Does not modify any files)",
                 ))
                 .arg(
                     Arg::with_name("check")
@@ -89,7 +131,7 @@ fn main() {
                 .about("Ensures that stderr files do not grow longer than a certain amount of lines."),
         )
         .subcommand(
-            SubCommand::with_name("ra-setup")
+            SubCommand::with_name("ra_setup")
                 .about("Alter dependencies so rust-analyzer can find rustc internals")
                 .arg(
                     Arg::with_name("rustc-repo-path")
@@ -114,43 +156,5 @@ fn main() {
                 )
                 .arg(Arg::with_name("lint").help("Which lint's page to load initially (optional)")),
         )
-        .get_matches();
-
-    match matches.subcommand() {
-        ("bless", Some(_)) => {
-            bless::bless();
-        },
-        ("fmt", Some(matches)) => {
-            fmt::run(matches.is_present("check"), matches.is_present("verbose"));
-        },
-        ("update_lints", Some(matches)) => {
-            if matches.is_present("print-only") {
-                update_lints::print_lints();
-            } else if matches.is_present("check") {
-                update_lints::run(update_lints::UpdateMode::Check);
-            } else {
-                update_lints::run(update_lints::UpdateMode::Change);
-            }
-        },
-        ("new_lint", Some(matches)) => {
-            match new_lint::create(
-                matches.value_of("pass"),
-                matches.value_of("name"),
-                matches.value_of("category"),
-            ) {
-                Ok(_) => update_lints::run(update_lints::UpdateMode::Change),
-                Err(e) => eprintln!("Unable to create lint: {}", e),
-            }
-        },
-        ("limit_stderr_length", _) => {
-            stderr_length_check::check();
-        },
-        ("ra-setup", Some(matches)) => ra_setup::run(matches.value_of("rustc-repo-path")),
-        ("serve", Some(matches)) => {
-            let port = matches.value_of("port").unwrap().parse().unwrap();
-            let lint = matches.value_of("lint");
-            serve::run(port, lint);
-        },
-        _ => {},
-    }
+        .get_matches()
 }
diff --git a/clippy_dev/src/ra_setup.rs b/clippy_dev/src/ra_setup.rs
@@ -52,7 +52,7 @@ fn inject_deps_into_manifest(
     // do not inject deps if we have aleady done so
     if cargo_toml.contains("[target.'cfg(NOT_A_PLATFORM)'.dependencies]") {
         eprintln!(
-            "cargo dev ra-setup: warning: deps already found inside {}, doing nothing.",
+            "cargo dev ra_setup: warning: deps already found inside {}, doing nothing.",
             manifest_path
         );
         return Ok(());
diff --git a/doc/basics.md b/doc/basics.md
@@ -8,7 +8,7 @@ the codebase take a look at [Adding Lints] or [Common Tools].
 [Common Tools]: https://github.com/rust-lang/rust-clippy/blob/master/doc/common_tools_writing_lints.md
 
 - [Basics for hacking on Clippy](#basics-for-hacking-on-clippy)
-  - [Get the code](#get-the-code)
+  - [Get the Code](#get-the-code)
   - [Building and Testing](#building-and-testing)
   - [`cargo dev`](#cargo-dev)
   - [PR](#pr)
@@ -87,7 +87,7 @@ cargo dev update_lints
 # create a new lint and register it
 cargo dev new_lint
 # (experimental) Setup Clippy to work with rust-analyzer
-cargo dev ra-setup
+cargo dev ra_setup
 ```
 
 ## PR
