Tracking Issue for RFC 3013: Checking conditional compilation at compile time

[ehuss]

trafficstars

This is a tracking issue for the RFC 3013: Checking conditional compilation at compile time (rust-lang/rfcs#3013).

Issues: https://github.com/rust-lang/rust/labels/F-check-cfg Documentation: https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/check-cfg.html

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• [x] Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• [X] Adjust documentation (check_cfg - Unstable book)
• [ ] Stabilization PR (FCP: https://github.com/rust-lang/rust/issues/82450#issuecomment-1965328542)

Unresolved Questions

• Which cfgs should be implicitly included? See this list in bootstrap for an example of some values that need "adding in" atop compiler defaults. (Try to go to master in the link above; the list may have been updated since then).

Implementation history

• #93915
• rust-lang/cargo#10408
• rust-lang/cargo#10428
• rust-lang/cargo#10486
• and many more

[Urgau]

The implementation of this RFC has been merged in rustc as unstable and is already in nightly for experimentation. :tada:

The next the steps for the rustc side will be to improve it with suggestions, auto-fix in obvious cases and hopefully add well known values (non-builtin target is a problem), the rfc doesn't mention it directly but this seems pretty obvious. Some of this stuff is already tackle by https://github.com/rust-lang/rust/pull/94175.

We almost certainly want rustdoc to also have it (because rustc doesn't know about doc tests and rustdoc also have --cfg so it seems good to be consistent), so I've opened https://github.com/rust-lang/rust/pull/94154 to tackle it.

And last but not least the cargo integration, the RFC intentionally left the control part out of the RFC, but I have some ideas about how it could be done. But as RFC said it seems uncontroversial for Cargo to enable checking for feature = "..." values, so I'm currently tackling this part. ~No PR for this yet, but I'm working on it.~

EDIT: Cargo PR https://github.com/rust-lang/cargo/pull/10408, merged ~but not yet in nightly~ see https://github.com/rust-lang/rust/issues/82450#issuecomment-1052085034

[jhpratt]

Wow, that's awesome! I didn't even know this was being worked on actively.

As someone who would like love to use this in CI, how feasible is it to try this on nightly, even if "just" for features? I ask because it sounds like cargo integration isn't a thing yet.

[Urgau]

Wow, that's awesome! I didn't even know this was being worked on actively.

:heart:

As someone who would like love to use this in CI, how feasible is it to try this on nightly, even if "just" for features? I ask because it sounds like cargo integration isn't a thing yet.

It would just be a matter of passing something like this -Z unstable-options --check-cfg='values(feature, "f_a", "f_b", "f_n")' where f_a, f_b, ... f_n are your features name to rustc with cargo you can use the RUSTFLAGS env variable do to it. This manual process is only temporary until cargo support is added.

RUSTFLAGS='-Z unstable-options --check-cfg=values(feature,"f_a","f_b","f_n")' cargo build

[jhpratt]

That's not bad at all! I'll definitely add in a CI check.

[Urgau]

Good news! rustdoc (https://github.com/rust-lang/rust/pull/94154) and cargo (only features, https://github.com/rust-lang/cargo/pull/10408) support have been merged. :fireworks: As well a some improvements to the lint (https://github.com/rust-lang/rust/pull/94175).

You can now simply use cargo build -Z check-cfg-fetaures to check values of every config feature. Works with: check, build and test.

$ cargo +nightly check -Z check-cfg-features

    Checking hoho v0.1.0 (/tmp/hoho)
warning: unexpected `cfg` condition value
 --> src/main.rs:1:7
  |
1 | #[cfg(feature = "tokiO")]
  |       ^^^^^^^^^^-------
  |                 |
  |                 help: did you mean (notice the capitalization): `"tokio"`
  |
  = note: `#[warn(unexpected_cfgs)]` on by default
  = note: expected values for `feature` are: default, serde, tokio

warning: `hoho` (bin "hoho") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.54s

[c410-f3r]

Thank you for all the hard work, @Urgau

[petrochenkov]

Enabling --check-cfg=values() currently incurs some compilation time hit (especially on small programs) because it will load target specifications for all targets (100-200 of them at the moment), while without --check-cfg=values() targets are only loaded on demand (so only 1-2 target specs are typically loaded).

In theory, majority of targets can be loaded at compiler's compile time instead (and put into a static). In that case only a few Apple targets will be loaded dynamically because they need to read environment variables from the host. The main blocker for this is supporting String::from("foo") at compile time (i.e. const allocations).

[petrochenkov]

Enabling --check-cfg=values() currently incurs some compilation time hit (especially on small programs)

Someone also needs to prepare a perf run to measure the exact slowdown.

[lcnr]

The main blocker for this is supporting String::from("foo") at compile time (i.e. const allocations).

Alternatively one could use some Cow or whatever for targets instead of String.

[Urgau]

Small update.

Work was done to reduce the cost associated with well known values checking in PR https://github.com/rust-lang/rust/pull/95202. It resulted in noticeable win, coming from +1.5% to +1%. More work could be done in the future with improvements in const functions.

The cargo integration has also evolved quite a bit with the introduction of a single flag: -Zcheck-cfg that take one or multiple values (https://github.com/rust-lang/cargo/pull/10566):

cargo check -Z check-cfg=features,names,values

Another big addition on the cargo side is the introduction of the build output rustc-check-cfg in build script (https://github.com/rust-lang/cargo/pull/10539):

// build.rs
println!("cargo:rustc-check-cfg=names(foo, bar)");

cargo check -Z check-cfg=output

More information can be found on cargo reference: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg

[Kampfkarren]

Would've loved to have this the other day. I wrote out #[cfg(feature = "debug_assertions")] in a ton of places instead of #[cfg(debug_assertions)].

[Wyvern]

Hi guys, following this thread put -Zcheck-cfg=features,names,values,output as command args works, but is there a way to put it in config.toml [unstable] section for now? Tried several times, still got parse errors...

[Urgau]

Hi guys, following this thread put -Zcheck-cfg=features,names,values,output as command args works, but is there a way to put it in config.toml [unstable] section for now? Tried several times, still got parse errors...

@Wyvern Sorry for the inconvenience, there is currently no way to set it because of a mistake from my part when changing a not so internal struct. I've posted a fix the cargo side https://github.com/rust-lang/cargo/pull/10799 (it should take at least a few days before being available in nightly).

[Urgau]

As part of my effort to improve check-cfg I opened several months ago MCP636 - Simplify and improve explicitness of the check-cfg syntax, which was accepted. The following represent the user-facing changes.

rustc changes

Add new simpler and more explicit syntax for check-cfg - #111072

New cfg form

It introduces the new cfg form and deprecate the previous two (names and values):

rustc --check-cfg 'cfg(name1, ..., nameN, values("value1", "value2", ... "valueN"))'

Default built-in names and values

It also changes the default for the built-in names and values checking.

• Built-in values checking would always be activated as long as a --check-cfg argument is present
• Built-in names checking would always be activated as long as a --check-cfg argument is present unless if any cfg(any()) arg is passed

cargo changes

Adjust -Zcheck-cfg for new rustc syntax and behavior - #12845

Unified options

All the options have been removed and -Zcheck-cfg no longer accepts any options (in the CLI or config.toml)

cargo build -Zcheck-cfg

This means that passing -Zcheck-cfg now always enables features, well known names and well known values checking.

Passing rustc-check-cfg: to cargo now requires the use of the new syntax.

---

If you have any remarks/issues feel free to open an issue about it or contact me on rust-lang Zulip instance.

[jplatte]

There is no way to mark existing well-known attributes as unknown, is there? Specifically, I think it would be nice if there was a way for rustc to raise a warning when it encounters cfg(test) (which cargo could then make use of for crates that use lib.test = false).

[epage]

We've been discussing --check-cfg checking --cfg (rust-lang/rust#100574) on zulip

So to step back, let's put this in scope of the use cases where --check-cfg helps:

• Case 1: User defines features and puts #[cfg]s in their code
  • --check-cfg catches typos
• Case 2: User puts platform #[cfg]s in their code
  • --check-cfg catches typos
• Case 3: A user of tokio wants to set RUSTFLAGS=--cfg tokio_unstable
• Case 4: User has a custom cfg in their project and they want to set RUSTFLAGS=--cfg mycfg
• Case 5. User has build.rs generate a cfg that they then#[cfg] on in their code
  • build.rs can include a directive on what cfgs it might generate
  • --check-cfg catches typos
• Case 6: A user building a meson package sets the extra_rustflags (defined by the packager) and wants to configure it through --cfg
  • See vlc allowing something like this
  • --check-cfg catches typos

Usually a mixture of these will exist within a workspace and its non-local dependencies

For me, this seems like it offers a lot of benefits and would love to see this stabilized as warning by default for cargo projects.

For Case 5, a user might forget the check-cfg directive and things just work because their #[cfg] is behind another #[cfg]. When the root #[cfg] is active, they can get an unexpected cfg lint for the valid #[cfg].

This also helps with Case 6 by reporting warnings when the user mistypes a --cfg

Case 3 and 4 are the problematic ones. For the packages that consume the cfg, they can have a build.rs directive declare it but that won't help the compilation of build.rs and rustc will emit a warning about an unexpected cfg on the command-line. Rustc will also emit a warning for every other package's build-targets (and their build script) in the build graph. Cargo passes --cap-lints. This means that the above false positives will only show up per workspace member and not per dependency. It also means means that --cfg being checked by --check-cfg won't catch typos in tokio in Case 3. Even if it does catch it in Case 4, it would be lost in the noise.

If we stabilized as-is, workarounds by users include

• User lives with a warning per package when passing in RUSTFLAGS=--cfg something
• ~~The user adds the build.rs directive to every workspace member marking it as accepted~~
  • The warning will still pop up for build scripts
• User passes RUSTFLAGS=--cfg something -Aunexpected_cfgs
  • User loses the other benefits of this feature when they use the --cfg
• User puts lints.rust.unexpected_cfgs = "allow" in every workspace member (helped via workspace inheritance)
  • User loses the other benefits of this feature
• User passes in RUSTFLAGS=--cfg something --check-cfg ...

RUSTFLAGS=--cfg is a more advanced use case (which is why we don't support well known cfgs to be defined in Cargo.toml). The cargo team would love to find higher levels of abstraction to replace RUSTFLAGS (see rust-lang/cargo#12739) and globals is one possible replacement for users interacting with --cfg as much.

That said, RUSTFLAGS and --cfg is where we are at now and we have no idea if or when abstractions will come along. And while its generally an advanced case, a tepid user dipping their toes into this thing recommended in a document somewhere shouldn't get a hostile experience that makes them lose what confidence they had in working with Rust.

With all of that said, cargo is not the only user of rustc and we need to consider what the correct mechanism / "API" rustc should provide to build systems (or people directly invoking it).

So where do we go from here?

Some options

• Keep the status quo, deciding too few users on too advanced of use cases can deal with it
• Don't lint --cfg, deferring to solving this problem later so the rest of the improvement doesn't get blocked on the rest of this discussion (ie revert #100574)
• Have cargo not set --check-cfg if RUSTFLAGS is used but that heuristic won't be obvious to users and can have very negative affects like turning on -Dwarnings will allow instead of deny unexpected_cfgs
• Decide we shouldn't lint --cfg (ie revert #100574)
• Split the lint in two (merge #117522)

[ehuss]

For the packages that consume the cfg, they can have a build.rs directive declare it and no warning will happen.

Passing cargo:rustc-check-cfg=cfg(…) in a build script doesn't silence the warning because the --cfg flag is passed when building the build script itself, and thus will issue a warning. That's partially why #11631 was proposing to define the valid cfg's in Cargo.toml.

Having users pass additional flags in RUSTFLAGS (like -A or --check-cfg) doesn't sound too awful, but it is definitely clunky, and will introduce a noisy experience for some users. I wish we had better options for the use cases where users are doing that. If we move forward with this as-is, I think we should at least do some kind of education for users to guide them on how to fix it.

[epage]

Passing cargo:rustc-check-cfg=cfg(…) in a build script doesn't silence the warning because the --cfg flag is passed when building the build script itself, and thus will issue a warning. That's partially why https://github.com/rust-lang/rust/issues/11631 was proposing to define the valid cfg's in Cargo.toml.

Ugh, overlooked that. I assume these are unique enough that there is no de-duplicating (if we have any) and so the user will see a warning per package and build script.

Having users pass additional flags in RUSTFLAGS (like -A or --check-cfg) doesn't sound too awful, but it is definitely clunky, and will introduce a noisy experience for some users. I wish we had better options for the use cases where users are doing that. If we move forward with this as-is, I think we should at least do some kind of education for users to guide them on how to fix it.

For me, the biggest issue is that this feature is meant to help people discover when they type something in and the very first time they try to use --cfg, they will be inundated with messages saying they did it wrong. Education has a very limited reach in helping people ignore this rather than assume they did something wrong and then they go through a long investigation to figure out why.

Once they know it (or were already "in the know"), then that is a functional, albeit clunky, workaround.

[ehuss]

I assume these are unique enough that there is no de-duplicating (if we have any) and so the user will see a warning per package and build script.

It will get de-duped, but there are summaries for each package that look like warning: `foo` (build script) generated 1 warning (1 duplicate).

Also, in the scenario where you don't pass --check-cfg anywhere, then you'll get a warning for every target in a package (build-script, lib, bins, tests, examples, etc.). So the "duplicate" warning message could potentially show up a large number of times. But on the positive side, it is only one line (for each target).

[jplatte]

There's also the case of people using #![cfg_attr(docsrs, feature(doc_auto_cfg)] and such. Luckily, this can be solved with a build script like I did here since that cfg is only ever actually passed via RUSTDOCFLAGS (and we don't use --check-cfg with it, so only need the build script for workspace members that have a docsrs attribute).

[epage]

we don't use --check-cfg with it, so only need the build script for workspace members that have a docsrs attribute

My hope / expectation is that we'll make --Zcheck-cfg the exclusive behavior of cargo when stabilized and I assume that would apply to cargo doc as well.

However, warnings during docs.rs's call to cargo doc so checking --cfg will likely not be a problem for this use case.

[Nemo157]

However, warnings during docs.rs's call to cargo doc so checking --cfg will likely not be a problem for this use case.

I do hope to one day surface warnings from docs.rs' builds to crate maintainers.

[Urgau]

Status update, as of 2024-01-18.

Two new PRs have been merged, #119473 and #119930. They come from a feedback from Cargo (thanks @epage), where we realised that empty values() were confusing and not intuitive.

• #119473 adds the none() value variant to be able to explicitly add the none variant (as in #[cfg(foo)] where the value is none). Previously, the only way to add a none value variant was to use an empty values(). Users should now use the much clearer values(none()) (which can be combined with strings).

• #119930 changed the meaning of empty values() to no longer mean values(none()) but to actually imply an empty set of expected values. This removes the un-intuitiveness and brings a way to have the inverse of values(any()) where instead of accepting any values, with empty values() (assuming no other cfg for the same config name) we will not be accepting any values. See the PR description for more context.

These two changes have allowed Cargo to go back to always using values(...) (even if there are zero declared features), which makes the Cargo --check-cfg invocation more in line with reality and allows for better diagnostics. See https://github.com/rust-lang/cargo/pull/13316 for further details.

[Urgau]

Status update, as of 2024-02-10:

1. A Crater run has been done in #120701, the full analysis report can be found here. The here is (very) condensed summary:

   After manually checking 3 categories (most unexpected features, target_*, and general) I wasn't able to find any false positive[^false_positives]; that means that 9649 actions would need to be taken across 5959 projects (1.4% of all the projects tested), by either: fix the typo, removing the staled condition, marking as expected the custom cfg, adding the feature to the features table...

   [^false_positives]: by "false positive" I mean: missing well known names/values

2. A Call for Testing has been issued in the RFC PR, it should appear in next week (2024-10-15) This Week in Rust.

[ojeda]

Thanks for this feature!

I gave it a try in the kernel: it was easy to set up and seemed to work as expected, with nice errors for small typos, e.g.:

error: unexpected `cfg` condition name: `CONFIG_KVNIT`
  --> rust/kernel/lib.rs:38:7
   |
38 | #[cfg(CONFIG_KVNIT)]
   |       ^^^^^^^^^^^^ help: there is a config with a similar name: `CONFIG_KUNIT`

Here are a few notes I noticed in a first pass:

• The kernel has ~20k (yes, kilo!) configuration options. For a quick test on this feature, I transformed the ~3k --cfg flags we pass in a small config (i.e. the enabled features) into --check-cfgs, but for proper checking we will need to pass all the 20k symbols. So this is another data point on having many config options, like the one someone else posted.

• The error message for an unknown condition name dumps all the known names in some cases, which is a bit too much (pages of the terminal) when you have thousands of those... :)

     = help: expected names are: `CONFIG_60XX_WDT`, `CONFIG_64BIT`, ... thousands of these ..., `CONFIG_ZSTD_DECOMPRESS`, `CONFIG_ZSWAP`, `debug_assertions`, `doc`, `doctest`, `miri`, `overflow_checks`, `panic`, `proc_macro`, `relocation_model`, `sanitize`, `sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `target_env`, `target_family`, `target_feature`, `target_has_atomic`, `target_has_atomic_equal_alignment`, `target_has_atomic_load_store`, `target_os`, `target_pointer_width`, `target_thread_local`, `target_vendor`, `test`, `testlib`, `unix`, `windows`
  

  At least it does not seem to repeat the list when the same one happens, but still, it is too much even one time.

• This help note is not really appropriate for calls to rustc that do not come from Cargo:

     = help: consider using a Cargo feature instead or adding `println!("cargo:rustc-check-cfg=cfg(CONFIG_KUNI)");` to the top of a `build.rs`
  

• Checking seems to be enabled just by passing -Zunstable-options -- is that expected? i.e. without passing any --check-cfg flag. The docs say:

  Well known names and values checking is always enabled as long as at least one --check-cfg argument is present.

[epage]

@ojeda any performance concerns with checking 20k cfg's? We did some basic testing on the windows package because that has "a lot" of features but 20k is in a category of its own.

Can those 20k cfg's be scoped down to relevant subsystems? I can understand the kernel as a whole having a lot but I would assume most of those are meant for specific subsystems and scoping could improve the messages and improve the checking for developers.

[Urgau]

@ojeda Thanks you for testing the future.

• The kernel has ~20k (yes, kilo!) configuration options. For a quick test on this feature, I transformed the ~3k --cfg flags we pass in a small config (i.e. the enabled features) into --check-cfgs, but for proper checking we will need to pass all the 20k symbols. So this is another data point on having many config options, like the one someone else posted.

:-) that's a lot. Did you use one --check-cfg argument per config or did you use the shorthand[^1] (one --check-cfg for all configs) ?

The shorthand version might at your scale make things a bit faster to decode.

Also did you need to use a @path argument?

• The error message for an unknown condition name dumps all the known names in some cases, which is a bit too much (pages of the terminal) when you have thousands of those... :) ```

Yeah, that's not fine. I will send a fix for it, probably similar to our very long types where we write the full type to disk.

• This help note is not really appropriate for calls to rustc that do not come from Cargo:

     = help: consider using a Cargo feature instead or adding `println!("cargo:rustc-check-cfg=cfg(CONFIG_KUNI)");` to the top of a `build.rs`
  

I thought of you (non Cargo users) when writing this diagnostic and like any other Cargo-specific diagnostic, it is conditional on the CARGO env being set. Could you check that it was or wasn't set?

• Checking seems to be enabled just by passing -Zunstable-options -- is that expected? i.e. without passing any --check-cfg flag.

I'm not able to reproduce this.

$ echo "#[cfg(uniz)] fn test() {}" > a.rs
$ rustc +nightly --crate-type=lib -Zunstable-options a.rs # does NOT lint
$ rustc +nightly --crate-type=lib -Zunstable-options --check-cfg='cfg()' a.rs # does lint

[^1]: https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/check-cfg.html#the-cfg-form "To avoid repeating the same set of values, use this form: rustc --check-cfg 'cfg(name1, ..., nameN, values("value1", "value2", ... "valueN")'"

[ojeda]

@ojeda any performance concerns with checking 20k cfg's? We did some basic testing on the windows package because that has "a lot" of features but 20k is in a category of its own.

I only tested the case with ~4k --check-cfgs (one for each option), but to give a rough idea, in a noisy VM, I get 585ms vs. 537ms for our kernel crate, i.e. about ~48ms, ~9%.

I gave a quick try to the grouping suggestion and I got 546ms, i.e. about ~9ms, ~2%.

So this could be significant (seconds) when we start having many Rust kernel modules for big configs, but in the worst case I guess we could skip it for normal builds and only check it in particular builds.

Can those 20k cfg's be scoped down to relevant subsystems? I can understand the kernel as a whole having a lot but I would assume most of those are meant for specific subsystems and scoping could improve the messages and improve the checking for developers.

It would require manual work maintaining the lists (and kernel developers are used to having all the configuration available), so maintaining custom lists would need to be useful for something else to be worth the tradeoff.

Did you use one --check-cfg argument per config or did you use the shorthand1 (one --check-cfg for all configs) ?

In the quick test I did, one per config, with many looking like this:

--check-cfg=cfg(CONFIG_SATA_SIS, values(none(), "y", "n", "m"))

But we could definitely do a single --check-cfg for e.g. all boolean options, then one for all the tristates, etc. I tried that -- please see above.

Also did you need to use a @path argument?

Yeah (we are already using @path for all the --cfgs we have).

Yeah, that's not fine. I will send a fix for it, probably similar to our very long types where we write the full type to disk.

Thanks! However, I wouldn't write the list to disk -- I don't think it is useful to see the list, and we would need to ignore those files in .gitignore and so on.

I thought of you (non Cargo users) when writing this diagnostic and like any other Cargo-specific diagnostic, it is conditional on the CARGO env being set. Could you check that it was or wasn't set?

Ah, I see; yeah, we use the variable to point to the Cargo binary (like we do for other compilers/tools).

Should that behavior be documented in src/doc/rustc, similar to Cargo that documents its environment variables? If it is not documented already, perhaps it could be changed to another variable. Otherwise, I guess we could unset it for rustc calls, but it is a bit unfortunate.

I'm not able to reproduce this.

My mistake -- it happened with core, for which I thought I was not passing the @path, but I do, so please ignore that.

[Urgau]

Thanks! However, I wouldn't write the list to disk -- I don't think it is useful to see the list, and we would need to ignore those files in .gitignore and so on.

@ojeda I'm a bit surprised by this; while I understand that the kernel may not need this because of documentation and other things, I'm not sure this can be generalized. Other build systems may not expose the full list easily, debugging is also an area where it is very useful to see the full list.

Do Rust-for-Linux pass -Zwrite-long-types-to-disk=no to rustc ? What would you think about a similar flag but for printing the full list ?

Ah, I see; yeah, we use the variable to point to the Cargo binary (like we do for other compilers/tools).

Should that behavior be documented in src/doc/rustc, similar to Cargo that documents its environment variables? If it is not documented already, perhaps it could be changed to another variable. Otherwise, I guess we could unset it for rustc calls, but it is a bit unfortunate.

I've looked at this, and I see at least 5 different usage of "does the CARGO env exist" to imply that rustc was called from Cargo. There are also usage of CARGO_CRATE_NAME and CARGO_PKG_NAME for roughly the same thing.

I don't think it is documented anywhere, since I think no one thought of users setting CARGO but not using it. We may want to switch to another Cargo env; and just to be sure, you're not setting any other Cargo specific env?

[epage]

I'm a bit surprised by this; while I understand that the kernel may not need this because of documentation and other things, I'm not sure this can be generalized. Other build systems may not expose the full list easily, debugging is also an area where it is very useful to see the full list.

Maybe its just me but I feel like if the list of suggestions is too long, its just not worth doing anything with. I don't see it being likely someone is going to open a file with 29k options to find the exact one they wanted.