From e87236391f933bf54878e25a65d92345e7318c6d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 29 Sep 2025 09:42:21 -0400 Subject: [PATCH] Re-order lock validation checks by severity (#16045) ## Summary I don't think it's common for this to matter, but in theory at least it's important that these are ordered by severity. Otherwise, e.g, changing the pre-release mode (and then returning early) could mean we retain the forks when we otherwise shouldn't. --- crates/uv/src/commands/project/lock.rs | 80 ++++++++++++++------------ 1 file changed, 43 insertions(+), 37 deletions(-) diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index f09dab033..b8a105522 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -947,16 +947,16 @@ async fn do_lock( #[derive(Debug)] enum ValidatedLock { - /// An existing lockfile was provided, and it satisfies the workspace requirements. - Satisfies(Lock), /// An existing lockfile was provided, but its contents should be ignored. Unusable(Lock), - /// An existing lockfile was provided, and the locked versions and forks should be preferred if - /// possible, even though the lockfile does not satisfy the workspace requirements. - Preferable(Lock), /// An existing lockfile was provided, and the locked versions should be preferred if possible, /// though the forks should be ignored. Versions(Lock), + /// An existing lockfile was provided, and the locked versions and forks should be preferred if + /// possible, even though the lockfile does not satisfy the workspace requirements. + Preferable(Lock), + /// An existing lockfile was provided, and it satisfies the workspace requirements. + Satisfies(Lock), } impl ValidatedLock { @@ -987,7 +987,10 @@ impl ValidatedLock { database: &DistributionDatabase<'_, Context>, printer: Printer, ) -> Result { - // Start with the most severe condition: a fundamental option changed between resolutions. + // Perform checks in a deliberate order, such that the most extreme conditions are tested + // first (i.e., every check that returns `Self::Unusable`, followed by every check that + // returns `Self::Versions`, followed by every check that returns `Self::Preferable`, and + // finally `Self::Satisfies`). if lock.resolution_mode() != options.resolution_mode { let _ = writeln!( printer.stderr(), @@ -997,15 +1000,6 @@ impl ValidatedLock { ); return Ok(Self::Unusable(lock)); } - if lock.prerelease_mode() != options.prerelease_mode { - let _ = writeln!( - printer.stderr(), - "Resolving despite existing lockfile due to change in pre-release mode: `{}` vs. `{}`", - lock.prerelease_mode().cyan(), - options.prerelease_mode.cyan() - ); - return Ok(Self::Preferable(lock)); - } if lock.fork_strategy() != options.fork_strategy { let _ = writeln!( printer.stderr(), @@ -1100,28 +1094,6 @@ impl ValidatedLock { return Ok(Self::Versions(lock)); } - if let Upgrade::Packages(_) = upgrade { - // If the user specified `--upgrade-package`, then at best we can prefer some of - // the existing versions. - debug!("Resolving despite existing lockfile due to `--upgrade-package`"); - return Ok(Self::Preferable(lock)); - } - - // If the Requires-Python bound has changed, we have to perform a clean resolution, since - // the set of `resolution-markers` may no longer cover the entire supported Python range. - if lock.requires_python().range() != requires_python.range() { - debug!( - "Resolving despite existing lockfile due to change in Python requirement: `{}` vs. `{}`", - lock.requires_python(), - requires_python, - ); - return if lock.fork_markers().is_empty() { - Ok(Self::Preferable(lock)) - } else { - Ok(Self::Versions(lock)) - }; - } - // If the set of supported environments has changed, we have to perform a clean resolution. let expected = lock.simplified_supported_environments(); let actual = environments @@ -1166,6 +1138,40 @@ impl ValidatedLock { return Ok(Self::Versions(lock)); } + // If the Requires-Python bound has changed, we have to perform a clean resolution, since + // the set of `resolution-markers` may no longer cover the entire supported Python range. + if lock.requires_python().range() != requires_python.range() { + debug!( + "Resolving despite existing lockfile due to change in Python requirement: `{}` vs. `{}`", + lock.requires_python(), + requires_python, + ); + return if lock.fork_markers().is_empty() { + Ok(Self::Preferable(lock)) + } else { + Ok(Self::Versions(lock)) + }; + } + + // If the pre-release mode has changed, we have to re-resolve, but can retain the existing + // versions and forks. + if lock.prerelease_mode() != options.prerelease_mode { + let _ = writeln!( + printer.stderr(), + "Resolving despite existing lockfile due to change in pre-release mode: `{}` vs. `{}`", + lock.prerelease_mode().cyan(), + options.prerelease_mode.cyan() + ); + return Ok(Self::Preferable(lock)); + } + + // If the user specified `--upgrade-package`, then at best we can prefer some of + // the existing versions. + if let Upgrade::Packages(_) = upgrade { + debug!("Resolving despite existing lockfile due to `--upgrade-package`"); + return Ok(Self::Preferable(lock)); + } + // If the user specified `--refresh`, then we have to re-resolve. if matches!(refresh, Some(Refresh::All(..) | Refresh::Packages(..))) { debug!("Resolving despite existing lockfile due to `--refresh`");