From 62692b4e1bdd3250a72cde9200c30610653de385 Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Sat, 10 May 2025 20:00:27 +0200 Subject: [PATCH] Fix detection of sorted dependencies when include-group is used (#13354) This follows on from #13334 to fix another case. ## Summary If a dependency group contained any `{ include-group = "..." }` entries, the sort detection would bail out. The root cause of the problem was gating the sort detection behind `deps.iter().all(Value::is_str)`. A public code search reveals that keeping include-groups at the top is by far the most common, but keeping them at the bottom isn't uncommon. In both of these cases, uv will now preserve the convention that is in use. Unless I've missed it, I don't think uv supports `uv add`ing an include-group, and so that wasn't tested here. ## Test Plan cargo test --------- Co-authored-by: Charlie Marsh --- crates/uv-workspace/src/pyproject_mut.rs | 125 ++++++--------- crates/uv/tests/it/edit.rs | 195 +++++++++++++++++++++++ 2 files changed, 247 insertions(+), 73 deletions(-) diff --git a/crates/uv-workspace/src/pyproject_mut.rs b/crates/uv-workspace/src/pyproject_mut.rs index 3a76620d4..63863e6a0 100644 --- a/crates/uv-workspace/src/pyproject_mut.rs +++ b/crates/uv-workspace/src/pyproject_mut.rs @@ -1,4 +1,3 @@ -use std::cmp::Ordering; use std::path::Path; use std::str::FromStr; use std::{fmt, mem}; @@ -1019,96 +1018,76 @@ pub fn add_dependency( Unsorted, } - /// Compare two [`Value`] requirements case-insensitively. - fn case_insensitive(a: &Value, b: &Value) -> Ordering { - a.as_str() - .map(str::to_lowercase) - .as_deref() - .map(split_specifiers) - .cmp( - &b.as_str() - .map(str::to_lowercase) - .as_deref() - .map(split_specifiers), - ) + fn is_sorted(items: I) -> bool + where + I: IntoIterator, + T: PartialOrd + Copy, + { + items.into_iter().tuple_windows().all(|(a, b)| a <= b) } - /// Naively compare two [`Value`] requirements case-insensitively. - fn case_insensitive_naive(a: &Value, b: &Value) -> Ordering { - a.as_str() - .map(str::to_lowercase) - .cmp(&b.as_str().map(str::to_lowercase)) - } - - /// Compare two [`Value`] requirements case-sensitively. - fn case_sensitive(a: &Value, b: &Value) -> Ordering { - a.as_str() - .map(split_specifiers) - .cmp(&b.as_str().map(split_specifiers)) - } - - /// Naively compare two [`Value`] requirements case-sensitively. - fn case_sensitive_naive(a: &Value, b: &Value) -> Ordering { - a.as_str().cmp(&b.as_str()) - } + // `deps` are either requirements (strings) or include groups (inline tables). + // Here we pull out just the requirements for determining the sort. + let reqs: Vec<_> = deps.iter().filter_map(Value::as_str).collect(); + let reqs_lowercase: Vec<_> = reqs.iter().copied().map(str::to_lowercase).collect(); // Determine if the dependency list is sorted prior to // adding the new dependency; the new dependency list // will be sorted only when the original list is sorted // so that user's custom dependency ordering is preserved. // - // Additionally, if the table is invalid (i.e. contains non-string values) - // we still treat it as unsorted for the sake of simplicity. + // Any items which aren't strings are ignored, e.g. + // `{ include-group = "..." }` in dependency-groups. // // We account for both case-sensitive and case-insensitive sorting. - let sort = deps - .iter() - .all(Value::is_str) - .then(|| { - if deps.iter().tuple_windows().all(|(a, b)| { - matches!(case_insensitive(a, b), Ordering::Less | Ordering::Equal) - }) { - Some(Sort::CaseInsensitive) - } else if deps.iter().tuple_windows().all(|(a, b)| { - matches!(case_sensitive(a, b), Ordering::Less | Ordering::Equal) - }) { - Some(Sort::CaseSensitive) - } else if deps.iter().tuple_windows().all(|(a, b)| { - matches!( - case_insensitive_naive(a, b), - Ordering::Less | Ordering::Equal - ) - }) { - Some(Sort::CaseInsensitiveNaive) - } else if deps.iter().tuple_windows().all(|(a, b)| { - matches!(case_sensitive_naive(a, b), Ordering::Less | Ordering::Equal) - }) { - Some(Sort::CaseSensitiveNaive) - } else { - None - } - }) - .flatten() - .unwrap_or(Sort::Unsorted); + let sort = if is_sorted( + reqs_lowercase + .iter() + .map(String::as_str) + .map(split_specifiers), + ) { + Sort::CaseInsensitive + } else if is_sorted(reqs.iter().copied().map(split_specifiers)) { + Sort::CaseSensitive + } else if is_sorted(reqs_lowercase.iter().map(String::as_str)) { + Sort::CaseInsensitiveNaive + } else if is_sorted(reqs) { + Sort::CaseSensitiveNaive + } else { + Sort::Unsorted + }; let req_string = req.to_string(); let index = match sort { - Sort::CaseInsensitive => deps.iter().position(|d| { - case_insensitive(d, &Value::from(req_string.as_str())) == Ordering::Greater + Sort::CaseInsensitive => deps.iter().position(|dep| { + dep.as_str().is_some_and(|dep| { + split_specifiers(&dep.to_lowercase()) + > split_specifiers(&req_string.to_lowercase()) + }) }), - Sort::CaseInsensitiveNaive => deps.iter().position(|d| { - case_insensitive_naive(d, &Value::from(req_string.as_str())) - == Ordering::Greater + Sort::CaseInsensitiveNaive => deps.iter().position(|dep| { + dep.as_str() + .is_some_and(|dep| dep.to_lowercase() > req_string.to_lowercase()) }), - Sort::CaseSensitive => deps.iter().position(|d| { - case_sensitive(d, &Value::from(req_string.as_str())) == Ordering::Greater - }), - Sort::CaseSensitiveNaive => deps.iter().position(|d| { - case_sensitive_naive(d, &Value::from(req_string.as_str())) == Ordering::Greater + Sort::CaseSensitive => deps.iter().position(|dep| { + dep.as_str() + .is_some_and(|dep| split_specifiers(dep) > split_specifiers(&req_string)) }), + Sort::CaseSensitiveNaive => deps + .iter() + .position(|dep| dep.as_str().is_some_and(|dep| *dep > *req_string)), Sort::Unsorted => None, }; - let index = index.unwrap_or(deps.len()); + let index = index.unwrap_or_else(|| { + // The dependency should be added to the end, ignoring any + // `include-group` items. This preserves the order for users who + // keep their `include-groups` at the bottom. + deps.iter() + .enumerate() + .filter_map(|(i, dep)| if dep.is_str() { Some(i + 1) } else { None }) + .last() + .unwrap_or(deps.len()) + }); let mut value = Value::from(req_string.as_str()); diff --git a/crates/uv/tests/it/edit.rs b/crates/uv/tests/it/edit.rs index a2dc91df4..b5894fa59 100644 --- a/crates/uv/tests/it/edit.rs +++ b/crates/uv/tests/it/edit.rs @@ -7514,6 +7514,201 @@ fn sorted_dependencies_name_specifiers() -> Result<()> { Ok(()) } +#[test] +fn sorted_dependencies_with_include_group() -> Result<()> { + let context = TestContext::new("3.12").with_filtered_counts(); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + { include-group = "coverage" }, + "pytest-mock>=3.14", + "pytest>=8.1.1", + ] + coverage = [ + "coverage>=7.4.4", + ] + "#})?; + + uv_snapshot!(context.filters(), context.add().args(["--dev", "pytest-randomly"]), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Installed [N] packages in [TIME] + + coverage==7.4.4 + + iniconfig==2.0.0 + + packaging==24.0 + + pluggy==1.4.0 + + pytest==8.1.1 + + pytest-mock==3.14.0 + + pytest-randomly==3.15.0 + "); + + let pyproject_toml = context.read("pyproject.toml"); + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + { include-group = "coverage" }, + "pytest-mock>=3.14", + "pytest-randomly>=3.15.0", + "pytest>=8.1.1", + ] + coverage = [ + "coverage>=7.4.4", + ] + "# + ); + }); + Ok(()) +} + +#[test] +fn sorted_dependencies_new_dependency_after_include_group() -> Result<()> { + let context = TestContext::new("3.12").with_filtered_counts(); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + { include-group = "coverage" }, + ] + coverage = [ + "coverage>=7.4.4", + ] + "#})?; + + uv_snapshot!(context.filters(), context.add().args(["--dev", "pytest"]), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Installed [N] packages in [TIME] + + coverage==7.4.4 + + iniconfig==2.0.0 + + packaging==24.0 + + pluggy==1.4.0 + + pytest==8.1.1 + "); + + let pyproject_toml = context.read("pyproject.toml"); + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + { include-group = "coverage" }, + "pytest>=8.1.1", + ] + coverage = [ + "coverage>=7.4.4", + ] + "# + ); + }); + Ok(()) +} + +#[test] +fn sorted_dependencies_include_group_kept_at_bottom() -> Result<()> { + let context = TestContext::new("3.12").with_filtered_counts(); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str(indoc! {r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + "pytest>=8.1.1", + { include-group = "coverage" }, + ] + coverage = [ + "coverage>=7.4.4", + ] + "#})?; + + uv_snapshot!(context.filters(), context.add().args(["--dev", "pytest-randomly"]), @r" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + Resolved [N] packages in [TIME] + Prepared [N] packages in [TIME] + Installed [N] packages in [TIME] + + coverage==7.4.4 + + iniconfig==2.0.0 + + packaging==24.0 + + pluggy==1.4.0 + + pytest==8.1.1 + + pytest-randomly==3.15.0 + "); + + let pyproject_toml = context.read("pyproject.toml"); + + insta::with_settings!({ + filters => context.filters(), + }, { + assert_snapshot!( + pyproject_toml, @r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + + [dependency-groups] + dev = [ + "pytest>=8.1.1", + "pytest-randomly>=3.15.0", + { include-group = "coverage" }, + ] + coverage = [ + "coverage>=7.4.4", + ] + "# + ); + }); + Ok(()) +} + /// Ensure that the custom ordering of the dependencies is preserved /// after adding a package. #[test]