clippy: warn `disallowed_methods` for `std::env::var` and friends by weihanglo · Pull Request #11828 · rust-lang/cargo

weihanglo · 2023-03-10T12:44:19Z

What does this PR try to resolve?

In #11588 we want to avoid reading from environment variables, #11727 did that well. I wonder if we could leverage tools to help with this. Thankfully, clippy has a disallowed_methods lint, helping us enforce the rule.

I am trying to dogfood tools from rust-lang org :)

How should we test and review this PR?

Before staring a review, let's make decisions on these topics.

We need to scrutinize each usage of std::env::var and std::env::var_os. Please review comments above each usage of those.

Additional information

Some variables are read from snapshot starting from #11727 might need double checks. See #11588 (comment).

rustbot · 2023-03-10T12:44:24Z

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

src/cargo/ops/fix.rs

src/cargo/util/profile.rs

src/cargo/core/features.rs

weihanglo

I think we need to make decisions on these things (in this order):

Should we leverage clippy::disallowed_methods in our codebase?
Do we want to have a centralized place for exceptions of env var access?

src/cargo/core/compiler/custom_build.rs

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

epage · 2023-03-10T17:04:12Z

Should we leverage clippy::disallowed_methods in our codebase?

I think it can be a useful tool

Do we want to have a centralized place for exceptions of env var access?

I lean towards "no" but am fine if we do end up centralizing. Its also something we can always change later with minimal impact, so its not a big deal either way.

weihanglo · 2023-03-10T17:16:41Z

Before starting the scrutiny of all environment variable access. Let's see if we agree on this:

@rfcbot poll T-cargo "Should we leverage clippy::disallowed_methods in our codebase?"

rfcbot · 2023-03-10T17:16:42Z

Team member @weihanglo has asked teams: T-cargo, for consensus on:

"Should we leverage clippy::disallowed_methods in our codebase?"

bors · 2023-03-17T02:12:03Z

☔ The latest upstream changes (presumably #11824) made this pull request unmergeable. Please resolve the merge conflicts.

In 11588 we want to avoid reading from environment variables, 11727 did that well. I wonder if we could leverage tools to help with this. Thankfully, clippy has a `disallowed_methods`[1] lint, helping us enforce the rule. [1]: https://rust-lang.github.io/rust-clippy/stable/index.html#disallowed_methods

See comments above for each usage about why it is allowed.

weihanglo · 2023-03-17T13:01:12Z

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

+    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();


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

weihanglo · 2023-03-17T13:03:34Z

This is ready to review. The examination of all usages of Config::get_env{_os} will come as a follow-up PR.

build.rs

epage · 2023-03-17T18:37:44Z

@bors r+

bors · 2023-03-17T18:37:46Z

📌 Commit 1071cce has been approved by epage

It is now in the queue for this repository.

bors · 2023-03-17T18:37:53Z

⌛ Testing commit 1071cce with merge 99ad42d...

bors · 2023-03-17T19:24:51Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing 99ad42d to master...

Update cargo 11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3 2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000 - docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870) - docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869) - Update curl-sys (rust-lang/cargo#11871) - Poll loop fixes (rust-lang/cargo#11624) - clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828) - Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859) - Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824) - align semantics of generated vcs ignore files (rust-lang/cargo#11855) - Add more information to wait-for-publish (rust-lang/cargo#11713) - docs: Address warnings (rust-lang/cargo#11856) - docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)

rustbot assigned ehuss Mar 10, 2023

weihanglo force-pushed the clippy-disallowed-methods branch 2 times, most recently from a00a551 to 290c0dd Compare March 10, 2023 13:06

rustbot added the A-configuration Area: cargo config files and env vars label Mar 10, 2023

epage reviewed Mar 10, 2023

View reviewed changes

src/cargo/ops/fix.rs Show resolved Hide resolved

src/cargo/util/profile.rs Show resolved Hide resolved

src/cargo/core/features.rs Show resolved Hide resolved

weihanglo commented Mar 10, 2023

View reviewed changes

src/cargo/core/compiler/custom_build.rs Show resolved Hide resolved

src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved

weihanglo added the T-cargo Team: Cargo label Mar 10, 2023

weihanglo force-pushed the clippy-disallowed-methods branch from 290c0dd to f78d081 Compare March 17, 2023 12:52

rustbot added the A-cli Area: Command-line interface, option parsing, etc. label Mar 17, 2023

weihanglo and others added 2 commits March 17, 2023 12:57

clippy: allow usages of std::env::var family by exceptions

5ccca80

See comments above for each usage about why it is allowed.

refactor(fix): Clarify env variable names are internal

1071cce

weihanglo force-pushed the clippy-disallowed-methods branch from f78d081 to 5ccca80 Compare March 17, 2023 12:58

weihanglo marked this pull request as ready for review March 17, 2023 12:58

weihanglo commented Mar 17, 2023

View reviewed changes

weihanglo changed the title ~~clippy: warn on disallowed_methods std::env::{var,var_os}~~ clippy: warn disallowed_methods for std::env::var and friends Mar 17, 2023

epage reviewed Mar 17, 2023

View reviewed changes

build.rs Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2023

bors merged commit 99ad42d into rust-lang:master Mar 17, 2023

weihanglo deleted the clippy-disallowed-methods branch March 18, 2023 06:42

0xPoe mentioned this pull request Mar 18, 2023

Make clippy happy #11867

Closed

weihanglo mentioned this pull request Mar 21, 2023

Update cargo rust-lang/rust#109463

Merged

ehuss added this to the 1.70.0 milestone Mar 26, 2023

weihanglo mentioned this pull request Apr 24, 2023

Avoid directly reading environment variables #11588

Closed

Conversation

weihanglo commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR try to resolve?

How should we test and review this PR?

Additional information

Uh oh!

rustbot commented Mar 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weihanglo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

epage commented Mar 10, 2023

Uh oh!

weihanglo commented Mar 10, 2023

Uh oh!

rfcbot commented Mar 10, 2023 • edited by Muscraft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Mar 17, 2023

Uh oh!

weihanglo Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

weihanglo commented Mar 17, 2023

Uh oh!

Uh oh!

epage commented Mar 17, 2023

Uh oh!

bors commented Mar 17, 2023

Uh oh!

bors commented Mar 17, 2023

Uh oh!

bors commented Mar 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weihanglo commented Mar 10, 2023 •

edited

Loading

weihanglo left a comment •

edited

Loading

rfcbot commented Mar 10, 2023 •

edited by Muscraft

Loading