Skip to content

Commit 90d6dc6

Browse files
committed
Make HashedConfigurationPlatform equality consistent with Hash
HashedConfigurationPlatform had a manual Hash impl using only output_hash, but derived PartialEq/Eq/Ord that compared all fields including is_exec_platform (via ConfigurationPlatform). This inconsistency caused the static interner to create separate entries for the same platform used in exec vs non-exec context. Fix: implement PartialEq/Eq/Ord/PartialOrd manually using only output_hash, consistent with the existing Hash and StrongHash impls.
1 parent 410acbe commit 90d6dc6

1 file changed

Lines changed: 72 additions & 12 deletions

File tree

  • app/buck2_core/src/configuration

app/buck2_core/src/configuration/data.rs

Lines changed: 72 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -422,16 +422,7 @@ impl ConfigurationDataData {
422422
}
423423
}
424424

425-
#[derive(
426-
Debug,
427-
Eq,
428-
PartialEq,
429-
Ord,
430-
PartialOrd,
431-
Allocative,
432-
derive_more::Display,
433-
Pagable
434-
)]
425+
#[derive(Debug, Allocative, derive_more::Display, Pagable)]
435426
#[display("{}", full_name)]
436427
pub(crate) struct HashedConfigurationPlatform {
437428
configuration_platform: ConfigurationPlatform,
@@ -442,14 +433,35 @@ pub(crate) struct HashedConfigurationPlatform {
442433
output_hash: ConfigurationHash,
443434
}
444435

445-
/// This will hash just the "output_hash" which should uniquely identify this data.
446-
#[allow(clippy::derived_hash_with_manual_eq)] // The derived PartialEq is still correct.
436+
/// Hash, equality, and ordering use only `output_hash` so that they are
437+
/// always consistent with each other. Whether `is_exec_platform` is folded
438+
/// into `output_hash` depends on `HASH_CFG_WITH_EXEC_PLATFORM`.
447439
impl std::hash::Hash for HashedConfigurationPlatform {
448440
fn hash<H: Hasher>(&self, state: &mut H) {
449441
self.output_hash.hash(state)
450442
}
451443
}
452444

445+
impl PartialEq for HashedConfigurationPlatform {
446+
fn eq(&self, other: &Self) -> bool {
447+
self.output_hash == other.output_hash
448+
}
449+
}
450+
451+
impl Eq for HashedConfigurationPlatform {}
452+
453+
impl PartialOrd for HashedConfigurationPlatform {
454+
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
455+
Some(self.cmp(other))
456+
}
457+
}
458+
459+
impl Ord for HashedConfigurationPlatform {
460+
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
461+
self.output_hash.cmp(&other.output_hash)
462+
}
463+
}
464+
453465
impl StrongHash for HashedConfigurationPlatform {
454466
fn strong_hash<H: Hasher>(&self, state: &mut H) {
455467
// This is already a strong hash (computed a few lines below).
@@ -565,4 +577,52 @@ mod tests {
565577
.unwrap();
566578
assert_eq!(configuration, looked_up);
567579
}
580+
581+
/// With `HASH_CFG_WITH_EXEC_PLATFORM` unset (the default), configs that
582+
/// differ only in `is_exec_platform` must be equal with matching hashes.
583+
#[test]
584+
fn test_exec_vs_non_exec_hash_eq_consistency() {
585+
use std::collections::hash_map::DefaultHasher;
586+
use std::hash::Hash;
587+
use std::hash::Hasher;
588+
589+
fn compute_hash(cfg: &ConfigurationData) -> u64 {
590+
let mut hasher = DefaultHasher::new();
591+
cfg.hash(&mut hasher);
592+
hasher.finish()
593+
}
594+
595+
let constraints = BTreeMap::from_iter([(
596+
ConstraintKey::testing_new("foo//bar:c"),
597+
ConstraintValue::testing_new("foo//bar:v", None),
598+
)]);
599+
600+
let cfg_non_exec = ConfigurationData::from_platform(
601+
"my//platform:cfg".to_owned(),
602+
ConfigurationDataData {
603+
constraints: constraints.clone(),
604+
},
605+
false,
606+
)
607+
.unwrap();
608+
609+
let cfg_exec = ConfigurationData::from_platform(
610+
"my//platform:cfg".to_owned(),
611+
ConfigurationDataData {
612+
constraints: constraints.clone(),
613+
},
614+
true,
615+
)
616+
.unwrap();
617+
618+
assert_eq!(
619+
cfg_non_exec, cfg_exec,
620+
"Configs differing only in is_exec_platform must be equal"
621+
);
622+
assert_eq!(
623+
compute_hash(&cfg_non_exec),
624+
compute_hash(&cfg_exec),
625+
"Equal configs must have equal hashes"
626+
);
627+
}
568628
}

0 commit comments

Comments
 (0)