New rustc nightly suggests adding a `build.rs` to use conditional compilation

[TheBlueMatt]

We use conditional compilation in a lot of places (eg the fuzzing cfg flag is standard to conditionally compile when fuzzing in the rust-fuzz ecosystem) and have a hard rule against unnecessary build.rss (as running build-time code is a red flag when auditing a crate and requires special care). Latest rustc nightly now generates a huge pile of warnings encouraging us to add a build.rs to make them go away, which isn't really acceptable. It seems this is encouraging bad practice to respond to a common practice - is there some way to just list fuzzing and some other super common flags and allow those?

[saethlin]

Yes there is a list of well-known cfgs, for example #124742. fuzzing would be a good addition. We did already try to add the super common ones, so you'll have to be specific about what others would be helpful.

[TheBlueMatt]

Fuzzing is the biggest ecosystem-wide one, but we also use cfg flags to land code that isn't complete yet and we don't want to enable but which requires a long series of steps and we don't want to try to land it all in one go, so we need the ability to add our own in Cargo.toml. I can't imagine this is a super uncommon use?

[apoelstra]

I fairly frequently use cfg flags to disable key parts of my crates (mainly: heavy cryptography) for the purpose of speeding up a fuzzer or allowing it to forge signatures so it can hit more paths. The names of these are deliberately uncommon and project-specific.

So I would also like a way to whitelist cfg flags. Cargo.toml would be great.

For now I am disabling the lint entirely with allow but I would like to have the lint on, since I've been burned by cfg typos many times in the past. But adding a build.rs will slow down compilation and raise eyebrows of anybody reviewing my crates.

[Urgau]

have a hard rule against unnecessary build.rs

It wouldn't be unnecessary, it would be used.

It seems this is encouraging bad practice

Adding a build.rs is not a bad practice.

fuzzing would be a good addition

The rule of thumb that was agreed in the stabilization of --check-cfg was to only include a restricted set of cfgs and only the least common for all users of Rust, including ones that do not use Cargo, like Rust-for-Linux.

is there some way to just list fuzzing

Yes in a build.rs with cargo::rustc-check-cfg=cfg(fuzzing). Or using a Cargo feature instead.

[TheBlueMatt]

Sorry, but "add additional code that runs at build-time that users have to audit separately just to disable a compile-time warning" is really offensive stance. Indeed, some folks have legitimate uses for build.rs, and within parts of the Rust ecosystem build.rs is a totally accepted practice, but that definitely isn't universal and for those building security-conscious software build.rs is an orange flag that requires extra auditing and care, and is absolutely not something that is universally acceptable without consideration.

More broadly, I also work on some projects that are optionally being built directly with rustc rather than cargo. While those builds won't suffer from this issue (AFAIU?), having a build.rs is obviously pretty painful in those cases.

[epage]

Fuzzing is the biggest ecosystem-wide one, but we also use cfg flags to land code that isn't complete yet and we don't want to enable but which requires a long series of steps and we don't want to try to land it all in one go, so we need the ability to add our own in Cargo.toml. I can't imagine this is a super uncommon use?

imo this sounds like a case for features. If you are concerned about others using it, there is a practice for unstable- prefixes and a proposal for official unstable feature support.

So I would also like a way to whitelist cfg flags. Cargo.toml would be great.

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

I've created the following Pre-RFC that would create something for Cargo but don't have the time to drive it to completion and no one else has picked it up

https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618

[TheBlueMatt]

imo this sounds like a case for features. If you are concerned about others using it, there is a practice for unstable- prefixes and a proposal for official unstable feature support.

There's a few issues with this - first of all just calling something "unstable-" doesn't mean no one can enable it, and it means a transitive dependency somewhere having a bug will mean my crate will suddenly generate completely bogus results. More generally, this isn't an "unstable" feature but rather a "you should never, ever, ever, ever enable this feature unless you're fuzzing, don't touch this".

However, more broadly, we rely on transitive fuzzing features in some cases, and the standard, ecosystem-wide cfg=fuzzing is really nice for that - I don't have to think about which transitive dependencies I have that have fuzzing-specific optimizations, and those transitive dependencies don't have to worry about exposing any kind of public API for fuzzing code enabling - the fuzzers just automatically set cfg=fuzzing and we're off to the races.

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

Then maybe the warnings should be off by default until we make progress here? Or, at a minimum, the suggested fix should be to turn off the warnings, rather than add a build.rs to all our crates.

--cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

Yep, that's exactly why we use them - they're "transparent" to cargo which makes them incredibly useful for things like fuzzing and in-progress code. Suddenly throwing tons of warnings at users for what seems like a perfectly reasonable use of a language feature seems like quite a regression (but of course I understand the utility of these warnings - we have existing python scripts that parse our code and detect undefined cfg flags :) ).

[saethlin]

imo this sounds like a case for features

Not the fuzzing component. cfgs like miri, fuzzing, and loom are used for ecosystem-wide coordination. They exist to be well-known. #124804

[apoelstra]

The original RFC proposed a [cfg] syntax but the cargo team rejected it as --cfgs are a low level rustc abstraction that don't fit within cargo (iiuc).

If this is true then why is Cargo giving flags to rustc that complain about them?

As Matt is saying, (one) purpose of using cfg flags is to bypass Cargo and its opinions.

[workingjubilee]

I refuse to believe that T-compiler, no matter what was proposed or read, really intended for dropping large swathes of needless lints on crates.io. The implications of this seem like an accident, much like #121708 was an accident.

It is my opinion that the lint is actually quite useful when it detects things like this:

#[cfg(target_os = "widnosw")]

or things like this:

#[cfg(linux)]

It is not impossible someone would want to use #[cfg(linux)], but that's a known value of target_os, so it is more likely you meant that. So it seems it would be acceptable to fire the lint on cases where rustc thinks you meant something else, but is wrong.

But I don't think "this only bothers maintainers"... since when are those people with lots of time on their hands and happy to spend it on squashing trivial lints?... justifies the current implementation. It is particularly sad to me because I noticed that the large swathes of false positives almost completely drown out cases where a more cautious implementation would catch something. I only caught the snippets of signal it actually sent to backtrace-rs because I paid very close attention... and I still ended up only throwing in those two actual fixes and simply adding this to the Cargo.toml:

[lints]
unexpected_cfgs = "allow"

As to whatever ideas the Cargo team has regarding future work done in this area: those sound good! I think I've expressed tentative support for one of them, the global features thing, and it cuts fairly close to Job Reasons, so I might even be able to start working on it out of paid time. But they also seem like a good reason for the lint to be more restrained than firing according to its current brutalistically simple logic, since the preferred implementation depends on work that hasn't been done yet.

[workingjubilee]

Also, I should note, that it was always my impression that this was primarily intended to catch this, or I might have said something sooner:

#[cfg(target_os = "widnosw")]

This is an expansive set of linting decisions that have all been landed as one blob. Deciding to lint on #[cfg(target_os = "widnosw")] is different than deciding to lint on #[cfg(linux)], which is different from deciding to lint on #[cfg(ynix)], which is different than deciding to lint on #[cfg(fuzzing)]. The last is the one we have the least information about unless we bake it into the compiler (and then we re-raise the question of other one-off cfgs).