Add new `invalid_build_cfg` lint #12862

GuillaumeGomez · 2024-05-28T16:18:22Z

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

rustbot · 2024-05-28T16:18:27Z

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

Urgau

I like this. Great to progress around this area.

I left some comments about the implementation and the lint name.

Urgau · 2024-05-28T21:26:49Z

clippy_lints/src/invalid_build_cfg.rs

+    /// if std::env::var("CARGO_CFG_WINDOWS").is_ok() {}
+    /// ```
+    #[clippy::version = "1.80.0"]
+    pub INVALID_BUILD_CFG,


Given that there are legitimate use cases for host-config in build.rs, I think it would be better if the lint name acknowledges it. the linked issue proposed misleading_cfg_in_build_script.

Suggested change

pub INVALID_BUILD_CFG,

pub MISLEADING_CFG_IN_BUILD_SCRIPT,

Urgau · 2024-05-28T21:27:08Z

clippy_lints/src/invalid_build_cfg.rs

+    #[clippy::version = "1.80.0"]
+    pub INVALID_BUILD_CFG,
+    suspicious,
+    "invalid use of cfg in `build.rs`"


Suggested change

"invalid use of cfg in `build.rs`"

"use of host configs in `build.rs`"

Urgau · 2024-05-28T21:28:52Z

clippy_lints/src/invalid_build_cfg.rs

+enum CfgAst {
+    Os(Symbol),
+    Any(Vec<CfgAst>),
+    All(Vec<CfgAst>),
+    Not(Box<CfgAst>),
+    TargetKeyValue(Symbol, Symbol),
+    Feature(Symbol),
+    OtherTarget(Symbol, Symbol),
+}


Some documentation would be nice, it's not immediately clear why Feature, Os and OtherTarget exists.

Urgau · 2024-05-28T21:34:10Z

clippy_lints/src/invalid_build_cfg.rs

+fn is_build_script(cx: &EarlyContext<'_>) -> bool {
+    cx.sess()
+        .opts
+        .crate_name
+        .as_ref()
+        .map_or(false, |crate_name| crate_name == "build_script_build")
+}


It would also be good to check if we are actually being called by Cargo.
There is a nice function for that, rustc_session::utils::was_invoked_from_cargo.

Urgau · 2024-05-28T21:36:35Z

tests/ui/invalid_build_cfg.rs

+    if cfg!(not(all(target_os = "freebsd", windows))) {}
+    if cfg!(all(target_os = "freebsd", any(windows, not(feature = "red")))) {}
+
+    // Should not warn.


Would also be good to add tests for custom cfgs, like loom, fuzzing, ...

Urgau · 2024-05-28T21:52:04Z

clippy_lints/src/invalid_build_cfg.rs

+    let mut tokens = tokens.trees().peekable();
+
+    while let Some(token) = tokens.next() {
+        match token {
+            TokenTree::Token(token, _) => {
+                match token.kind {
+                    TokenKind::Ident(name, _) => {
+                        if name == sym::feature || name.as_str().starts_with("target_") {
+                            if let Some(next_token) = tokens.next()
+                                && let TokenTree::Token(next_token, _) = next_token
+                                && matches!(next_token.kind, TokenKind::Eq)
+                                && let Some(next_token) = tokens.next()
+                                && let TokenTree::Token(next_token, _) = next_token
+                                && let TokenKind::Literal(lit) = next_token.kind
+                                && matches!(lit.kind, LitKind::Str | LitKind::StrRaw(_))


Instead of all this custom parsing logic could it be possible to use MetaItem::from_tokens? which is what I think the compiler is using internally; and then use meta_list_item.

y21 · 2024-06-01T11:56:10Z

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

flip1995 · 2024-06-03T09:24:44Z

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.

Urgau · 2024-06-03T11:09:49Z

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).

xFrednet · 2024-06-30T20:03:57Z

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

GuillaumeGomez · 2024-07-01T09:27:18Z

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?

xFrednet · 2024-07-01T12:16:13Z

Sure! The I-nominated label is used for that. I've added it now, but feel free to add it yourself, next time :)

GuillaumeGomez · 2024-07-01T12:17:05Z

Noted, thanks!

bors · 2024-07-17T19:03:29Z

☔ The latest upstream changes (presumably #13088) made this pull request unmergeable. Please resolve the merge conflicts.

Add new invalid_build_cfg lint

72a580c

rustbot assigned y21 May 28, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 28, 2024

Urgau reviewed May 28, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 30, 2024

xFrednet added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 1, 2024

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `invalid_build_cfg` lint #12862

Add new `invalid_build_cfg` lint #12862

GuillaumeGomez commented May 28, 2024

rustbot commented May 28, 2024

Urgau left a comment

Urgau May 28, 2024

Urgau May 28, 2024

Urgau May 28, 2024

Urgau May 28, 2024

Urgau May 28, 2024 •

edited

Loading

Urgau May 28, 2024

y21 commented Jun 1, 2024

flip1995 commented Jun 3, 2024

Urgau commented Jun 3, 2024 •

edited

Loading

xFrednet commented Jun 30, 2024

GuillaumeGomez commented Jul 1, 2024

xFrednet commented Jul 1, 2024

GuillaumeGomez commented Jul 1, 2024

bors commented Jul 17, 2024

	"invalid use of cfg in `build.rs`"
	"use of host configs in `build.rs`"

Add new invalid_build_cfg lint #12862

Are you sure you want to change the base?

Add new invalid_build_cfg lint #12862

Conversation

GuillaumeGomez commented May 28, 2024

rustbot commented May 28, 2024

Urgau left a comment

Choose a reason for hiding this comment

Urgau May 28, 2024

Choose a reason for hiding this comment

Urgau May 28, 2024

Choose a reason for hiding this comment

Urgau May 28, 2024

Choose a reason for hiding this comment

Urgau May 28, 2024

Choose a reason for hiding this comment

Urgau May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Urgau May 28, 2024

Choose a reason for hiding this comment

y21 commented Jun 1, 2024

flip1995 commented Jun 3, 2024

Urgau commented Jun 3, 2024 • edited Loading

xFrednet commented Jun 30, 2024

GuillaumeGomez commented Jul 1, 2024

xFrednet commented Jul 1, 2024

GuillaumeGomez commented Jul 1, 2024

bors commented Jul 17, 2024

Add new `invalid_build_cfg` lint #12862

Add new `invalid_build_cfg` lint #12862

Urgau May 28, 2024 •

edited

Loading

Urgau commented Jun 3, 2024 •

edited

Loading