Skip to content

Commit 0c157e0

Browse files
authored
simplify SourceID Hash (#14800)
### What does this PR try to resolve? Despite being interned `SourceId::Eq` is not a `ptr::eq`. Which in turn is because `SourceId`s concept of identity is a complete mess. The code relies on having to IDs that are `Eq` but do not have the same values for their fields. As one measure of this `SourceId` has an `impl Hash` which does something different from `fn full_hash` and `fn stable_hash`. Separately `SourceIdInner` has a different implementation. Similar levels of complexity exist for `Eq`. Every one of these `impl`s was added due to a real bug/issue we've had that needs to stay fixed. Not all of witch are reproducible enough to have made it into our test suite. I [have some ideas](#14665 (comment)) for how to reorganize the types so that this is easier to reason about and faster. But given the history and the complexity I want to move extremely carefully. ### How should we test and review this PR? The test pass, and it's a one line change, but this still needs careful review. ### Additional information r? @ehuss I remember you and Alex working very hard to track down most of these bugs.
2 parents cfa27f2 + 9c19032 commit 0c157e0

File tree

1 file changed

+10
-9
lines changed

1 file changed

+10
-9
lines changed

src/cargo/core/source_id.rs

+10-9
Original file line numberDiff line numberDiff line change
@@ -532,7 +532,10 @@ impl SourceId {
532532
url == CRATES_IO_INDEX || url == CRATES_IO_HTTP_INDEX || is_overridden_crates_io_url(url)
533533
}
534534

535-
/// Hashes `self`.
535+
/// Hashes `self` to be used in the name of some Cargo folders, so shouldn't vary.
536+
///
537+
/// For git and url, `as_str` gives the serialisation of a url (which has a spec) and so
538+
/// insulates against possible changes in how the url crate does hashing.
536539
///
537540
/// For paths, remove the workspace prefix so the same source will give the
538541
/// same hash in different locations, helping reproducible builds.
@@ -550,7 +553,11 @@ impl SourceId {
550553
return;
551554
}
552555
}
553-
self.hash(into)
556+
self.inner.kind.hash(into);
557+
match self.inner.kind {
558+
SourceKind::Git(_) => (&self).inner.canonical_url.hash(into),
559+
_ => (&self).inner.url.as_str().hash(into),
560+
}
554561
}
555562

556563
pub fn full_eq(self, other: SourceId) -> bool {
@@ -665,16 +672,10 @@ impl fmt::Display for SourceId {
665672
}
666673
}
667674

668-
/// The hash of `SourceId` is used in the name of some Cargo folders, so shouldn't
669-
/// vary. `as_str` gives the serialisation of a url (which has a spec) and so
670-
/// insulates against possible changes in how the url crate does hashing.
671675
impl Hash for SourceId {
672676
fn hash<S: hash::Hasher>(&self, into: &mut S) {
673677
self.inner.kind.hash(into);
674-
match self.inner.kind {
675-
SourceKind::Git(_) => self.inner.canonical_url.hash(into),
676-
_ => self.inner.url.as_str().hash(into),
677-
}
678+
self.inner.canonical_url.hash(into);
678679
}
679680
}
680681

0 commit comments

Comments
 (0)