Auto merge of #11828 - weihanglo:clippy-disallowed-methods, r=epage

clippy: warn `disallowed_methods` for `std::env::var` and friends

Author: bors

build.rs

@@ -7,13 +7,15 @@ use std::process::Command;
 fn main() {
     commit_info();
     compress_man();
-    println!(
-        "cargo:rustc-env=RUST_HOST_TARGET={}",
-        std::env::var("TARGET").unwrap()
-    );
+    // ALLOWED: Accessing environment during build time shouldn't be prohibited.
+    #[allow(clippy::disallowed_methods)]
+    let target = std::env::var("TARGET").unwrap();
+    println!("cargo:rustc-env=RUST_HOST_TARGET={target}");
 }
 
 fn compress_man() {
+    // ALLOWED: Accessing environment during build time shouldn't be prohibited.
+    #[allow(clippy::disallowed_methods)]
     let out_path = Path::new(&std::env::var("OUT_DIR").unwrap()).join("man.tgz");
     let dst = fs::File::create(out_path).unwrap();
     let encoder = GzBuilder::new()

clippy.toml

@@ -0,0 +1,6 @@
+disallowed-methods = [
+    { path = "std::env::var", reason = "Use `Config::get_env` instead. See rust-lang/cargo#11588" },
+    { path = "std::env::var_os", reason = "Use `Config::get_env_os` instead. See rust-lang/cargo#11588" },
+    { path = "std::env::vars", reason = "Not recommended to use in Cargo. See rust-lang/cargo#11588" },
+    { path = "std::env::vars_os", reason = "Not recommended to use in Cargo. See rust-lang/cargo#11588" },
+]

src/bin/cargo/cli.rs

@@ -413,6 +413,9 @@ impl GlobalArgs {
 }
 
 pub fn cli() -> Command {
+    // ALLOWED: `RUSTUP_HOME` should only be read from process env, otherwise
+    // other tools may point to executables from incompatible distributions.
+    #[allow(clippy::disallowed_methods)]
     let is_rustup = std::env::var_os("RUSTUP_HOME").is_some();
     let usage = if is_rustup {
         "cargo [+toolchain] [OPTIONS] [COMMAND]"

src/bin/cargo/main.rs

@@ -1,5 +1,6 @@
 #![warn(rust_2018_idioms)] // while we're getting used to 2018
 #![allow(clippy::all)]
+#![warn(clippy::disallowed_methods)]
 
 use cargo::util::toml::StringOrVec;
 use cargo::util::CliError;

src/cargo/core/compiler/custom_build.rs

@@ -410,6 +410,10 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Job> {
                 // If we're opting into backtraces, mention that build dependencies' backtraces can
                 // be improved by requesting debuginfo to be built, if we're not building with
                 // debuginfo already.
+                //
+                // ALLOWED: Other tools like `rustc` might read it directly
+                // through `std::env`. We should make their behavior consistent.
+                #[allow(clippy::disallowed_methods)]
                 if let Ok(show_backtraces) = std::env::var("RUST_BACKTRACE") {
                     if !built_with_debuginfo && show_backtraces != "0" {
                         build_error_context.push_str(&format!(
@@ -727,6 +731,10 @@ impl BuildOutput {
                                 None => return false,
                                 Some(n) => n,
                             };
+                            // ALLOWED: the process of rustc boostrapping reads this through
+                            // `std::env`. We should make the behavior consistent. Also, we
+                            // don't advertise this for bypassing nightly.
+                            #[allow(clippy::disallowed_methods)]
                             std::env::var("RUSTC_BOOTSTRAP")
                                 .map_or(false, |var| var.split(',').any(|s| s == name))
                         };

src/cargo/core/compiler/fingerprint/mod.rs

@@ -764,6 +764,19 @@ pub enum StaleItem {
 }
 
 impl LocalFingerprint {
+    /// Read the environment variable of the given env `key`, and creates a new
+    /// [`LocalFingerprint::RerunIfEnvChanged`] for it.
+    ///
+    // TODO: This is allowed at this moment. Should figure out if it makes
+    // sense if permitting to read env from the config system.
+    #[allow(clippy::disallowed_methods)]
+    fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint {
+        let key = key.as_ref();
+        let var = key.to_owned();
+        let val = env::var(key).ok();
+        LocalFingerprint::RerunIfEnvChanged { var, val }
+    }
+
     /// Checks dynamically at runtime if this `LocalFingerprint` has a stale
     /// item inside of it.
     ///
@@ -1661,10 +1674,7 @@ fn local_fingerprints_deps(
     local.extend(
         deps.rerun_if_env_changed
             .iter()
-            .map(|var| LocalFingerprint::RerunIfEnvChanged {
-                var: var.clone(),
-                val: env::var(var).ok(),
-            }),
+            .map(LocalFingerprint::from_env),
     );
 
     local

src/cargo/core/features.rs

@@ -946,10 +946,7 @@ impl CliUnstable {
             self.add(flag, &mut warnings)?;
         }
 
-        if self.gitoxide.is_none()
-            && std::env::var_os("__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2")
-                .map_or(false, |value| value == "1")
-        {
+        if self.gitoxide.is_none() && cargo_use_gitoxide_instead_of_git2() {
             self.gitoxide = GitoxideFeatures::safe().into();
         }
         Ok(warnings)
@@ -1171,9 +1168,15 @@ impl CliUnstable {
 
 /// Returns the current release channel ("stable", "beta", "nightly", "dev").
 pub fn channel() -> String {
+    // ALLOWED: For testing cargo itself only.
+    #[allow(clippy::disallowed_methods)]
     if let Ok(override_channel) = env::var("__CARGO_TEST_CHANNEL_OVERRIDE_DO_NOT_USE_THIS") {
         return override_channel;
     }
+    // ALLOWED: the process of rustc boostrapping reads this through
+    // `std::env`. We should make the behavior consistent. Also, we
+    // don't advertise this for bypassing nightly.
+    #[allow(clippy::disallowed_methods)]
     if let Ok(staging) = env::var("RUSTC_BOOTSTRAP") {
         if staging == "1" {
             return "dev".to_string();
@@ -1183,3 +1186,12 @@ pub fn channel() -> String {
         .release_channel
         .unwrap_or_else(|| String::from("dev"))
 }
+
+/// Only for testing and developing. See ["Running with gitoxide as default git backend in tests"][1].
+///
+/// [1]: https://doc.crates.io/contrib/tests/running.html#running-with-gitoxide-as-default-git-backend-in-tests
+// ALLOWED: For testing cargo itself only.
+#[allow(clippy::disallowed_methods)]
+fn cargo_use_gitoxide_instead_of_git2() -> bool {
+    std::env::var_os("__CARGO_USE_GITOXIDE_INSTEAD_OF_GIT2").map_or(false, |value| value == "1")
+}

src/cargo/core/resolver/types.rs

@@ -15,6 +15,9 @@ pub struct ResolverProgress {
     time_to_print: Duration,
     printed: bool,
     deps_time: Duration,
+    /// Provides an escape hatch for machine with slow CPU for debugging and
+    /// testing Cargo itself.
+    /// See [rust-lang/cargo#6596](https://github.com/rust-lang/cargo/pull/6596) for more.
     #[cfg(debug_assertions)]
     slow_cpu_multiplier: u64,
 }
@@ -31,6 +34,9 @@ impl ResolverProgress {
             // Architectures that do not have a modern processor, hardware emulation, etc.
             // In the test code we have `slow_cpu_multiplier`, but that is not accessible here.
             #[cfg(debug_assertions)]
+            // ALLOWED: For testing cargo itself only. However, it was communicated as an public
+            // interface to other developers, so keep it as-is, shouldn't add `__CARGO` prefix.
+            #[allow(clippy::disallowed_methods)]
             slow_cpu_multiplier: std::env::var("CARGO_TEST_SLOW_CPU_MULTIPLIER")
                 .ok()
                 .and_then(|m| m.parse().ok())

src/cargo/core/shell.rs

@@ -17,6 +17,8 @@ impl TtyWidth {
     /// Returns the width of the terminal to use for diagnostics (which is
     /// relayed to rustc via `--diagnostic-width`).
     pub fn diagnostic_terminal_width(&self) -> Option<usize> {
+        // ALLOWED: For testing cargo itself only.
+        #[allow(clippy::disallowed_methods)]
         if let Ok(width) = std::env::var("__CARGO_TEST_TTY_WIDTH_DO_NOT_USE_THIS") {
             return Some(width.parse().unwrap());
         }

src/cargo/core/source/source_id.rs

@@ -428,9 +428,7 @@ impl SourceId {
             _ => return false,
         }
         let url = self.inner.url.as_str();
-        url == CRATES_IO_INDEX
-            || url == CRATES_IO_HTTP_INDEX
-            || std::env::var("__CARGO_TEST_CRATES_IO_URL_DO_NOT_USE_THIS").as_deref() == Ok(url)
+        url == CRATES_IO_INDEX || url == CRATES_IO_HTTP_INDEX || is_overridden_crates_io_url(url)
     }
 
     /// Hashes `self`.
@@ -884,3 +882,10 @@ mod tests {
         assert_eq!(source_id, deserialized);
     }
 }
+
+/// Check if `url` equals to the overridden crates.io URL.
+// ALLOWED: For testing Cargo itself only.
+#[allow(clippy::disallowed_methods)]
+fn is_overridden_crates_io_url(url: &str) -> bool {
+    std::env::var("__CARGO_TEST_CRATES_IO_URL_DO_NOT_USE_THIS").map_or(false, |v| v == url)
+}

src/cargo/lib.rs

@@ -4,6 +4,7 @@
 // Due to some of the default clippy lints being somewhat subjective and not
 // necessarily an improvement, we prefer to not use them at this time.
 #![allow(clippy::all)]
+#![warn(clippy::disallowed_methods)]
 #![warn(clippy::self_named_module_files)]
 #![allow(rustdoc::private_intra_doc_links)]
 

src/cargo/ops/fix.rs

@@ -17,10 +17,10 @@
 //! Cargo begins a normal `cargo check` operation with itself set as a proxy
 //! for rustc by setting `primary_unit_rustc` in the build config. When
 //! cargo launches rustc to check a crate, it is actually launching itself.
-//! The `FIX_ENV` environment variable is set so that cargo knows it is in
+//! The `FIX_ENV_INTERNAL` environment variable is set so that cargo knows it is in
 //! fix-proxy-mode.
 //!
-//! Each proxied cargo-as-rustc detects it is in fix-proxy-mode (via `FIX_ENV`
+//! Each proxied cargo-as-rustc detects it is in fix-proxy-mode (via `FIX_ENV_INTERNAL`
 //! environment variable in `main`) and does the following:
 //!
 //! - Acquire a lock from the `LockServer` from the master cargo process.
@@ -63,10 +63,20 @@ use crate::util::Config;
 use crate::util::{existing_vcs_repo, LockServer, LockServerClient};
 use crate::{drop_eprint, drop_eprintln};
 
-const FIX_ENV: &str = "__CARGO_FIX_PLZ";
-const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE";
-const EDITION_ENV: &str = "__CARGO_FIX_EDITION";
-const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS";
+/// **Internal only.**
+/// Indicates Cargo is in fix-proxy-mode if presents.
+/// The value of it is the socket address of the [`LockServer`] being used.
+/// See the [module-level documentation](mod@super::fix) for more.
+const FIX_ENV_INTERNAL: &str = "__CARGO_FIX_PLZ";
+/// **Internal only.**
+/// For passing [`FixOptions::broken_code`] through to cargo running in proxy mode.
+const BROKEN_CODE_ENV_INTERNAL: &str = "__CARGO_FIX_BROKEN_CODE";
+/// **Internal only.**
+/// For passing [`FixOptions::edition`] through to cargo running in proxy mode.
+const EDITION_ENV_INTERNAL: &str = "__CARGO_FIX_EDITION";
+/// **Internal only.**
+/// For passing [`FixOptions::idioms`] through to cargo running in proxy mode.
+const IDIOMS_ENV_INTERNAL: &str = "__CARGO_FIX_IDIOMS";
 
 pub struct FixOptions {
     pub edition: bool,
@@ -87,20 +97,20 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions) -> CargoResult<()> {
     // Spin up our lock server, which our subprocesses will use to synchronize fixes.
     let lock_server = LockServer::new()?;
     let mut wrapper = ProcessBuilder::new(env::current_exe()?);
-    wrapper.env(FIX_ENV, lock_server.addr().to_string());
+    wrapper.env(FIX_ENV_INTERNAL, lock_server.addr().to_string());
     let _started = lock_server.start()?;
 
     opts.compile_opts.build_config.force_rebuild = true;
 
     if opts.broken_code {
-        wrapper.env(BROKEN_CODE_ENV, "1");
+        wrapper.env(BROKEN_CODE_ENV_INTERNAL, "1");
     }
 
     if opts.edition {
-        wrapper.env(EDITION_ENV, "1");
+        wrapper.env(EDITION_ENV_INTERNAL, "1");
     }
     if opts.idioms {
-        wrapper.env(IDIOMS_ENV, "1");
+        wrapper.env(IDIOMS_ENV_INTERNAL, "1");
     }
 
     *opts
@@ -339,7 +349,10 @@ to prevent this issue from happening.
 /// Returns `None` if `fix` is not being run (not in proxy mode). Returns
 /// `Some(...)` if in `fix` proxy mode
 pub fn fix_get_proxy_lock_addr() -> Option<String> {
-    env::var(FIX_ENV).ok()
+    // ALLOWED: For the internal mechanism of `cargo fix` only.
+    // Shouldn't be set directly by anyone.
+    #[allow(clippy::disallowed_methods)]
+    env::var(FIX_ENV_INTERNAL).ok()
 }
 
 /// Entry point for `cargo` running as a proxy for `rustc`.
@@ -360,7 +373,7 @@ pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> {
         .ok();
     let mut rustc = ProcessBuilder::new(&args.rustc).wrapped(workspace_rustc.as_ref());
     rustc.retry_with_argfile(true);
-    rustc.env_remove(FIX_ENV);
+    rustc.env_remove(FIX_ENV_INTERNAL);
     args.apply(&mut rustc);
 
     trace!("start rustfixing {:?}", args.file);
@@ -404,7 +417,7 @@ pub fn fix_exec_rustc(config: &Config, lock_addr: &str) -> CargoResult<()> {
         // user's code with our changes. Back out everything and fall through
         // below to recompile again.
         if !output.status.success() {
-            if config.get_env_os(BROKEN_CODE_ENV).is_none() {
+            if config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_none() {
                 for (path, file) in fixes.files.iter() {
                     debug!("reverting {:?} due to errors", path);
                     paths::write(path, &file.original_code)?;
@@ -578,7 +591,7 @@ fn rustfix_and_fix(
     // worse by applying fixes where a bug could cause *more* broken code.
     // Instead, punt upwards which will reexec rustc over the original code,
     // displaying pretty versions of the diagnostics we just read out.
-    if !output.status.success() && config.get_env_os(BROKEN_CODE_ENV).is_none() {
+    if !output.status.success() && config.get_env_os(BROKEN_CODE_ENV_INTERNAL).is_none() {
         debug!(
             "rustfixing `{:?}` failed, rustc exited with {:?}",
             filename,
@@ -847,9 +860,15 @@ impl FixArgs {
         }
 
         let file = file.ok_or_else(|| anyhow::anyhow!("could not find .rs file in rustc args"))?;
-        let idioms = env::var(IDIOMS_ENV).is_ok();
-
-        let prepare_for_edition = env::var(EDITION_ENV).ok().map(|_| {
+        // ALLOWED: For the internal mechanism of `cargo fix` only.
+        // Shouldn't be set directly by anyone.
+        #[allow(clippy::disallowed_methods)]
+        let idioms = env::var(IDIOMS_ENV_INTERNAL).is_ok();
+
+        // ALLOWED: For the internal mechanism of `cargo fix` only.
+        // Shouldn't be set directly by anyone.
+        #[allow(clippy::disallowed_methods)]
+        let prepare_for_edition = env::var(EDITION_ENV_INTERNAL).ok().map(|_| {
             enabled_edition
                 .unwrap_or(Edition::Edition2015)
                 .saturating_next()

src/cargo/util/config/environment.rs

@@ -66,6 +66,11 @@ pub struct Env {
 impl Env {
     /// Create a new `Env` from process's environment variables.
     pub fn new() -> Self {
+        // ALLOWED: This is the only permissible usage of `std::env::vars{_os}`
+        // within cargo. If you do need access to individual variables without
+        // interacting with `Config` system, please use `std::env::var{_os}`
+        // and justify the validity of the usage.
+        #[allow(clippy::disallowed_methods)]
         let env: HashMap<_, _> = std::env::vars_os().collect();
         let (case_insensitive_env, normalized_env) = make_case_insensitive_and_normalized_env(&env);
         Self {

src/cargo/util/job.rs

@@ -32,6 +32,9 @@ mod imp {
         // when-cargo-is-killed-subprocesses-are-also-killed, but that requires
         // one cargo spawned to become its own session leader, so we do that
         // here.
+        //
+        // ALLOWED: For testing cargo itself only.
+        #[allow(clippy::disallowed_methods)]
         if env::var("__CARGO_TEST_SETSID_PLEASE_DONT_USE_ELSEWHERE").is_ok() {
             libc::setsid();
         }

src/cargo/util/profile.rs

@@ -22,6 +22,8 @@ pub struct Profiler {
 }
 
 fn enabled_level() -> Option<usize> {
+    // ALLOWED: for profiling Cargo itself, not intended to be used beyond Cargo contributors.
+    #[allow(clippy::disallowed_methods)]
     env::var("CARGO_PROFILE").ok().and_then(|s| s.parse().ok())
 }