-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Remove cargo::rustc-check-cfg
to only have one place to configure the unknown_cfgs
lint
#13945
Comments
The declarative approach also works for cfgs used in build.rs directly: rust-lang/rust#125368. |
Currently, we have it documented to encourage
When we document the |
@epage , please do notice my note at the end in #125368 — the fact that the build-rs-printf method does not suppress warnings creates a problem because currently it's possible for the warning to recommend a mitigation which has no effect. Assuming that isn't fixable, if you do keep in the build-rs-printf method, I think you should be careful to document build-rs-printf will not helpful if cfgs are used in build.rs itself, and ideally you should modify the warning hint to not recommend the build-rs-printf fix if it detects (because this is detectable) that the warning has been triggered within build.rs. Alternately, removing the build-rs-printf method would mean you don't need that complexity in messaging/warning plumbing. |
I agree with @epage, keeping
It is fixable. I just didn't yet have time to do it. |
While it works, it's also not 100% the same thing, the |
I see, I think this argument makes sense. It also means that tools outputting I still feel that a) having a non-declarative version of the config makes it harder for tools, because they need to run untrusted code to get a list of cfgs, and b) users need to be taught about two things that do roughly the same thing, are two significant downsides to this approach. If we started with |
With the split I gave of static vs dynamic |
Hi, I'm not sure I understand what you mean by "the split I gave of static vs dynamic cfgs", but the case I give in #125368 is the case of a .rs file which for whatever reason is |
There are currently two places where the lint can be configured, in build.rs using
cargo::rustc-check-cfg
and in theCargo.toml
in thelints
section. Generally, having only one place is better than having two, as users don't have to look into two places to find the configuration.Since this feature hasn't been shipped to a stable compiler yet, we could currently still remove one of them without backward compatibility hazards.
The possible cfgs of a crate across all build configurations are known statically, since they're at most all that appear in the source code. This means that all possible configurations should be expressible in
Cargo.toml
.The text was updated successfully, but these errors were encountered: