Skip to content

Add new misleading_cfg_in_build_script lint#153721

Open
GuillaumeGomez wants to merge 6 commits into
rust-lang:mainfrom
GuillaumeGomez:misleading_cfg_in_build_script
Open

Add new misleading_cfg_in_build_script lint#153721
GuillaumeGomez wants to merge 6 commits into
rust-lang:mainfrom
GuillaumeGomez:misleading_cfg_in_build_script

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Member

@GuillaumeGomez GuillaumeGomez commented Mar 11, 2026

View all comments

Fixes #125441.
Fixes rust-lang/rust-clippy#9419.

Take-over of rust-lang/rust-clippy#12862.

It was originally implemented in clippy but the clippy thought it made for sense for such a lint to be directly implemented in rustc. I applied all suggestions made on the original PR and also improved the code overall (added docs too).

So on a build.rs from cargo crate, we will check for any target_*, unix and windows cfg and warn about them.

Since you made the original review on clippy.

r? @Urgau

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 11, 2026

Some changes occurred in check-cfg diagnostics

cc @Urgau

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2026
@GuillaumeGomez GuillaumeGomez force-pushed the misleading_cfg_in_build_script branch from 262d2e2 to 3e99ab7 Compare March 11, 2026 14:35
@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Added a ui test to ensure this lint is only emitted on cargo build.rs scripts.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the misleading_cfg_in_build_script branch from 3e99ab7 to ea1c733 Compare March 11, 2026 16:00
@rust-log-analyzer

This comment has been minimized.

Comment on lines +202 to +205
fn is_build_script(cx: &EarlyContext<'_>) -> bool {
rustc_session::utils::was_invoked_from_cargo()
&& cx.sess().opts.crate_name.as_deref() == Some("build_script_build")
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems quite hacky and brittle for a lint.

Is there a better to detect if we are compiling a Cargo build script? If there isn't, we could maybe consider having the lint allowed by-default, but have Cargo implicitly set it to warn for build-scripts.

cc @rust-lang/cargo

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, not great. :-/

I was kinda hoping that we had a "build script" crate kind but we don't (which is logical but still, not fun).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just put it in t-cargo meeting agenda :)
next meeting time: Tue, Mar 17, 2026, 10:00 UTC-4

https://hackmd.io/-vJMV3isQfqu2d2J91t1eA

@GuillaumeGomez GuillaumeGomez force-pushed the misleading_cfg_in_build_script branch from ea1c733 to 28c7221 Compare March 11, 2026 20:58
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

CI is finally happy. ^^'

@ehuss ehuss added the I-cargo-nominated Nominated for discussion during a cargo team meeting. label Mar 13, 2026
@ehuss ehuss moved this to Nominated in Cargo status tracker Mar 17, 2026
@ehuss
Copy link
Copy Markdown
Contributor

ehuss commented Mar 17, 2026

The cargo team discussed this today (notes). Some comments from that discussion:

  1. We were largely OK with the approach of making this allow-by-default and having cargo set it to a warning when building a build script.
  2. I was concerned about the possible amount of false-positives this would generate. Has there been a crater run or other analysis of what kind of false-positives this might hit? For example, if some build script has "cfg(windows) ... run executable that only exists on windows", it seems like this would be a false positive.
  3. I assume this would need lang-team approval. It might be good to get this on their radar.
  4. As an outsider looking at the diff, I am rather surprised to see yet another cfg parser being implemented here. I'm curious why this isn't using the standard cfg parser?

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

  1. Sounds good.
  2. It's a valid concern, to which I don't really have an answer to. Only answer is to run a crater run to see the impact it would have.
  3. We're not clear which team is "in charge" here.
  4. Unless I missed something, the cfg parser requires data I don't have? I'd love to be proven wrong though, would reduce the code quite a lot!

@Urgau
Copy link
Copy Markdown
Member

Urgau commented Mar 17, 2026

I assume this would need lang-team approval. It might be good to get this on their radar.

I would expect a T-cargo FCP, with maybe T-compiler, but not T-lang. The lint proposed here has nothing to do with the language, it's purely a compiler and Cargo affair.

To elaborate a bit more, this lint could be in cargo it-self if we had a custom linting system, and it wouldn't need T-lang approval for that, so I don't think it needs one. We also (T-compiler) didn't ask T-lang to FCP the #[cfg]/--cfg lints.

As an outsider looking at the diff, I am rather surprised to see yet another cfg parser being implemented here. I'm curious why this isn't using the standard cfg parser?

I wouldn't look to closely at the impl. I haven't yet done a review of it, I wanted to wait for T-cargo input first.

Copy link
Copy Markdown
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I share the concerns from Eric, about having another cfg parser.

Fortunately, this lint is simple enough that it can use the same infra as the unexpected_cfgs lint and perform the verification directly when parsing the cfgs them-selves in rustc_attr_parsing.

This will require some code re-shuffling between different crates (rustc_attr_parsing, rustc_lint_defs, rustc_lint, ...). You can follow what has been done for the UNEXPECTED_CFGS lint.

View changes since this review

Comment thread compiler/rustc_lint/src/misleading_cfg_in_build_script.rs Outdated
Comment thread compiler/rustc_lint/src/misleading_cfg_in_build_script.rs Outdated
Comment thread compiler/rustc_lint/src/misleading_cfg_in_build_script.rs Outdated
@Urgau Urgau added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 18, 2026
@GuillaumeGomez GuillaumeGomez force-pushed the misleading_cfg_in_build_script branch from 2d14a6b to 06e2527 Compare March 19, 2026 13:34
@rust-log-analyzer

This comment has been minimized.

@ehuss
Copy link
Copy Markdown
Contributor

ehuss commented Mar 23, 2026

I would expect a T-cargo FCP, with maybe T-compiler, but not T-lang. The lint proposed here has nothing to do with the language, it's purely a compiler and Cargo affair.

I don't think that's how lints have been handled in the past. They all need to be approved by the lang team. I'm not aware of any exclusions.

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

I'm surprised: I added lints in the past without needing t-lang approval (although it might have been a few years for the last one... ^^').

@traviscross traviscross added T-lang Relevant to the language team I-lang-nominated Nominated for discussion during a lang team meeting. labels Mar 23, 2026
rust-bors Bot pushed a commit that referenced this pull request May 11, 2026
…, r=<try>

Add new `misleading_cfg_in_build_script` lint
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 11, 2026

☀️ Try build successful (CI)
Build commit: 174774e (174774e66d501beae7194df11e0b825ba98f06ab, parent: 64a965e9013a9d14e83c4d370af26f6be6bf96fb)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Error occured while categorizing benchmark run:

could not find start commit for bound Commit("64a965e9013a9d14e83c4d370af26f6be6bf96fb")

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

Failed again? O.o

Let's try a full thing just in case. ^^'

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request May 11, 2026
…, r=<try>

Add new `misleading_cfg_in_build_script` lint
@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented May 11, 2026

rust-lang/triagebot#2401 should fix the rustc-perf error.

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 12, 2026

☀️ Try build successful (CI)
Build commit: be965f1 (be965f1eed9ef118145a9e32331b9ce1e57874b2, parent: 64a965e9013a9d14e83c4d370af26f6be6bf96fb)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Error occured while categorizing benchmark run:

could not find start commit for bound Commit("64a965e9013a9d14e83c4d370af26f6be6bf96fb")

@GuillaumeGomez
Copy link
Copy Markdown
Member Author

I'll wait for it to fixed. Please restart it once fixed on your side @Kobzol. ;)

@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented May 12, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request May 12, 2026
…, r=<try>

Add new `misleading_cfg_in_build_script` lint
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 12, 2026

☀️ Try build successful (CI)
Build commit: f7e8e16 (f7e8e162edaeb4d30b8bb9761903d167e58b3844, parent: 29b7590130c83542a095cdf1323ed0f78eec2bb8)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (f7e8e16): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-1.0%, -0.8%] 6
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 1

Max RSS (memory usage)

Results (primary 2.6%, secondary -1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.4% [-1.4%, -1.4%] 1
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

Cycles

Results (secondary -1.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.2% [2.4%, 4.3%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.4% [-4.0%, -2.3%] 5
All ❌✅ (primary) - - 0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 517.584s -> 509.112s (-1.64%)
Artifact size: 398.01 MiB -> 398.05 MiB (0.01%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 12, 2026
@GuillaumeGomez
Copy link
Copy Markdown
Member Author

@Urgau How does it look to you?

Comment thread compiler/rustc_ast/src/attr/mod.rs Outdated
@Urgau
Copy link
Copy Markdown
Member

Urgau commented May 13, 2026

How does it look to you?

I haven't done yet a deep-dive review, but that looks pretty good.

Happy to see the logic moved directly in the parsing, it seems to simplify a bunch of things. Even perf seems to be happy.

I will try to do a review before RustWeek, otherwise it will after.

@Urgau Urgau added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. perf-regression Performance regression. labels May 13, 2026
@GuillaumeGomez GuillaumeGomez force-pushed the misleading_cfg_in_build_script branch from 0ecdc4f to a114a87 Compare May 14, 2026 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) I-lang-radar Items that are on lang's radar and will need eventual work or consideration. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Warn for cfg!(target_* = "whatever") usage in build scripts Lint for cfg usage which would provide a misleading result in a build script.