Lint against unexpected cfgs in `[target.'cfg(...)']` #14581

Urgau · 2024-09-23T13:40:27Z

What does this PR try to resolve?

Since #13571, we now lint by default on unexpected cfgs in Rust code, but one area that is missing are cfg expressions in Cargo.toml and .cargo/config.toml. This PR address this missing piece by (unstably) introducing -Zcheck-target-cfgs based on rustc --print=check-cfg.

The feature ~~voluntarily does not always follow RUSTFLAGS (to avoid mistakes with one-off and for consistency)~~ but instead follows Rust unexpected_cfgs lint configuration:

[target.'cfg(foo)'.dependencies] # will not get linted about
cfg-if = "1.0"

[lints.rust.unexpected_cfgs]
level = "warn"
check-cfg = ['cfg(foo)'] # because it's mark as expected here

In terms of implementation, one global --print=check-cfg is retrieved and fetched, packages that have their own check-cfg inputs have each their own --print=check-cfg, otherwise the expected cfgs will get mixed up. Each --print=check-cfg is cached and only done if necessary.

How should we test and review this PR?

As asked I tried putting everything into small commits, they may not reflect the de-coupling possible as they follow the implementation but they are still IMO very useful. However I strongly recommand doing a first-pass of the complete diff first.

In terms of tests, I added some combinations that I though would be useful, I can add more if desired.

Additional information

Documentation for --print=check-cfg. It was one of motivation of the compiler MCP rust-lang/compiler-team#743.

r? @epage

crates/cargo-platform/src/cfg.rs

epage · 2024-09-25T19:34:23Z

src/cargo/core/compiler/build_context/target_info.rs

@@ -56,6 +57,80 @@ pub struct TargetInfo {
    pub rustdocflags: Rc<[String]>,
 }

+/// Check Config (aka `--print=check-cfg` representation)
+#[derive(Debug, Default, Clone)]
+pub struct CheckCfg {


Should this live in cargo-platform?

A valid answer can "maybe later" so we can keep it close for now as we figure out what this should look like

Idk, but one thing to keep in mind is that --print=check-cfg is still unstable and I don't if we should expose unstable rustc parts in cargo-platform.

While that flag is unstable, the formatting of check-cfg's is already stable and this is using the same format, right?

Negative, the input of --check-cfg and the output of --print=check-cfg are different.
I choose to make the output of --print=check-cfg very close to --print=cfg, so it would be easier to parse for consumer.

As suggested I put those in cargo-platform for now. f8179c5 (#14581)

I choose to make the output of --print=check-cfg very close to --print=cfg, so it would be easier to parse for consumer.

Is there a place that documents differences?

I worry that having check-cfg and print-cfg be different will make things more difficult, not less, as we now have two distinct ways of talking about the same kind of thing.

Yes, the documentation of --print=check-cfg, on the left is the --check-cfg input and on the right is the --print=check-cfg output.

My concern about re-using the --check-cfg syntax is that it's not easy to parse and would basically require every user to have a MetaItem parser, while with the simplified output you only need to split the first = and strip the " for the value, much easier.

I'm concerned we may add support for something that isn't expressible in this syntax, e.g. https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618#additional-globalsvalues-validation-rules-24

imo the name --print=check-cfg needs to change because that is not what its doing (but printing another format with the same intent) and giving it this name is confusing as I look through the PR. I know in the context of this PR what it means now but in viewing the code as someone without that context, that isn't clear at all.

Similarly, this is terrible UI design for rustc's command-line. The user has to not just read the docs but carefully parse a bullet list to try to understand that this is not check-cfg thats being printed.

src/cargo/core/compiler/build_context/target_info.rs

epage · 2024-09-25T19:43:23Z

src/cargo/core/compiler/build_context/target_info.rs

+        let check_cfg = {
+            let mut process = rustc.workspace_process();
+
+            apply_env_config(gctx, &mut process)?;
+            process
+                .arg("-")
+                .arg("--print=check-cfg")
+                .arg("--check-cfg=cfg()")
+                .arg("-Zunstable-options")
+                .env_remove("RUSTC_LOG");
+
+            // Removes `FD_CLOEXEC` set by `jobserver::Client` to pass jobserver
+            // as environment variables specify.
+            if let Some(client) = gctx.jobserver_from_env() {
+                process.inherit_jobserver(client);
+            }
+
+            let (output, _error) = rustc
+                .cached_output(&process, 0)
+                .with_context(|| "failed to run `rustc` to learn about check-cfg information")?;
+
+            let lines = output.lines();
+
+            lines.fold(CheckCfg::default(), |mut check_cfg, line| {
+                check_cfg.process_line(line);
+                check_cfg
+            })
+        };


The feature voluntarily does not follow RUSTFLAGS (to avoid mistakes with one-off and for consistency) but instead follows Rust unexpected_cfgs lint configuration:

Could you clarify what you mean by this?

Calling rustc is "expensive" for no-op builds which is especially important for cargo-script. Yes, this is cached, but I'd like to better understand why we need this over adding it to another call.

I want to avoid confusing users who might add --check-cfg args in RUSTFLAGS, thinking that it works while at the second they will remove them from RUSTFLAGS it will not anymore; but now that I think about it more, it's already the case for rustc (since it gets RUSTFLAGS), so this behaviour would be worse than not forwarding it as it would work with rustc but not Cargo.

I will revert this behaviour and merge it into the others --prints.

Having this respect RUSTFLAGS would likely require this to be processed during compilation. Properly processing RUSTFLAGS is a complex, expensive operation as it is fixed point as it evaluates how new --cfgs opt-in more RUSTFLAGS.

TargetInfo as some handling of RUSTFLAGS, so we now use it's handling of RUSTFLAGS. 18f2caf (#14581)

epage · 2024-09-25T19:43:45Z

src/cargo/core/compiler/build_context/target_info.rs

+            let mut process = rustc.workspace_process();
+
+            apply_env_config(gctx, &mut process)?;
+            process
+                .arg("-")
+                .arg("--print=check-cfg")
+                .arg("--check-cfg=cfg()")
+                .arg("-Zunstable-options")
+                .env_remove("RUSTC_LOG");
+
+            // Removes `FD_CLOEXEC` set by `jobserver::Client` to pass jobserver
+            // as environment variables specify.
+            if let Some(client) = gctx.jobserver_from_env() {
+                process.inherit_jobserver(client);
+            }
+
+            let (output, _error) = rustc
+                .cached_output(&process, 0)
+                .with_context(|| "failed to run `rustc` to learn about check-cfg information")?;


Note: I've not yet inspected this for alignment with our other rustc calls

tests/testsuite/cfg.rs

src/cargo/ops/cargo_compile/mod.rs

epage · 2024-09-25T19:53:24Z

src/cargo/ops/cargo_compile/mod.rs

+                gctx.shell().warn(format!(
+                    "{prefix}unexpected `cfg` condition value: `{value}` for `{cfg}` in `[target.'cfg({cfg_expr})'{suffix}]`"
+                ))?;
+            }
+            None => {
+                gctx.shell().warn(format!(
+                    "{prefix}unexpected `cfg` condition name: `{name}`{cfg} in `[target.'cfg({cfg_expr})'{suffix}]`",
+                    cfg = if value.is_some() {
+                        format!(" for `{cfg}`")
+                    } else {
+                        format!("")
+                    }
+                ))?;
+            }


As this aligns closely with our new linting system, I'm assuming @Muscraft would prefer these to be using annotate-snippets

The last revision of this PR is now using annotate-snippets.

src/cargo/core/compiler/build_context/target_info.rs

src/doc/src/reference/unstable.md

crates/cargo-platform/Cargo.toml

epage · 2024-09-25T19:57:40Z

This probably could have used a conversation with the Cargo team first to ensure alignment with where we are going with lints. If there are questions about the direction I'm pointing this towards, feel free to reach out!

src/cargo/ops/cargo_compile/mod.rs

epage · 2024-09-26T03:26:13Z

src/cargo/ops/cargo_compile/mod.rs

+    if lint_rustflags.iter().any(|a| a == "--check-cfg") {
+        Ok(Some(CheckCfg::new(gctx, rustc, Some(lint_rustflags))?))


I don't feel comfortable having us parse a command line

I guess we don't parse it but forward it to rustc but then we are conditionally including unrelated flags in the call to rustc which feels odd and potentially brittle

Yeah, it's a hack and I don't really like it either, but we just deduplicated all the logic in #14567, that I didn't wanted to reintroduce one that will potentially stay indefinitely and the unrelated flags are only allow/warn/deny which is fine for --print=check-cfg.

src/cargo/ops/cargo_compile/mod.rs

crates/cargo-platform/src/check_cfg.rs

src/cargo/util/lints.rs

bors · 2024-10-01T20:43:31Z

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

src/cargo/util/lints.rs

bors · 2024-10-08T21:49:36Z

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

bors · 2024-10-31T21:55:42Z

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

bors · 2024-11-15T20:28:05Z

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

with the help of the Cargo lints infra

rustbot assigned epage Sep 23, 2024

rustbot added A-cfg-expr Area: Platform cfg expressions A-documenting-cargo-itself Area: Cargo's documentation A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2024