From 976bc9ba0eb7e86694a1d44f54d57820be1d46a5 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Mon, 20 May 2024 13:11:07 -0400 Subject: [PATCH] uv-resolver: make PubGrubPackage orderable It turns out that we use PubGrubPackage as the key in hashmaps in a fair few places. And when we iterate over hashmaps, the order is unspecified. This can in turn result in changes in output as a result of changes in the PubGrubPackage definition, purely as a function of its changing hash. This is confusing as there should be no semantic difference. Thus, this is a precursor to introducing some more determinism to places I found in the error reporting whose output depending on hashmap iteration order. --- crates/distribution-types/src/parsed_url.rs | 10 +++++----- crates/pep508-rs/src/verbatim_url.rs | 13 ++++++++++++- crates/uv-git/src/git.rs | 2 +- crates/uv-git/src/lib.rs | 2 +- crates/uv-resolver/src/pubgrub/package.rs | 4 ++-- 5 files changed, 21 insertions(+), 10 deletions(-) diff --git a/crates/distribution-types/src/parsed_url.rs b/crates/distribution-types/src/parsed_url.rs index d1c39d74a..a34401099 100644 --- a/crates/distribution-types/src/parsed_url.rs +++ b/crates/distribution-types/src/parsed_url.rs @@ -23,7 +23,7 @@ pub enum ParsedUrlError { UrlParse(String, #[source] url::ParseError), } -#[derive(Debug, Clone, Hash, PartialEq, Eq)] +#[derive(Debug, Clone, Hash, PartialEq, PartialOrd, Eq, Ord)] pub struct VerbatimParsedUrl { pub parsed_url: ParsedUrl, pub verbatim: VerbatimUrl, @@ -36,7 +36,7 @@ pub struct VerbatimParsedUrl { /// * A remote archive (`https://`), optional with a subdirectory (source dist only). /// /// A URL in a requirement `foo @ ` must be one of the above. -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] pub enum ParsedUrl { /// The direct URL is a path to a local directory or file. Path(ParsedPathUrl), @@ -51,7 +51,7 @@ pub enum ParsedUrl { /// /// Examples: /// * `file:///home/ferris/my_project` -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] pub struct ParsedPathUrl { pub url: Url, pub path: PathBuf, @@ -63,7 +63,7 @@ pub struct ParsedPathUrl { /// Examples: /// * `git+https://git.example.com/MyProject.git` /// * `git+https://git.example.com/MyProject.git@v1.0#egg=pkg&subdirectory=pkg_dir` -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] pub struct ParsedGitUrl { pub url: GitUrl, pub subdirectory: Option, @@ -96,7 +96,7 @@ impl TryFrom for ParsedGitUrl { /// * A built distribution: `https://files.pythonhosted.org/packages/62/06/d5604a70d160f6a6ca5fd2ba25597c24abd5c5ca5f437263d177ac242308/tqdm-4.66.1-py2.py3-none-any.whl` /// * A source distribution with a valid name: `https://files.pythonhosted.org/packages/62/06/d5604a70d160f6a6ca5fd2ba25597c24abd5c5ca5f437263d177ac242308/tqdm-4.66.1.tar.gz` /// * A source dist with a recognizable extension but invalid name: `https://github.com/foo-labs/foo/archive/master.zip#egg=pkg&subdirectory=packages/bar` -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord)] pub struct ParsedArchiveUrl { pub url: Url, pub subdirectory: Option, diff --git a/crates/pep508-rs/src/verbatim_url.rs b/crates/pep508-rs/src/verbatim_url.rs index af8b9df76..6c61e0ff3 100644 --- a/crates/pep508-rs/src/verbatim_url.rs +++ b/crates/pep508-rs/src/verbatim_url.rs @@ -14,7 +14,7 @@ use crate::Pep508Url; /// A wrapper around [`Url`] that preserves the original string. #[derive(Debug, Clone, Eq, derivative::Derivative, serde::Deserialize, serde::Serialize)] -#[derivative(PartialEq, Hash)] +#[derivative(PartialEq, Hash, Ord)] pub struct VerbatimUrl { /// The parsed URL. #[serde( @@ -24,6 +24,7 @@ pub struct VerbatimUrl { url: Url, /// The URL as it was provided by the user. #[derivative(PartialEq = "ignore")] + #[derivative(Ord = "ignore")] #[derivative(Hash = "ignore")] given: Option, } @@ -163,6 +164,16 @@ impl VerbatimUrl { } } +// This impl is written out because the `derive` doesn't seem to get it right. +// Or does it in a way where Clippy complains about non-canonical `PartialOrd` +// impls. So we just do it by hand by deferring to the derived `Ord` impl. This +// guarantees they are consistent. +impl PartialOrd for VerbatimUrl { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + impl std::str::FromStr for VerbatimUrl { type Err = VerbatimUrlError; diff --git a/crates/uv-git/src/git.rs b/crates/uv-git/src/git.rs index 5bc4b8de8..3fb01ad00 100644 --- a/crates/uv-git/src/git.rs +++ b/crates/uv-git/src/git.rs @@ -23,7 +23,7 @@ use crate::FetchStrategy; const CHECKOUT_READY_LOCK: &str = ".ok"; /// A reference to commit or commit-ish. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum GitReference { /// A specific branch. Branch(String), diff --git a/crates/uv-git/src/lib.rs b/crates/uv-git/src/lib.rs index bc7f8d9dc..c2bc4aa0b 100644 --- a/crates/uv-git/src/lib.rs +++ b/crates/uv-git/src/lib.rs @@ -12,7 +12,7 @@ mod source; mod util; /// A URL reference to a Git repository. -#[derive(Debug, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Hash, Ord)] pub struct GitUrl { /// The URL of the Git repository, with any query parameters, fragments, and leading `git+` /// removed. diff --git a/crates/uv-resolver/src/pubgrub/package.rs b/crates/uv-resolver/src/pubgrub/package.rs index c82add503..2810bba19 100644 --- a/crates/uv-resolver/src/pubgrub/package.rs +++ b/crates/uv-resolver/src/pubgrub/package.rs @@ -10,7 +10,7 @@ use crate::resolver::Urls; /// 2. Uses the same strategy as pip and posy to handle extras: for each extra, we create a virtual /// package (e.g., `black[colorama]`), and mark it as a dependency of the real package (e.g., /// `black`). We then discard the virtual packages at the end of the resolution process. -#[derive(Debug, Clone, Eq, Hash, PartialEq)] +#[derive(Debug, Clone, Eq, Hash, PartialEq, PartialOrd, Ord)] pub enum PubGrubPackage { /// The root package, which is used to start the resolution process. Root(Option), @@ -90,7 +90,7 @@ impl PubGrubPackage { } } -#[derive(Debug, Clone, Eq, PartialEq, Hash)] +#[derive(Debug, Clone, Eq, PartialEq, PartialOrd, Hash, Ord)] pub enum PubGrubPython { /// The Python version installed in the current environment. Installed,