Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clippy: warn disallowed_methods for std::env::var and friends #11828

Merged
merged 3 commits into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
epage marked this conversation as resolved.
Show resolved Hide resolved
}

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()
Expand Down
3 changes: 3 additions & 0 deletions src/bin/cargo/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]"
Expand Down
8 changes: 8 additions & 0 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
epage marked this conversation as resolved.
Show resolved Hide resolved
if let Ok(show_backtraces) = std::env::var("RUST_BACKTRACE") {
if !built_with_debuginfo && show_backtraces != "0" {
build_error_context.push_str(&format!(
Expand Down Expand Up @@ -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))
};
Expand Down
18 changes: 14 additions & 4 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that on the current master cargo, when changing variable inside [env] config table, build script won't rerun.

LocalFingerprint::RerunIfEnvChanged { var, val }
}

/// Checks dynamically at runtime if this `LocalFingerprint` has a stale
/// item inside of it.
///
Expand Down Expand Up @@ -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
Expand Down
20 changes: 16 additions & 4 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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") {
epage marked this conversation as resolved.
Show resolved Hide resolved
if staging == "1" {
return "dev".to_string();
Expand All @@ -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")
}
6 changes: 6 additions & 0 deletions src/cargo/core/resolver/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand All @@ -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())
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/core/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
11 changes: 8 additions & 3 deletions src/cargo/core/source/source_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -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)
}
19 changes: 19 additions & 0 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,19 @@ use crate::util::Config;
use crate::util::{existing_vcs_repo, LockServer, LockServerClient};
use crate::{drop_eprint, drop_eprintln};

/// **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: &str = "__CARGO_FIX_PLZ";
/// **Internal only.**
/// For passing [`FixOptions::broken_code`] through to cargo running in proxy mode.
const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE";
/// **Internal only.**
/// For passing [`FixOptions::edition`] through to cargo running in proxy mode.
const EDITION_ENV: &str = "__CARGO_FIX_EDITION";
/// **Internal only.**
/// For passing [`FixOptions::idioms`] through to cargo running in proxy mode.
const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS";

pub struct FixOptions {
Expand Down Expand Up @@ -339,6 +349,9 @@ 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> {
// ALLOWED: For the internal mechanism of `cargo fix` only.
// Shouldn't be set directly by anyone.
#[allow(clippy::disallowed_methods)]
env::var(FIX_ENV).ok()
}

Expand Down Expand Up @@ -847,8 +860,14 @@ impl FixArgs {
}

let file = file.ok_or_else(|| anyhow::anyhow!("could not find .rs file in rustc args"))?;
// 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).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).ok().map(|_| {
enabled_edition
.unwrap_or(Edition::Edition2015)
Expand Down
5 changes: 5 additions & 0 deletions src/cargo/util/config/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/util/job.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
2 changes: 2 additions & 0 deletions src/cargo/util/profile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}

Expand Down