weihanglo commented 2 years ago

Summary

RFC: #3013 rustc tracking issue: rust-lang/rust#82450

Documentation:

This tracks the integration of rustc --check-cfg flag (RFC #3013), which enables complete or partial checking of conditional compilation configuration at compile time.

Unresolved Issues

[ ] Should these flags be enabled by default? What's the unintrusive interface to provide for users to opt-out the checking?
[x] If we feel the diagnostic on using a feature when no features are defined is insufficient, then do we feel it would not be incongruous with the design to fix this in the future? (see #13011).

Post-RFC decisions

Support cargo:rustc-check-cfg despite :: being introduced in 1.77 and this being released after that. This was done to allow for wider MSRVs while using this feature

Future Extensions

No response

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.

Implementation history:

est31 commented 2 years ago

Over in https://github.com/rust-lang/rust/pull/93628 I've discovered two things:

First, a papercut: apparently there are empty values() calls when the feature is enabled, even if there is another values(...) call with content present already. In that case, values() is not needed, right?
Second, due to #4423 specifying custom --check-cfg inside RUSTFLAGS does not reach all of cargo's units, as in, there is no way to specify custom flags for the host units. Apparently it's still an open question how one should enable people to specify custom possible cfg names, but the way bootstrap does it via RUSTFLAGS is IMO not enough to get --check-cfg stabilized. Even if a user-friendly way via toml editing exists, ideally there would also be a way that can be set programmatically via some kind of environment variable. edit: resolved, see https://github.com/rust-lang/cargo/issues/10554#issuecomment-1156823406

Urgau commented 2 years ago

* First, a papercut: apparently there are empty `values()` calls when the feature is enabled, even if there is another `values(...)` call with content present already. In that case, `values()` is not needed, right?

You're mixing things, bootstrap (the tool that build rustc, rustdoc, ...) use cargo -Zcheck-cfg=names,values,features to enable:

well known names: --check-cfg=names()
well known values: --check-cfg=values()
well known features: --check-cfg=values(feature, ...)

And then use RUSTFLAGS to set extra check cfg (most of them will go any when cargo beta has support for rustc-check-cfg in build script). Cargo is not involved at all in this process so if there are duplicates, nothing can be done. This explain what you see.

Concerning empty values() the unstable book as a section about it check-cfg values: If values() is specified, then rustc will enable the checking of well-known values defined by itself. Note that it's necessary to specify the values() form to enable the checking of well known values, specifying the other forms doesn't implicitly enable it.

Btw, this should have been posted on the rust-lang/rust Tracking Issue for RFC 3013: Checking conditional compilation at compile time.

* Second, due to [There is no way to set RUSTFLAGS for build scripts when cross-compiling #4423](https://github.com/rust-lang/cargo/issues/4423) specifying custom `--check-cfg` inside `RUSTFLAGS` does not reach all of cargo's units, as in, there is no way to specify custom flags for the host units. Apparently it's [still an open question](https://github.com/rust-lang/rust/issues/82450#issuecomment-1046819607) how one should enable people to specify custom possible cfg names, but the way bootstrap does it via `RUSTFLAGS` is IMO not enough to get `--check-cfg` stabilized. Even if a user-friendly way via toml editing exists, ideally there would also be a way that can be set programmatically via some kind of environment variable.

Why would this be a concern for stabilization ? I don't think it's that big of a deal, bootstrap and the rust-lang/rust codebase is very special, the vast majority of project who would use check-cfg (which is less probably than 1%) would simply add rustc-check-cfg to their build script or in their Cargo.toml. I don't think anyone would use RUSTFLAGS for this.

Nevertheless I agree that I would be great if there was a way to forward RUSTFLAGS to proc-macro when --target is specified.

est31 commented 2 years ago

Concerning empty values() the unstable book as a section about it check-cfg values:

Oh I see, that's a weird choice that IMO violates the least surprise principle. I've thought that this was some genuine bug of cargo's integration of --check-cfg. I guess how that feature can be improved is indeed discussed in the rustc tracking issue. But it's a papercut, shrug.

the vast majority of project who would use check-cfg (which is less probably than 1%) would simply add rustc-check-cfg to their build script or in their Cargo.toml. I don't think anyone would use RUSTFLAGS for this.

My original reasoning was that there is a use case of wanting to enable some feature that needs different code across a code base. You can sort of do that with per-cargo-package features but they are not convenient to use when you have deep dependency hierarchies. So some projects like the rust compiler opt to use RUSTFLAGS instead. I don't think it's something exclusive to the compiler, others might do it similarly. I think the blocker is for some mechanism for multi crate projects to share the list of available cfgs.

Edit: with your general argument over in https://github.com/rust-lang/rust/pull/93628#issuecomment-1156246548 that cfgs specified in RUSTFLAGS don't reach crates either, I think you are right that it's not useful. The rust compiler's bootstrap was running into the issue because it went out of its way to specify the bootstrap cfg for proc macros as well. So I think my second concern is resolved now.

illicitonion commented 1 year ago

I recently released an update to a reasonably popular crate (thousands of reverse dependencies, 10M downloads) which had a significantly behaviour regression, and caused noticeable ecosystem breakage (including a spurious report of nightly being broken).

We had tests for the regression, but they were never getting run because they were hidden behind a feature, and there was a typo in the cfg(feature) attribute, meaning we weren't actually running those tests in CI.

I just tested out this feature for our example breakage, and it would have prevented us from releasing that broken code. I'd love to see it stabilise! The only modification I'd suggest is defaulting to deny rather than warn.

Urgau commented 11 months ago

Adjust -Zcheck-cfg for new rustc syntax and behavior - #12845 was merged and brings some major changes to the feature from Cargo side.

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

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

New defaults

With the removal of all options passing -Zcheck-cfg now always enables features, well known names and well known values checking. This matches rustc defaults.

SteveLauC commented 8 months ago

Do we miss the docsrs condition, which is used by the docs.rs site?


$ cat src/main.rs
$ /usr/bin/cat src/main.rs
#![cfg_attr(docsrs, feature(doc_cfg))]

fn main() {}

$ cargo +nightly check -Z unstable-options -Z check-cfg
warning: unexpected `cfg` condition name: `docsrs`
 --> src/main.rs:1:13
  |
1 | #![cfg_attr(docsrs, feature(doc_cfg))]
  |             ^^^^^^
  |
  = help: expected names are: `debug_assertions`, `doc`, `doctest`, `feature`, `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`, `unix`, `windows`
  = help: consider using a Cargo feature instead or adding `println!("cargo:rustc-check-cfg=cfg(docsrs)");` to the top of a `build.rs`
  = note: see <https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg> for more information about checking conditional configuration
  = note: `#[warn(unexpected_cfgs)]` on by default

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

Urgau commented 8 months ago

Do we miss the docsrs condition, which is used by the docs.rs site?

As far as I can tell, the docsrs config is a custom configuration and is not emitted by docs.rs, as can be seen in count-less Cargo.toml: rustdoc-args = ["--cfg", "docsrs"] [1] [2].

It is therefore not eligible to be a well known configuration and has to be (like any other custom config) be included "manually" either by:

adding println!("cargo:rustc-check-cfg=cfg(docsrs)"); to the top of a build.rs
using a Cargo feature

SteveLauC commented 8 months ago

As far as I can tell, the docsrs config is a custom configuration and is not emitted by docs.rs

Thanks for showing me this! TIL! Then we should whitelist it with println!("cargo:rustc-check-cfg=cfg(docsrs)");, it is so common that I thought it is something built-in:D

epage commented 8 months ago

I feel like docsrs is a no-win situation

build.rs files come with a build-time / audit cost
However, there is no way to put an allow on a #[cfg]
Some of the docsrs usages have to appear at the top-level of a package (e.g. nightly features) and so you can't just allow in a more limited scope
Requiring libraries to do a blanket allow would just negate this feature
Switching to features is not a complete replacement. I try to use features where possible to make it easy to view my documentation locally (e.g. clap cookbook) but I use docsrs for nightly rust features, like auto-cfg, so I can locally run cargo check --all-features without it bombing out.

Urgau commented 8 months ago

https://github.com/rust-lang/cargo/pull/13316 was just merged. This PR is being built from two rustc changes https://github.com/rust-lang/rust/pull/119473 and https://github.com/rust-lang/rust/pull/119930 which fixes some issues with empty values().

For Cargo this means that when there are no features declared, we now correctly declare the config feature as an expected name (but without any expected values). This makes the Cargo --check-cfg invocation more in line with the reality and allows better diagnostics:

- warning: unexpected `cfg` condition name: `feature`
+ warning: unexpected `cfg` condition value: `feature`
    --> $DIR/cargo-feature.rs
     |
  LL | #[cfg(feature = "serde")]
-    |       ^^^^^^^^^^^^^^^^^
+    |       ^^^^^^^^^^^^^^^^^ help: remove the condition
     |
-    = help: consider defining some features in `Cargo.toml`
+    = note: no expected values for `feature`
+    = help: consider adding `serde` as a feature in `Cargo.toml`

This is an answer to the 2nd Unresolved Issues, by simply improving the diagnostics.

Nemo157 commented 8 months ago

Skimming my dependencies check-cfg outputs, there's a couple I noticed:

warning: unexpected `cfg` condition name: `fuzzing`
   --> crates.io/h2-0.4.2/src/lib.rs:132:7
    |
132 | #[cfg(fuzzing)]
    |       ^^^^^^^
    |

This cfg is set by utiltiies like cargo-fuzz or cargo-afl.

warning: unexpected `cfg` condition value: `8`
   --> crates.io/tokio-util-0.7.10/src/codec/length_delimited.rs:638:9
    |
638 |         target_pointer_width = "8",
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: expected values for `target_pointer_width` are: `16`, `32`, `64`

The usage is a bit silly, but is it really bad to implement code that is forward compatible if rust ever gets an 8-bit target?

Urgau commented 8 months ago

Skimming my dependencies check-cfg outputs, there's a couple I noticed, cfg(fuzzing).

@Nemo157, fuzzing would need to be manually added to the crate expected cfgs. You can follow the warning suggestion to do that, but basically every custom cfg needs to be known so it can correctly identified as expected; in this case by adding println!("cargo:rustc-check-cfg=cfg(fuzzing)"); to the top of the crate build.rs.

warning: unexpected `cfg` condition value: `8`
   --> crates.io/tokio-util-0.7.10/src/codec/length_delimited.rs:638:9
    |
638 |         target_pointer_width = "8",
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: expected values for `target_pointer_width` are: `16`, `32`, `64`
The usage is a bit silly, but is it really bad to implement code that is forward compatible if rust ever gets an 8-bit target?

No, not at all and in fact the warning only informing you, not telling you that it's bad, that the current Rust toolchain doesn't have any knowledge of a target_pointer_width of "8". In no way should it taken as forbid kind of thing.

So as above, adding println!("cargo:rustc-check-cfg=cfg(target_pointer_width,values(\"8\"))"); to the top of the crate build.rs would mark the cfg as expected and thus "remove" the warning.

Nemo157 commented 8 months ago

I brought cfg(fuzzing) up because I feel it might fall under whatever notability requirement there is to have it as a default tool cfg (it's at about the same usage magnitude as cfg(miri), it's set by tools that are recommended by wg-secure-code, though I don't believe any of the tools are maintained as part of the project).

Urgau commented 8 months ago

I brought cfg(fuzzing) up because I feel it might fall under whatever notability requirement there is to have it as a default tool cfg [...]

I see, interesting. The current "rule" has been to include all the cfgs (including nightly ones) set by the Rust toolchain (the compiler or one of the dev tools). I don't know if we should add cfgs of tools recommended by the project, especially since that cfg will show up in the diagnostics for every users, even ones that don't use the tool, we should be careful about that.

epage commented 7 months ago

@Urgau one I just ran into with env_logger is rustbuild. Unsure if its still used today but it looks like its for when you are a dependency of rustc. A part of me doesn't want to add a build.rs just for this but I suspect its too specialized that we should not mark it as "well known".

epage commented 7 months ago

@Urgau I think a found a false positive when building https://github.com/toml-rs/toml

No kstring feature: https://github.com/toml-rs/toml/blob/27108d587fadef55551e59fd578347cc28d9e170/crates/toml_edit/Cargo.toml#L29-L40 Yet code like this is able to compile: https://github.com/toml-rs/toml/blob/27108d587fadef55551e59fd578347cc28d9e170/crates/toml_edit/src/internal_string.rs#L56-L66

I think cargo still creates feature = "foo" for optional dependencies even if an implicit feature is not created but -Zcheck-cfg is not adding it to the set of feature.

Whether we want to make a value judgement on people relying on that would be a bit wider of a conversation. If we go down that route, we should at least have a test for this.

epage commented 7 months ago

Another in the camp of "is this really worth a build.rs is a --cfg nightly as sometimes that is used for diagnostics like warn(rustdoc::missing_doc_code_examples) which are internal facing. Maybe if its only internal-facing a unstable-nightly feature would work better.

(this represents the end of running this on almost all of my repos plus some repos from others that I happen to have cloned)

Urgau commented 7 months ago

@Urgau I think a found a false positive when building https://github.com/toml-rs/toml

@epage I've looked at this and I don't think there is a bug here. While it's true that kstring is an optional dep and should have a feature, it is not the case if the the dependency is linked to any other features and indeed the kstring dependency is linked: perf = ["dep:kstring"]. So no implicit feature: kstring is created.

Trying to manually build the crate with the kstring feature leads to an error (with a great error message btw):

$ cargo +nightly check --features "kstring"
error: Package `toml_edit v0.22.4 (/tmp/toml/crates/toml_edit)` does not have feature `kstring`. It has an optional dependency with that name, but that dependency uses the "dep:" syntax in the features table, so it does not have an implicit feature with that name.

Urgau commented 7 months ago

@Urgau one I just ran into with env_logger is rustbuild. Unsure if its still used today but it looks like its for when you are a dependency of rustc. A part of me doesn't want to add a build.rs just for this but I suspect its too specialized that we should not mark it as "well known".

I don't think the cfg rustbuild is used anymore, I have never seen it and we don't expected it in bootstrap well know cfgs. The only cfg that is currently used is bootstrap or the rustc-std-workspace-core crate hack. I think you can safely remove it.

Urgau commented 7 months ago

Another in the camp of "is this really worth a build.rs is a --cfg nightly as sometimes that is used for diagnostics like warn(rustdoc::missing_doc_code_examples) which are internal facing. Maybe if its only internal-facing a unstable-nightly feature would work better.

Using a feature would also be my preference. It particular using a feature integrates better with other crates that might use the same cfg.

Nemo157 commented 7 months ago

I see there was previously a PR for declaratively specifying allowed cfg via the Cargo.toml https://github.com/rust-lang/cargo/pull/11631

I wonder if this idea could be resurrected for cases like fuzzing or loom (or nightly) where you don't necessarily have a build-script, but instead have some known cfg that will be injected by external tooling or manual RUSTFLAGS.

epage commented 7 months ago

@epage I've looked at this and I don't think there is a bug here. While it's true that kstring is an optional dep and should have a feature, it is not the case if the the dependency is linked to any other features and indeed the kstring dependency is linked: perf = ["dep:kstring"]. So no implicit feature: kstring is created.

For some reason, I thought cargo still passed them along but when checking with verbose, I'm not seeing it.

epage commented 7 months ago

Using a feature would also be my preference. It particular using a feature integrates better with other crates that might use the same cfg.

nightly is a top-level concern, rather than a concern of the direct dependent.

I wonder if this idea could be resurrected for cases like fuzzing or loom (or nightly) where you don't necessarily have a build-script, but instead have some known cfg that will be injected by external tooling or manual RUSTFLAGS.

This would likely be picked back up via https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618

ijackson commented 7 months ago

I'm here because of the Call for Testing. I tried this with derive-adhoc which does some quite exciting things in its tests.

What I found:

I had to add the println! to derive-adhoc-macros's build.rs. That build.rs only exists right now because it causes the OUT_DIR env var to be set by cargo. It would be nice to get rid of it. Adding more reasons for it is not great.
I had to add the println! to a new build.rs in derive-adhoc. That's not great.
When I did, I got the error below.
I removed the rust-version from my Cargo.toml to get it to run and then I found that I would have to add a build.rs with a println! to several of my stunt test packages.

I stopped there. I think my conclusion is that a println in build.rs may be a bad approach.

error: the `cargo::` syntax for build script output instructions was added in Rust 1.77.0, but the minimum supported Rust version of `derive-adhoc v0.9.0 (/volatile/rustcargo/Rustup/Derive-Adhoc/derive-adhoc)` is 1.56.

epage commented 7 months ago

I tried this with derive-adhoc which does some quite exciting things in its tests.

@ijackson could you go into more detail on how you are using --cfgs that you need to declare so many within your project?

When I did, I got the error below.

Also, you can use cargo: instead of removing or updating your MSRV.

I stopped there. I think my conclusion is that a println in build.rs may be a bad approach.

If we stabilize as-is, your only options will be

println
allow the warning
ignore the warning

What we need to be able to understand is

Was the feature working correctly
How representative your situation is to evaluate if the design is mature enough for stabilization

In the past, there was a proposal for a [cfg] table in Cargo.toml but this got rejected by the cargo team iirc. Generally, our hope is that https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618 will be able to replace the last remaining cases of --cfg in people's workflows (except build.rs changing lib.rs) and that would do the check-cfgs for you like features.

ijackson commented 7 months ago

Hi. Thanks for engaging.

@ijackson could you go into more detail on how you are using --cfgs that you need to declare so many within your project?

I'm not using --cfg. These are #[cfg(feature = ...)] but they aren't always declared in the Cargo.toml. This is because, for testing purposes I have multiple crates which consist of the same .rs files. For example, I have multiple versions of my main crate, and I use cargo features which exist only in the testing versions to modify the behaviour in a controlled way.

It is important that these weird testing features don't appear in the Cargo.toml of published crates.

When I did, I got the error below.

Also, you can use cargo: instead of removing or updating your MSRV.

Aha. (I'm surprised that this potential difficulty wasn't revealed when you tested your "call for testing", before publication.)

* `allow` the warning

I could use an allow but of course then I don't get the benefit of the new check. I guess I could reimplement something like the check ad-hoc with a perl script or something. Or just not have it checked; evidently I think that's tolerable since I haven't implemented an ad-hoc check myself so far :-).

That would be better than proliferating build scripts, with their effect on compile times etc.

What we need to be able to understand is
* Was the feature working correctly

None of the output suggested to me that it wasn't, apart from the complaint about MSRV.

* How representative your situation is to evaluate if the design is mature enough for stabilization

I don't know how common the techniques I have used are in the ecosystem. The key thing I'm doing is to use path = and/or symlinks to arrange that the same .rs files are used for what cargo thinks of are separate packages. The test packages have their own Cargo.tomls (which are semiautomatically maintained to be similar to the real packages).

I have found this an effective and convenient technique.

The "private version with different conditional compilations" is a bit like what #[cfg(test)] does - but my way allows testing of cross-crate behaviours. In derive-adhoc I use it to test cross-crate semver compatibility, and cross-crate import/export of derive-adhoc's template macros etc.

I think this is by far the most practical way of running these kinds of tests. The alternative would probably involve having each test run publish the current source code (perhaps with manglings) to a private testing registry, or something.

If anyone else is doing this kind of thing, you won't find the results on crates.io: after all one goal is to avoid advertising all the strange stuff. You could possibly find it by searching git repositories, but it's not clear to me what you'd meed to grep for.

epage commented 7 months ago

I'm not using --cfg. These are #[cfg(feature = ...)] but they aren't always declared in the Cargo.toml. This is because, for testing purposes I have multiple crates which consist of the same .rs files. For example, I have multiple versions of my main crate, and I use cargo features which exist only in the testing versions to modify the behaviour in a controlled way.

It is important that these weird testing features don't appear in the Cargo.toml of published crates.

@ijackson that seems well off the beaten path. Why is it the features can't appear in Cargo.toml? Is it because you don't want people using them? docs.rs has the convention of hiding features that start with _ (#10794) and #10882 is about native support for that in cargo/docs.rs/crates.io

Why are these features needed for testing? What kind of tests are these?

ijackson commented 7 months ago

@ijackson that seems well off the beaten path. Why is it the features can't appear in Cargo.toml? Is it because you don't want people using them? docs.rs has the convention of hiding features that start with _ (#10794) and #10882 is about native support for that in cargo/docs.rs/crates.io

Yes, that is why. As you observe, the support for such private features in the ecosystem (even cargo itself) isn't complete. There are open questions, like whether --all-features would still turn them on. Obviously I absolutely don't want --all-features to turn on my crazy testing stuff, so right now I can't use this convention.

Why are these features needed for testing? What kind of tests are these?

I don't think this tracking issue is quite the right place for a full explanation. Probably the best thing is for me to refer to some of the internal doc comments, eg: https://gitlab.torproject.org/Diziet/rust-derive-adhoc/-/blob/main/tests/pub-export/pub-b/pub-b.rs?ref_type=heads#L7

Urgau commented 7 months ago

`-Zcheck-cfg` Stabilization Report :tada:

Summary

This enables rustc's checking of conditional compilation at compile time. Internally, cargo will be passing a new command line option --check-cfg to all rustc and rustdoc invocations.

By default, Cargo features will be checked along with well known cfg's (e.g. see rust-lang/rust#82450). Custom cfg's will be considered undefined and the user will need to specify them via cargo::rustc-check-cfg in their build scripts.

Since rust#117522, this only affects #[cfg]s in code and does not affect setting custom --cfg via RUSTFLAGS .

Example

cargo check

[package]
name = "y"
version = "0.1.0"
edition = "2021"

[features]
lasers = []
zapping = []

#[cfg(feature = "lasers")]  // This condition is expected, as "lasers" is an expected value of `feature`
fn shoot_lasers() {}

#[cfg(feature = "monkeys")] // This condition is UNEXPECTED, as "monkeys" is NOT an expected value of
                            // `feature`
fn write_shakespeare() {}

#[cfg(windosw)]             // This condition is UNEXPECTED, it's supposed to be `windows`
fn win() {}

warning: unexpected `cfg` condition value: `monkeys`
 --> src/lib.rs:4:7
  |
4 | #[cfg(feature = "monkeys")] // This condition is UNEXPECTED, as "monkeys" is NOT an expected valu...
  |       ^^^^^^^^^^^^^^^^^^^
  |
  = note: expected values for `feature` are: `lasers`, `zapping`
  = help: consider adding `monkeys` as a feature in `Cargo.toml`
  = note: see <https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg> for more information about checking conditional configuration
  = note: `#[warn(unexpected_cfgs)]` on by default

warning: unexpected `cfg` condition name: `windosw`
 --> src/lib.rs:8:7
  |
8 | #[cfg(windosw)]             // This condition is UNEXPECTED, it's supposed to be `windows`
  |       ^^^^^^^ help: there is a config with a similar name: `windows`
  |
  = help: consider using a Cargo feature instead or adding `println!("cargo:rustc-check-cfg=cfg(windosw)");` to the top of a `build.rs`
  = note: see <https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#check-cfg> for more information about checking conditional configuration

warning: `foo` (lib) generated 2 warnings

Well known cfgs

When the feature is enabled, Cargo inherits the well-known names and values from rustc. These well known cfgs make the feature near-zero impact for the vast majority of users, as they don't need to mark them as expected.

Cargo also adds a few of it's own:

feature with the list of features
docsrs (as it has a closer relationship with the crate ecosystem and by extension Cargo than rustc).

Performance

The feature has very little performance impact, zero for small crates and a few milliseconds max for huge crates (>99000 cfgs, see below).

Was done using `hyperfine` on an Intel Core i7-7700HQ (4 cores, 3.8GHz max). They represent both extreme, helloworld (0 feature and 1 cfg) and windows-rs (~700 features and >99000 cfgs in the code). ### `windows` crate ``` $ hyperfine --prepare "cargo clean" "cargo +nightly check" "cargo +nightly check -Zcheck-cfg" Benchmark 1: cargo +nightly check Time (mean ± σ): 805.4 ms ± 12.5 ms [User: 675.2 ms, System: 150.8 ms] Range (min … max): 792.1 ms … 824.6 ms 10 runs Benchmark 2: cargo +nightly check -Zcheck-cfg Time (mean ± σ): 822.5 ms ± 11.9 ms [User: 691.1 ms, System: 150.7 ms] Range (min … max): 803.6 ms … 836.6 ms 10 runs Summary cargo +nightly check ran 1.02 ± 0.02 times faster than cargo +nightly check -Zcheck-cfg ``` Or around +17.1ms for 4 (local) crates, with the being one being at ~700 features and 99102 total `#[cfg]`s in the code! (Also note that there was 10/15 warnings and they were not suppressed so the linting/display machinery was printing them) ### helloworld (lib) ``` $ hyperfine --prepare "cargo clean" "cargo +nightly check" "cargo +nightly check -Zcheck-cfg" Benchmark 1: cargo +nightly check Time (mean ± σ): 67.5 ms ± 1.1 ms [User: 30.3 ms, System: 34.9 ms] Range (min … max): 65.9 ms … 69.2 ms 20 runs Benchmark 2: cargo +nightly check -Zcheck-cfg Time (mean ± σ): 67.4 ms ± 0.9 ms [User: 31.0 ms, System: 34.0 ms] Range (min … max): 65.4 ms … 69.4 ms 21 runs Summary cargo +nightly check -Zcheck-cfg ran 1.00 ± 0.02 times faster than cargo +nightly check ``` No visible impact.

Enable-by-default

Per the tracking issue, there is one last but important question to address:

Should these flags be enabled by default? What's the unintrusive interface to provide for users to opt-out the checking?

I propose that the behavior be enabled by default without a way to opt out, otherwise users will probably not see their mistakes in time or not know about the flag at all.

Impact

A crater run was done in rust#120701, the full summary can be found there. The sort version is:

After manually checking 3 categories (most unexpected features, target_*, and general) I wasn't able to find any false positive[^false_positives]; in total 9649 actions would need to be taken across 5959 projects (1.4% of all the projects tested), by either: fixing 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

A Call for testing was also performed and the responses were generaly positive, there were questions about the default set well know names/values and some anti pattern #[path] file sharing, but those are ortogonal to this stabilization.

Mitigations options

However to mitigate it's impact we probably want to annonce the feature ahead of time and provide ahead of time a "migration guide" for users of custom configs.

None

We can consider that the impact is minimal enough and that positives of the feature are strong enough to not do anything more.

Edition bound

We have a edition (2024) that is approaching, we could bound the feature to being enabled on being enabled only for edition>=2024.

on Cargo side

We would only pass --check-cfg on newer editions.

on rustc side

We (Cargo) would always pass --check-cfg but the lint would be allow by default for edition<2024 and warn-by-default for edition>=2024.

Documentation and Testing

The feature is currently documented in the check-cfg section of the Cargo unstable features chapter. Stabilizing the feature would mainly involve documentating cargo::rustc-check-cfg as stable and providing users with a "migration page".

The feature is being extensively tested under tests/testsuite/check_cfg.rs (~20 tests). To give a some highlights:

features_with_namespaced_features: test the feature with optional deps, explicit feature and path dependencies
well_known_names_values: test that even without any Cargo features Cargo still emits the appropriate --check-cfg args
well_known_names_values_doctest: test the integration with rustdoc
build_script: test cargo::rustc-check-cfg in build script
features_fingerprinting: simple test that makes sure that Cargo rerun rustc if the delcared features changes

Real World Usage

Unresolved questions

How to deal with the impact of enabling the feature? See above for run down of the options.
Should the build script directive warning be appeased ahead of the stabilization ?

epage commented 7 months ago

@rfcbot fcp merge

rfcbot commented 7 months ago

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

[x] @Eh2406
[x] @Muscraft
[x] @arlosi
[x] @ehuss
[x] @epage
[ ] @joshtriplett
[ ] @weihanglo

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented 7 months ago

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot commented 7 months ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

ErichDonGubler commented 3 months ago

Shout-out to the Cargo team for developing this validation that caught a very real bug for Mozilla at https://github.com/ErichDonGubler/moz-webgpu-cts/pull/115. 👏🏻 Thank you!

weihanglo commented 3 months ago

Credit goes to @Urgau who did most of the work and still take care of it now :)

rust-lang / cargo

Tracking Issue for rustc `--check-cfg` integration #10554

Summary

Unresolved Issues

Post-RFC decisions

Future Extensions

About tracking issues

Implementation history:

Unified options

New defaults

`-Zcheck-cfg` Stabilization Report :tada:

Summary

Example

Well known cfgs

Performance

Enable-by-default

Impact

Mitigations options

None

Edition bound

on Cargo side

on rustc side

Documentation and Testing

Real World Usage

Unresolved questions

rust-lang / cargo

Tracking Issue for rustc `--check-cfg` integration #10554

Summary

Unresolved Issues

Post-RFC decisions

Future Extensions

About tracking issues

Implementation history:

Unified options

New defaults

-Zcheck-cfg Stabilization Report :tada:

Summary

Example

Well known cfgs

Performance

Enable-by-default

Impact

Mitigations options

None

Edition bound

on Cargo side

on rustc side

Documentation and Testing

Real World Usage

Unresolved questions

`-Zcheck-cfg` Stabilization Report :tada: