Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected cfgs warns on quite-intentional user-cfgs #124821

Closed
workingjubilee opened this issue May 6, 2024 · 9 comments
Closed

Unexpected cfgs warns on quite-intentional user-cfgs #124821

workingjubilee opened this issue May 6, 2024 · 9 comments
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. F-check-cfg --check-cfg T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented May 6, 2024

Cargo features are not actually appropriate for all conditional compilation, as not all conditional-compilation-keys are additive. Some are in fact mutually exclusive. Cargo does not have an adequate answer for this problem yet. It is my opinion that in general, except when it is likely a typo (levenshtein 1 from an expected cfg key or key-value pair) or is an unknown value for an expected cfg key, the Rust compiler should not be quite as judgemental of "interesting" uses of cfgs.

Because rustc complained about so many cfgs, this basically clobbered backtrace-rs with a million irrelevant warnings that almost drowned out the one or two interesting things that did need to be fixed.

There are also ecosystem crates that use the pattern of requiring users to set a cfg to enable something, to yield control to the binary's builder, despite being a library crate, or that use it for things that should not be resolved the same way Cargo features are.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 6, 2024
@workingjubilee workingjubilee added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. F-check-cfg --check-cfg labels May 6, 2024
@workingjubilee
Copy link
Member Author

cc @Urgau

@workingjubilee
Copy link
Member Author

To be clear a large part of my complaint here is "there are cases where this is going to produce 100 warnings and then like 10 of them are going to be really good catches but they wind up buried under the other 90".

@Noratrieb
Copy link
Member

The idea is to write a build scripts that emits the check cfg directives to tell rustc which ones to additionally expect.

@Urgau
Copy link
Member

Urgau commented May 6, 2024

To be clear a large part of my complaint here is "there are cases where this is going to produce 100 warnings and then like 10 of them are going to be really good catches but they wind up buried under the other 90".

That's why we have a mechanism for adding custom cfg, it's the cargo::rustc-check-cfg Cargo instruction.

And if you don't want to build scripts, then crates are intended to use Cargo features instead.

@workingjubilee
Copy link
Member Author

And if someone doesn't want a build script and doesn't want a Cargo feature?

@workingjubilee workingjubilee added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 6, 2024
@Urgau
Copy link
Member

Urgau commented May 6, 2024

There isn't a third option, either it's a custom cfg and so users are expected to use a build.rs or it's a Cargo feature and they have nothing to do.

rustc (and by extension Cargo) needs to known the expected cfgs in order to lint on the unexpected ones and since there is two "kind" of cfg, custom cfg and Cargo feature, for Cargo feature there is nothing else to do, and for the custom ones we added the cargo::rustc-check-cfg instruction.

@workingjubilee
Copy link
Member Author

There is a third option: be more conservative about when to issue the lint.

@epage
Copy link
Contributor

epage commented May 6, 2024

Should this be closed as duplicate of #124800 to consolidate the discussion?

@workingjubilee
Copy link
Member Author

Yeah for some reason #124800 slipped my notice when I scanned for duplicate issues.

@workingjubilee workingjubilee closed this as not planned Won't fix, can't repro, duplicate, stale May 6, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 6, 2024
tbu- added a commit to tbu-/rust that referenced this issue May 7, 2024
This allows to find solutions to the false positives that were found in
the ecosystem before turning it to `warn` by default again.

Most projects hit by this seem to just disable the warning, which
indicates that it isn't working as expected.

CC rust-lang#124800
CC rust-lang#124804
CC rust-lang#124821
CC hyperium/hyper#3660
CC microsoft/windows-rs#3022
CC rust-bitcoin/rust-bitcoin#2748
CC tokio-rs/tokio#6538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. F-check-cfg --check-cfg T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants