Skip to content

Commit 2a9527b

Browse files
committed
assert that this comparison is unneeded
1 parent 4945803 commit 2a9527b

File tree

1 file changed

+28
-4
lines changed

1 file changed

+28
-4
lines changed

src/cargo/core/source_id.rs

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -598,14 +598,38 @@ impl Ord for SourceId {
598598
other => return other,
599599
}
600600

601-
// If the `kind` and the `url` are equal, then for git sources we also
602-
// ensure that the canonical urls are equal.
601+
let ord = self.inner.canonical_url.cmp(&other.inner.canonical_url);
602+
603603
match (&self.inner.kind, &other.inner.kind) {
604604
(SourceKind::Git(_), SourceKind::Git(_)) => {
605-
self.inner.canonical_url.cmp(&other.inner.canonical_url)
605+
// In the pre-PR code we returned Ord here,
606+
// so there is no chance that this commit has broken anything about this match arm.
607+
}
608+
_ => {
609+
// In the pre-PR code we returned cmp of url here, so let's make sure that's the same.
610+
assert_eq!(self.inner.url.cmp(&other.inner.url), ord);
611+
// I am quite sure that this assert will never fire.
612+
// In order for it to fire either `url`s are equal but `canonical_url`s are not,
613+
// or the other way around. The algorithm for constructing a canonical URL is deterministic,
614+
// so if it's given the same URL it will return the same canonical URL.
615+
616+
// But what if we have two different URLs that canonical eyes the same?
617+
// I assert that the second one would get thrown out when the second `SourceId` was interned.
618+
// `SourceId::new` is the only way to make a `SourceId`. It allways construct them with
619+
// `precise: None`. Furthermore, it uses `SourceId::wrap` to see if it has ever constructed
620+
// a previous instance with a `SourceIdInner` that is `SourceIdInner::eq` with the one beeing added.
621+
// `SourceIdInner::eq` only looks at `kind`, `precise`, and `canonical_url`.
622+
// Proof by contradiction: If we had constructed two `SourceId` that:
623+
// 1. have the same `kind` (check that a few lines ago)
624+
// 2. have the same `precise` (as at construction time it is allways None)
625+
// 3. have the same `canonical_url` (by the assumption of this paragraph)
626+
// then the `ptr::eq` would return equal. Even if they were constructed with different `url`s,
627+
// `SourceId::wrap` would have noticed that they `SourceIdInner::eq`
628+
// and thus returned a pointer to the first one.
606629
}
607-
_ => self.inner.url.cmp(&other.inner.url),
608630
}
631+
632+
ord
609633
}
610634
}
611635

0 commit comments

Comments
 (0)