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: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

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

Urgau · 2025-05-25T12:57:20Z

@epage @Muscraft I have updated the PR with the conclusion of our discussion at the All Hands. That is, the lint is now under Cargo cargo::unexpected_cfgs.

I don't remember if we had settle on the lint config position, ie if we should share check-cfg = [...] from rust.unexpected_cfgs or if cargo.unexpected_cfgs should have a distinct one. I've left the sharing for now.

with the help of the Cargo lints infra

epage · 2025-05-27T17:40:21Z

src/doc/src/reference/unstable.md

+[lints.rust.unexpected_cfgs]
+level = "warn"
+check-cfg = ['cfg(foo)']


I don't remember if we had settle on the lint config position, ie if we should share check-cfg = [...] from rust.unexpected_cfgs or if cargo.unexpected_cfgs should have a distinct one. I've left the sharing for now.

I would say if we have our own lint, we have our own lint config.

If anything, in light of lints being fractured like this, I would see us re-evaluating whether we should have per-lint config or a general config table.

epage · 2025-05-27T17:41:42Z

tests/testsuite/cfg.rs

@@ -871,3 +872,255 @@ fn unexpected_cfgs_target() {
 "#]])
        .run();
 }
+
+#[cargo_test(nightly, reason = "--print=check-cfg is unstable in rustc")]


Could we consolidate all of the tests into one commit?

epage · 2025-05-27T17:42:06Z

tests/testsuite/cfg.rs

Could you add a feature gate test?

epage · 2025-05-27T17:42:42Z

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

Replace rustc --print=cfg parsing with handrolled loop

There a reason this isn't just squashed into the commit that used the iterators?

epage · 2025-05-27T17:43:24Z

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

+                process.arg("--print=crate-name"); // `___` as a delimiter.
+                process.arg("--print=check-cfg");
+
+                process.arg("--check-cfg=cfg()"); // otherwise `--print=check-cfg` won't output


Why is that?

The comment is a bit misleading, --print=check-cfg alone would print something, just not what we want, without --check-cfg=cfg() it would be like we didn't enable check-cfg checking and so --print=check-cfg would only return the default state (of no checking).

We also have --check-cfg=cfg() when generating the check_cfg_args.

The intent of --check-cfg=cfg() in here or in that comment is unclear.

epage · 2025-05-27T17:45:40Z

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

+                let mut check_cfg = CheckCfg::default();
+                check_cfg.exhaustive = true;


API-wise, this feels dirty that we are having to tell it that the default state before parsing is exhaustive = true

We only know if it's exhaustive by the absence of any()=any() and any(), which we can't know inside parse_print_check_cfg_line since it doesn't know if we have finished processing all the lines.

What about having associated function CheckCfg::parse_print_check_cfg_lines (note the s at the end), which would return a CheckCfg (no Default impl anymore), this function would do the business of parsing all the lines and setting the exhaustive flag appropriately.

What about having associated function CheckCfg::parse_print_check_cfg_lines

That could work.

Would it also make sense for the constructor to set exhaustive to true?

epage · 2025-05-27T17:46:59Z

src/cargo/util/lints.rs

+    // FIXME: While it doesn't doesn't really matter for `--print=check-cfg`, we should
+    // still pass the actual requested targets instead of an empty array so that the
+    // target info can always be de-duplicated.
+    // FIXME: Move the target info creation before any linting is done and re-use it for
+    // all the packages.


How are we expected to track these FIXMEs?

Idk, what about creating GitHub issues about those.

One thing to note is that if we follow #14581 (comment), and have the --print=check-cfg in the same check/build unit, those fixme disappear.

src/cargo/util/lints.rs

epage · 2025-05-27T17:50:56Z

src/cargo/util/lints.rs

+    groups: &[],
+    default_level: LintLevel::Warn,
+    edition_lint_opts: None,
+    feature_gate: None,


Right now only the acquisition of check-cfgs from the compiler is feature gated. The lint should be feature gated as well.

epage · 2025-05-27T17:52:17Z

src/cargo/util/lints.rs

+    ),
+};
+
+pub fn unexpected_target_cfgs(


@Muscraft could you also review this new lint since we are so new at these?

epage · 2025-05-27T17:55:59Z

src/cargo/util/lints.rs

+                    // Retrieving the span can fail if our pretty-priting doesn't match what the
+                    // user wrote: `unix="foo"` and `unix = "foo"`, for that reason we don't fail
+                    // and just produce a slightly worth diagnostic.
+                    if let Some(span) = get_span(manifest.document(), &["target", &*format!("cfg({cfg_expr})")], false) {


Hmm, I wonder how often this will happen and if we need to do something else.

I wish we had a style guide for Cargo fleshed out so we could just assume someone is using whatever style is emitted by cargo fmt.

epage · 2025-05-27T17:59:02Z

src/cargo/util/lints.rs

+    // 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") {


I don't feel too comfortable having this stringly-typed API from our Cargo.toml parser to here

Me either. Looking at the flags like this is ugly, I would love to not have to do this. A better way would probably be to pass always pass --print=check-cfg (with a file path, with maybe with the JSON output at some point) in the check/build invocation of the crate, that way we only have one compilation unit. It would be a much larger change though.

Alternatively, we could refactor things so the generation of lint_rustflags is delayed and so we have access to more specific check-cfg configuration.

epage · 2025-05-27T18:00:28Z

src/cargo/util/lints.rs

+    // 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)?)


Why are we calling rustc for this rather than processing them ourselves?

--check-cfg args are opaque for Cargo and I would like for it to stay that way, otherwise will have to duplicate the parsing logic in Cargo, keep them consistent, it's already non trivial in rustc. Processing them in Cargo would also means that any evolution would be bound on both Cargo and rustc implementing them and stabilizing them at the same time. I really don't want to go that route.

Except instead we have a parser for something not quite like check-cfg and have to process that.

--check-cfg and --print=check-cfg are very different, --check-cfg is an input that accepts multiple ways for doing the same thing, it can evolve, and new way of doing things, for example, we may one day add the shorthand target(...); on the other-hand the output of --print=check-cfg is meant to be stable, no matter the inputs given in --check-cfg.

With --print=check-cfg each line a expected key-value (except for any()=any() or any() but those are exceptions), it's much simpler to parse and it doesn't vary if the input changes, like if introduced the shorthand target(...) to --check-cfg, the output would stay with the full form, no matter the input given.

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