Include declared list of features in fingerprint for `-Zcheck-cfg` #13012

Urgau · 2023-11-19T17:59:07Z

This PR include the declared list of features in fingerprint for -Zcheck-cfg (#10554)

--check-cfg verifies that #[cfg()] attributes are valid in Rust code, which includes --cfg features foo definitions that came from [features] tables in Cargo.toml. For us to correctly re-check cfgs on feature changes,we need to include them in the fingerprint.

For example, if a user deletes an entry from [features], they should then get warnings about any #[cfg()] attributes left in the code. Without this change, that won't happen. With this change, it now does.

rustbot · 2023-11-19T17:59:12Z

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

Urgau · 2023-12-01T18:28:54Z

@epage Is there something I can do to help move this PR forward?

epage · 2023-12-01T18:37:24Z

I'm still catching up after the US holiday last week

weihanglo

This PR does exactly what it advertises, though I don't know where it comes from. Can you share prior discussions or references in the PR description?

epage · 2023-12-01T19:12:24Z

I've updated the description to add more context

epage · 2023-12-01T19:15:05Z

tests/testsuite/check_cfg.rs

@@ -141,6 +141,53 @@ fn features_with_namespaced_features() {
        .run();
 }

+#[cargo_test(nightly, reason = "--check-cfg is unstable")]
+fn features_fingerprint() {


This test is verifying the fingerprinting itself but not the end-to-end desired result.

I would recommend updating the test to have a #[cfg()] attribute in the source and then the change_file call will remove the feature and a check-cfg warning should now show up.

I would also recommend adding the test in its own commit and making it the first commit. It should pass, showing the bad behavior. Then in the commit you update the fingerprint code, you update the test as well.

This will

Test the test

Show reviewers that the code change had the desired result

In particular, this would have helped a lot with weighanglo's question.

This test is verifying the fingerprinting itself but not the end-to-end desired result.

I'm always a bit hesitant to check the diagnostic output of the lint since it changes (sometimes often).

So I just check for the presence of the lint name unexpected_cfgs in the output.
And as requested I split-up the PR into a "buggy test commit first" follow by a "impl commit fix".

(I also added a third commit that avoid unnecessary invalidation when reordering the features)

src/cargo/core/compiler/fingerprint/mod.rs

Urgau · 2023-12-01T20:30:10Z

This PR does exactly what it advertises, though I don't know where it comes from.

I was in the making of GitoxideLabs/gitoxide#1123, doing cargo check -Zcheck-cfg and looking at the unexpected cfg warnings when I noticed that a feature was missing in the [features] table, so I added it and re-executed cargo check -Zcheck-cfg but the warning was still present. Debugging the issue revealed that rustc wasn't being called when updating the [features] table, so I concluded that it was missing in the fingerprint, so I made this PR.

Sorry for not mentioning it properly

Otherwise changing (add, removing, ...) features from the `[features]` table would not cause rustc to be called with the new `--check-cfg`, showing potentially outdated warnings.

epage · 2023-12-01T21:44:54Z

Thanks!

@bors r+

bors · 2023-12-01T21:45:24Z

📌 Commit f32f43d has been approved by epage

It is now in the queue for this repository.

bors · 2023-12-01T21:46:31Z

⌛ Testing commit f32f43d with merge aa6e46e...

bors · 2023-12-01T22:28:48Z

☀️ Test successful - checks-actions
Approved by: epage
Pushing aa6e46e to master...

Update cargo 25 commits in 26333c732095d207aa05932ce863d850fb309386..58fb23140972092a12f7011d17a7db1d99e3eacf 2023-11-28 20:07:39 +0000 to 2023-12-02 14:15:16 +0000 - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066) r? ghost

Update cargo 27 commits in 26333c732095d207aa05932ce863d850fb309386..623b788496b3e51dc2f9282373cf0f6971a229b5 2023-11-28 20:07:39 +0000 to 2023-12-02 18:10:03 +0000 - docs(book): make old title anchorable (rust-lang/cargo#13102) - Revert "chore(deps): update rust crate openssl to 0.10.60 [security]" (rust-lang/cargo#13101) - test(install): use TCP connection instead of thread sleep (rust-lang/cargo#13099) - test(mdman): Switch to snapbox (rust-lang/cargo#13098) - Include declared list of features in fingerprint for `-Zcheck-cfg` (rust-lang/cargo#13012) - chore(deps): update compatible (rust-lang/cargo#13083) - chore(ci): Always update gix packages together (rust-lang/cargo#13093) - chore(deps): update rust crate windows-sys to 0.52 (rust-lang/cargo#13089) - refactor(toml): Decouple logic from schema (rust-lang/cargo#13080) - Have cargo add --optional <dep> create a <dep> = "dep:<dep> feature (rust-lang/cargo#13071) - Add `--public` for `cargo add` (rust-lang/cargo#13046) - chore(deps): update rust crate toml_edit to 0.21.0 (rust-lang/cargo#13088) - chore(deps): update rust crate rusqlite to 0.30.0 (rust-lang/cargo#13087) - test(trim-paths): exercise with real world debugger (rust-lang/cargo#13091) - Fixed uninstall a running binary failed on Windows (rust-lang/cargo#13053) - chore(deps): update rust crate itertools to 0.12.0 (rust-lang/cargo#13086) - Add more options to registry test support. (rust-lang/cargo#13085) - Don't filter on workspace members when scraping doc examples (rust-lang/cargo#13077) - Remove the outdated comment (rust-lang/cargo#13076) - fix(resolver): Remove unused public-deps error handling (rust-lang/cargo#13036) - Fixes error count display is different when there's only one error left (rust-lang/cargo#12484) - fix: reorder `--remap-path-prefix` flags for `-Zbuild-std` (rust-lang/cargo#13065) - remove jobserver env var in some tests (rust-lang/cargo#13072) - doc: clarify different target has different set of `CARGO_CFG_*` values (rust-lang/cargo#13069) - docs: remove review capacity notice in PR template (rust-lang/cargo#13070) - chore(deps): update rust crate openssl to 0.10.60 [security] (rust-lang/cargo#13068) - fix(resolver): De-prioritize no-rust-version in MSRV resolver (rust-lang/cargo#13066)

Include `lints.rust.unexpected_cfgs.check-cfg` in the fingerprint ### What does this PR try to resolve? When changing the `--check-cfg` args in the `lints.rust.unexpected_cfgs.check-cfg` lint config, the build should be restarted as the arguments we pass to the compiler change, and they can change the diagnostics output by generating new or removing some `unexpected_cfgs` warning(s). ### How should we test and review this PR? Look at the before and after test (separate commit). ### Additional information Similar to #13012 which did that for the declared features. Contrary to that PR, I didn't add a new `DirtyReason` variant, since even the `[lints]` table didn't get one. r? `@epage`

rustbot assigned epage Nov 19, 2023

rustbot added A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2023

weihanglo reviewed Dec 1, 2023

View reviewed changes

epage reviewed Dec 1, 2023

View reviewed changes

weihanglo approved these changes Dec 1, 2023

View reviewed changes

epage reviewed Dec 1, 2023

View reviewed changes

src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved

Urgau added 3 commits December 1, 2023 21:59

Add test showing the buggy behavior of check-cfg and the fingerprint

6c4069b

Include declared list of features in fingerprint for -Zcheck-cfg

7d1fc45

Otherwise changing (add, removing, ...) features from the `[features]` table would not cause rustc to be called with the new `--check-cfg`, showing potentially outdated warnings.

Avoid fingerprint invalidation when reordering the declared features

f32f43d

Urgau force-pushed the check-cfg-fingerprint branch from 5a8483b to f32f43d Compare December 1, 2023 21:07

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2023

bors merged commit aa6e46e into rust-lang:master Dec 1, 2023
20 checks passed

weihanglo mentioned this pull request Dec 2, 2023

Update cargo rust-lang/rust#118541

Merged

rustbot added this to the 1.76.0 milestone Dec 3, 2023

weihanglo mentioned this pull request Dec 13, 2023

Tracking Issue for rustc --check-cfg integration #10554

Closed

2 tasks

Urgau mentioned this pull request May 23, 2024

Include lints.rust.unexpected_cfgs.check-cfg in the fingerprint #13958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include declared list of features in fingerprint for `-Zcheck-cfg` #13012

Include declared list of features in fingerprint for `-Zcheck-cfg` #13012

Urgau commented Nov 19, 2023 •

edited by epage

Loading

rustbot commented Nov 19, 2023

Urgau commented Dec 1, 2023

epage commented Dec 1, 2023

weihanglo left a comment

epage commented Dec 1, 2023

epage Dec 1, 2023

Urgau Dec 1, 2023

Urgau commented Dec 1, 2023

epage commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

Include declared list of features in fingerprint for -Zcheck-cfg #13012

Include declared list of features in fingerprint for -Zcheck-cfg #13012

Conversation

Urgau commented Nov 19, 2023 • edited by epage Loading

rustbot commented Nov 19, 2023

Urgau commented Dec 1, 2023

epage commented Dec 1, 2023

weihanglo left a comment

Choose a reason for hiding this comment

epage commented Dec 1, 2023

epage Dec 1, 2023

Choose a reason for hiding this comment

Urgau Dec 1, 2023

Choose a reason for hiding this comment

Urgau commented Dec 1, 2023

epage commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

bors commented Dec 1, 2023

Include declared list of features in fingerprint for `-Zcheck-cfg` #13012

Include declared list of features in fingerprint for `-Zcheck-cfg` #13012

Urgau commented Nov 19, 2023 •

edited by epage

Loading