Skip to content
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

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

Merged
merged 3 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/cargo/core/compiler/fingerprint/dirty_reason.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ pub enum DirtyReason {
old: String,
new: String,
},
DeclaredFeaturesChanged {
old: String,
new: String,
},
TargetConfigurationChanged,
PathToSourceChanged,
ProfileConfigurationChanged,
Expand Down Expand Up @@ -141,6 +145,9 @@ impl DirtyReason {
DirtyReason::FeaturesChanged { .. } => {
s.dirty_because(unit, "the list of features changed")
}
DirtyReason::DeclaredFeaturesChanged { .. } => {
s.dirty_because(unit, "the list of declared features changed")
}
DirtyReason::TargetConfigurationChanged => {
s.dirty_because(unit, "the target configuration changed")
}
Expand Down
23 changes: 23 additions & 0 deletions src/cargo/core/compiler/fingerprint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
//! Target Name | ✓ | ✓
//! TargetKind (bin/lib/etc.) | ✓ | ✓
//! Enabled Features | ✓ | ✓
//! Declared Features | ✓ |
//! Immediate dependency’s hashes | ✓[^1] | ✓
//! [`CompileKind`] (host/target) | ✓ | ✓
//! __CARGO_DEFAULT_LIB_METADATA[^4] | | ✓
Expand Down Expand Up @@ -572,6 +573,8 @@ pub struct Fingerprint {
rustc: u64,
/// Sorted list of cfg features enabled.
features: String,
/// Sorted list of all the declared cfg features.
declared_features: String,
/// Hash of the `Target` struct, including the target name,
/// package-relative source path, edition, etc.
target: u64,
Expand Down Expand Up @@ -876,6 +879,7 @@ impl Fingerprint {
profile: 0,
path: 0,
features: String::new(),
declared_features: String::new(),
deps: Vec::new(),
local: Mutex::new(Vec::new()),
memoized_hash: Mutex::new(None),
Expand Down Expand Up @@ -922,6 +926,12 @@ impl Fingerprint {
new: self.features.clone(),
};
}
if self.declared_features != old.declared_features {
return DirtyReason::DeclaredFeaturesChanged {
old: old.declared_features.clone(),
new: self.declared_features.clone(),
};
}
if self.target != old.target {
return DirtyReason::TargetConfigurationChanged;
}
Expand Down Expand Up @@ -1200,6 +1210,7 @@ impl hash::Hash for Fingerprint {
let Fingerprint {
rustc,
ref features,
ref declared_features,
target,
path,
profile,
Expand All @@ -1215,6 +1226,7 @@ impl hash::Hash for Fingerprint {
(
rustc,
features,
declared_features,
target,
path,
profile,
Expand Down Expand Up @@ -1431,6 +1443,9 @@ fn calculate_normal(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Finger
allow_features.hash(&mut config);
}
let compile_kind = unit.kind.fingerprint_hash();
let mut declared_features = unit.pkg.summary().features().keys().collect::<Vec<_>>();
declared_features.sort(); // to avoid useless rebuild if the user orders it's features
// differently
Ok(Fingerprint {
rustc: util::hash_u64(&cx.bcx.rustc().verbose_version),
target: util::hash_u64(&unit.target),
Expand All @@ -1439,6 +1454,14 @@ fn calculate_normal(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult<Finger
// actually affect the output artifact so there's no need to hash it.
path: util::hash_u64(path_args(cx.bcx.ws, unit).0),
features: format!("{:?}", unit.features),
// Note we curently only populate `declared_features` when `-Zcheck-cfg`
// is passed since it's the only user-facing toggle that will make this
// fingerprint relevant.
declared_features: if cx.bcx.config.cli_unstable().check_cfg {
format!("{declared_features:?}")
} else {
"".to_string()
},
deps,
local: Mutex::new(local),
memoized_hash: Mutex::new(None),
Expand Down
71 changes: 71 additions & 0 deletions tests/testsuite/check_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,77 @@ fn features_with_namespaced_features() {
.run();
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"

[features]
f_a = []
f_b = []
"#,
)
.file("src/lib.rs", "#[cfg(feature = \"f_b\")] fn entry() {}")
.build();

p.cargo("check -v -Zcheck-cfg")
.masquerade_as_nightly_cargo(&["check-cfg"])
.with_stderr_contains(x!("rustc" => "cfg" of "feature" with "f_a" "f_b"))
.with_stderr_does_not_contain("[..]unexpected_cfgs[..]")
.run();

p.cargo("check -v -Zcheck-cfg")
.masquerade_as_nightly_cargo(&["check-cfg"])
.with_stderr_does_not_contain("[..]rustc[..]")
.run();

// checking that re-ordering the features does not invalid the fingerprint
p.change_file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"

[features]
f_b = []
f_a = []
"#,
);

p.cargo("check -v -Zcheck-cfg")
.masquerade_as_nightly_cargo(&["check-cfg"])
.with_stderr_does_not_contain("[..]rustc[..]")
.run();

p.change_file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.1.0"

[features]
f_a = []
"#,
);

p.cargo("check -v -Zcheck-cfg")
.masquerade_as_nightly_cargo(&["check-cfg"])
// we check that the fingerprint is indeed dirty
.with_stderr_contains("[..]Dirty[..]the list of declared features changed")
// that is cause rustc to be called again with the new check-cfg args
.with_stderr_contains(x!("rustc" => "cfg" of "feature" with "f_a"))
// and that we indeed found a new warning from the unexpected_cfgs lint
.with_stderr_contains("[..]unexpected_cfgs[..]")
.run();
}

#[cargo_test(nightly, reason = "--check-cfg is unstable")]
fn well_known_names_values() {
let p = project()
Expand Down