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 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 now use and it's handling to have 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/core/compiler/build_context/target_info.rs

epage · 2024-09-27T20:04:49Z

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

+    /// `CheckCfg` informations extracted from `rustc --print=check-cfg`.
+    check_cfg: Option<CheckCfg>,


Please call this something else if its not check-cfg

tests/testsuite/cfg.rs

epage · 2024-09-27T20:10:10Z

src/cargo/util/lints.rs

+    // FIXME: This method is broken is several ways, it doesn't take into account
+    // `rustc` flags 'via `RUSTFLAGS`), nor the possible lints groups, ...


How are we tracking this?

epage · 2024-09-27T20:10:20Z

src/cargo/util/lints.rs

+    // FIXME: While it doesn't doesn't really matter for `--print=check-cfg`, wee should
+    // still pass the actual requested targets instead of an empty array so that the
+    // target info can be de-duplicated.


How are we tracking this?

src/cargo/util/lints.rs

epage · 2024-09-27T20:19:10Z

src/cargo/util/lints.rs

        return Ok(());
    };

+    // If we have extra `--check-cfg` args comming from the lints config, we need to
+    // refetch the `--print=check-cfg` with those extra args.
+    let lint_rustflags = pkg.manifest().lint_rustflags();
+    let check_cfg = if lint_rustflags.iter().any(|a| a == "--check-cfg") {
+        Some(target_info.check_cfg_with_extra_args(gctx, &rustc, lint_rustflags)?)


Is there a reason we are doing both a "global" and " extra_args" calls rather than just the latter?

The "global" call is always done, cached and already contains the --print=check-cfg, so getting it is mostly "free", and even if we don't use the print-check-cfg it parsed it is still useful as it gives us all the RUSTFLAGS args that we can just re-use for free to the "extra_args" case.

If we don't need to ask for it from rustc twice, we probably shouldn't. Removing the global check_cfg seems like it would simplify this overall PR as we no longer need to interject ourselves into the regular rustc query, even if we still need it for processing rustflags.

Well we don't really ask for rustc twice, we are just re-using a already available information for when there is no [lints.rust.unexpected_cfgs.check-cfg] specified, which I think is the vast majority of projects.

The same "global" call will be used for cfgs, split-debuginfo support, ...

Schematically, if we have 3 packages, we would currently do 1 rustc call for the "global".

Now imaging that one the package as extra check-cfg args from the lint config, with the current strategy we would only do 2 rustc calls, the "global" and the one specialized for the package.

If we were to not "interject ourselves into the regular rustc query" (aka "global") there would 4 rustc calls (the global and 1 per package) instead of 2.

Btw I'm getting mixed signal with this comment and the one this conversation:

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.

Which is why I changed strategy to reduce the number of calls.

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.

In preperation for new APIs around check-cfg

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