rust-lang
Add new `invalid_build_cfg` lint
Fixes https://github.com/rust-lang/rust-clippy/issues/9419.
A few questions here though: I saw that the PRINT_STDOUT/PRINT_STDERR lints were only supposed to run on build.rs files (like this lint) and I assumed they were tested in tests/workspace_test/build.rs, but it seems I was wrong as I don't see any UI file. So question is: how could I test this lint?
Another important point: this lint can only be written before the expansion pass (because of the cfg! macro), forcing me to add it to this pass. Is it okay?
changelog: Add new invalid_build_cfg lint
r? @y21
rustbot has assigned @y21. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Re. pre expansion passes: I personally think it would be fine for something like build script-specific lints where I think the problems that those lints typically have (lint level attributes not properly propagating) isn't as much of a concern since a build script is its own crate with almost always just one module, but I'm gonna mention @flip1995 here because I think he has a stronger opinion on adding more pre expansion lints from what I've seen.
I am a little worried about this being fairly FP-heavy and warn-by-default at the same time since there are genuine use cases for cfg! in build scripts (e.g. for deciding whether to spawn powershell or a unix shell, it wouldn't make sense to use cargo env vars), but we can wait with the category until the FCP
pre-expansion lints are indeed not my favorites. The allowing of those lints is the main problem. I can't quite remember if the problem was with outer, inner or crate level lint attributes though...
I am a little worried about this being fairly FP-heavy and warn-by-default at the same time since there are genuine use cases for
cfg!in build scripts
That concern, I share.
It believe that #[cfg] attrs are not preserved after expansion, which I think is why all #[cfg] related lints run in pre-expension in EarlyAttributes.
Regarding the high risk of FP, that is a valid concern, which is in part why I didn't wanted this lint to be part of rustc.
Looking at GitHub there are currently around 3.3k build.rs with #[cfg(target.*, I looked at the first 3 pages and found 1 case that could be considered be a FP. That's nothing of a conclusion but I think is still draws a rough idea that FP would be low, perhaps less than 5%.
It's also worth nothing that in most cases I looked at, cross-compilation is broken for those crates (missing linking libraries, wrong cfgs, ...) and would in part be fixed by proper use of CARGO_* envs.
I also think if the lint clearly indicate in it's output that it may be fine to have #[cfg] on host and suggest a way to silence the lint (even if it's just #[allow]/#[expect] with a SAFETY:-like comment), that it would IMO considerably reduce the annoyance that the lint may cause and still provide some value.
In other word, even if it may be a FP (in the sense the cfg is really for the host and not the target) there would still be value for crate authors/maintainers, by indicating the reason for the host cfg, à la SAFETY: comment (similar to what is done with the unsafe-op-in-unsafe-fn lint).
Hey @GuillaumeGomez , this is a ping from triage, since there hasn't been any activity in some time. Are you still planning to continue this implementation?
If you have any questions, you're always welcome to ask them in this PR or on Zulip.
@rustbot author
I'm waiting to know if the clippy team is going to accept this lint and if so, with which changes. Could it be put on the calendar @xFrednet please?
Sure! The I-nominated label is used for that. I've added it now, but feel free to add it yourself, next time :)
:umbrella: The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.
:umbrella: The latest upstream changes (possibly d28d2344d000aa96bef729cf408731f952f71fb0) made this pull request unmergeable. Please resolve the merge conflicts.
Zulip thread discussing this lint is here.
@GuillaumeGomez have you looked into implementing this in a way that allows lint level attributes to work? The zulip thread ended with it probably being better to implement in rustc.
