Skip to content

Commit 48a80ee

Browse files
JamieMasonJamieMason
committed
fix(core): apply semver groups to highest semver calculation
Refs #314
1 parent b5d2c38 commit 48a80ee

File tree

5 files changed

+296
-6
lines changed

5 files changed

+296
-6
lines changed

‎src/dependency.rs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,12 +182,23 @@ impl Dependency {
182182
}
183183

184184
/// Get the highest (or lowest) semver specifier in this group.
185+
///
186+
/// When an instance belongs to a semver group, its preferred range is applied
187+
/// to produce an adjusted specifier before comparison. This means a semver
188+
/// group that widens a range (e.g. exact → caret) can promote that instance
189+
/// to become the highest via the range-greediness tiebreaker.
185190
pub fn get_highest_or_lowest_specifier(&self) -> Option<Rc<Specifier>> {
186191
let prefer_highest = matches!(self.variant, VersionGroupVariant::HighestSemver);
187192
let specifiers = self
188193
.get_instances()
189194
.filter(|instance| instance.descriptor.specifier.get_node_version().is_some())
190-
.map(|instance| Rc::clone(&instance.descriptor.specifier));
195+
.map(|instance| {
196+
instance
197+
.preferred_semver_range
198+
.as_ref()
199+
.and_then(|range| instance.descriptor.specifier.with_range(range))
200+
.unwrap_or_else(|| Rc::clone(&instance.descriptor.specifier))
201+
});
191202

192203
if prefer_highest {
193204
specifiers.max()

‎src/visit_packages/preferred_semver.rs‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use {
77
semver_range::SemverRange,
88
},
99
log::debug,
10+
std::rc::Rc,
1011
};
1112

1213
#[cfg(test)]
@@ -182,7 +183,12 @@ pub fn visit(dependency: &Dependency, ctx: &Context) {
182183
if !instance.descriptor.specifier.has_same_version_number_as(&highest_specifier) {
183184
debug!("{L5}differs to the highest semver version");
184185
debug!("{L6}mark as error");
185-
instance.mark_fixable(FixableInstance::DiffersToHighestOrLowestSemver, &highest_specifier);
186+
let fix_target = instance
187+
.preferred_semver_range
188+
.as_ref()
189+
.and_then(|range| highest_specifier.with_range(range))
190+
.unwrap_or_else(|| Rc::clone(&highest_specifier));
191+
instance.mark_fixable(FixableInstance::DiffersToHighestOrLowestSemver, &fix_target);
186192
return;
187193
}
188194
debug!("{L5}is the same as the highest semver version");
@@ -220,7 +226,22 @@ pub fn visit(dependency: &Dependency, ctx: &Context) {
220226
}
221227
} else {
222228
debug!("{L4}it is not in a semver group which prefers a different semver range to the highest semver version");
223-
if instance.already_equals(&highest_specifier) {
229+
if instance.must_match_preferred_semver_range() && !instance.matches_preferred_semver_range() {
230+
let preferred_semver_range = &instance.preferred_semver_range.clone().unwrap();
231+
debug!("{L5}but its actual range does not match its semver group's preferred range ({preferred_semver_range:?})");
232+
if instance.specifier_with_preferred_semver_range_will_satisfy(&highest_specifier) {
233+
debug!("{L6}the preferred semver range will satisfy the highest semver version");
234+
debug!("{L7}mark as fixable error");
235+
instance.mark_fixable(
236+
FixableInstance::SemverRangeMismatch,
237+
&instance.get_specifier_with_preferred_semver_range().unwrap(),
238+
);
239+
} else {
240+
debug!("{L6}the preferred semver range will not satisfy the highest semver version");
241+
debug!("{L7}mark as unfixable error");
242+
instance.mark_conflict(SemverGroupAndVersionConflict::MismatchConflictsWithHighestOrLowestSemver);
243+
}
244+
} else if instance.already_equals(&highest_specifier) {
224245
debug!("{L5}it is identical to the highest semver version");
225246
debug!("{L6}mark as valid");
226247
instance.mark_valid(ValidInstance::IsHighestOrLowestSemver, &highest_specifier);

‎src/visit_packages/preferred_semver_test.rs‎

Lines changed: 183 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1429,15 +1429,15 @@ mod highest_or_lowest {
14291429
overridden: None,
14301430
},
14311431
ExpectedInstance {
1432-
state: InstanceState::valid(SatisfiesHighestOrLowestSemver),
1432+
state: InstanceState::valid(IsHighestOrLowestSemver),
14331433
dependency_name: "@typescript/native-preview",
14341434
id: "@typescript/native-preview in /devDependencies of pkg-a",
14351435
actual: "7.0.0-dev.20260117.1",
14361436
expected: Some("7.0.0-dev.20260117.1"),
14371437
overridden: None,
14381438
},
14391439
ExpectedInstance {
1440-
state: InstanceState::valid(SatisfiesHighestOrLowestSemver),
1440+
state: InstanceState::valid(IsHighestOrLowestSemver),
14411441
dependency_name: "lodash",
14421442
id: "lodash in /devDependencies of pkg-a",
14431443
actual: "4.17.21",
@@ -1470,6 +1470,187 @@ mod highest_or_lowest {
14701470
},
14711471
]);
14721472
}
1473+
1474+
/// https://github.com/JamieMason/syncpack/issues/314#issuecomment-3848268945
1475+
/// When versions differ AND a semver group exists, the fix target for
1476+
/// DiffersToHighestOrLowestSemver should apply the semver group's preferred
1477+
/// range — not inherit the range from the highest specifier.
1478+
#[test]
1479+
fn differs_to_highest_semver_should_apply_semver_group_range() {
1480+
let ctx = TestBuilder::new()
1481+
.with_packages(vec![
1482+
json!({
1483+
"name": "pkg-a",
1484+
"devDependencies": {
1485+
"react": "^18.0.0"
1486+
}
1487+
}),
1488+
json!({
1489+
"name": "pkg-b",
1490+
"devDependencies": {
1491+
"react": "17.0.0"
1492+
}
1493+
}),
1494+
json!({
1495+
"name": "pkg-c",
1496+
"devDependencies": {
1497+
"react": "17.0.0"
1498+
}
1499+
}),
1500+
])
1501+
.with_semver_group(json!({
1502+
"label": "pin all ranges",
1503+
"range": "",
1504+
"packages": ["**"],
1505+
"dependencies": ["**"]
1506+
}))
1507+
.build_and_visit_packages();
1508+
1509+
expect(&ctx).to_have_instances(vec![
1510+
ExpectedInstance {
1511+
state: InstanceState::suspect(InvalidLocalVersion),
1512+
dependency_name: "pkg-a",
1513+
id: "pkg-a in /version of pkg-a",
1514+
actual: "",
1515+
expected: Some(""),
1516+
overridden: None,
1517+
},
1518+
// pkg-a has the highest version but wrong range (^ instead of pinned)
1519+
ExpectedInstance {
1520+
state: InstanceState::fixable(SemverRangeMismatch),
1521+
dependency_name: "react",
1522+
id: "react in /devDependencies of pkg-a",
1523+
actual: "^18.0.0",
1524+
expected: Some("18.0.0"),
1525+
overridden: None,
1526+
},
1527+
ExpectedInstance {
1528+
state: InstanceState::suspect(InvalidLocalVersion),
1529+
dependency_name: "pkg-b",
1530+
id: "pkg-b in /version of pkg-b",
1531+
actual: "",
1532+
expected: Some(""),
1533+
overridden: None,
1534+
},
1535+
// pkg-b has a lower version — fix should update version AND apply pinned range
1536+
// BUG: currently suggests ^18.0.0 (inherits ^ from highest)
1537+
ExpectedInstance {
1538+
state: InstanceState::fixable(DiffersToHighestOrLowestSemver),
1539+
dependency_name: "react",
1540+
id: "react in /devDependencies of pkg-b",
1541+
actual: "17.0.0",
1542+
expected: Some("18.0.0"),
1543+
overridden: None,
1544+
},
1545+
ExpectedInstance {
1546+
state: InstanceState::suspect(InvalidLocalVersion),
1547+
dependency_name: "pkg-c",
1548+
id: "pkg-c in /version of pkg-c",
1549+
actual: "",
1550+
expected: Some(""),
1551+
overridden: None,
1552+
},
1553+
// pkg-c same as pkg-b
1554+
ExpectedInstance {
1555+
state: InstanceState::fixable(DiffersToHighestOrLowestSemver),
1556+
dependency_name: "react",
1557+
id: "react in /devDependencies of pkg-c",
1558+
actual: "17.0.0",
1559+
expected: Some("18.0.0"),
1560+
overridden: None,
1561+
},
1562+
]);
1563+
}
1564+
1565+
/// Exploratory: when a semver group makes one instance greedier (e.g. exact
1566+
/// → ^), that adjusted specifier becomes the highest via the range tiebreaker.
1567+
/// Other instances (not in a semver group) should follow the new highest.
1568+
#[test]
1569+
fn semver_group_greedier_range_becomes_highest() {
1570+
let ctx = TestBuilder::new()
1571+
.with_packages(vec![
1572+
json!({
1573+
"name": "pkg-a",
1574+
"dependencies": {
1575+
"react": "1.0.0"
1576+
}
1577+
}),
1578+
json!({
1579+
"name": "pkg-b",
1580+
"dependencies": {
1581+
"react": "~1.0.0"
1582+
}
1583+
}),
1584+
json!({
1585+
"name": "pkg-c",
1586+
"dependencies": {
1587+
"react": "~1.0.0"
1588+
}
1589+
}),
1590+
])
1591+
.with_semver_group(json!({
1592+
"label": "use caret for pkg-a",
1593+
"range": "^",
1594+
"packages": ["pkg-a"]
1595+
}))
1596+
.build_and_visit_packages();
1597+
1598+
expect(&ctx).to_have_instances(vec![
1599+
ExpectedInstance {
1600+
state: InstanceState::suspect(InvalidLocalVersion),
1601+
dependency_name: "pkg-a",
1602+
id: "pkg-a in /version of pkg-a",
1603+
actual: "",
1604+
expected: Some(""),
1605+
overridden: None,
1606+
},
1607+
// pkg-a has exact 1.0.0 but semver group wants ^ → adjusted to ^1.0.0
1608+
// ^1.0.0 is greedier than ~1.0.0, making it the highest
1609+
// pkg-a's actual range (exact) doesn't match its preferred range (^)
1610+
ExpectedInstance {
1611+
state: InstanceState::fixable(SemverRangeMismatch),
1612+
dependency_name: "react",
1613+
id: "react in /dependencies of pkg-a",
1614+
actual: "1.0.0",
1615+
expected: Some("^1.0.0"),
1616+
overridden: None,
1617+
},
1618+
ExpectedInstance {
1619+
state: InstanceState::suspect(InvalidLocalVersion),
1620+
dependency_name: "pkg-b",
1621+
id: "pkg-b in /version of pkg-b",
1622+
actual: "",
1623+
expected: Some(""),
1624+
overridden: None,
1625+
},
1626+
// pkg-b has ~1.0.0, no semver group — follows adjusted highest ^1.0.0
1627+
ExpectedInstance {
1628+
state: InstanceState::fixable(DiffersToHighestOrLowestSemver),
1629+
dependency_name: "react",
1630+
id: "react in /dependencies of pkg-b",
1631+
actual: "~1.0.0",
1632+
expected: Some("^1.0.0"),
1633+
overridden: None,
1634+
},
1635+
ExpectedInstance {
1636+
state: InstanceState::suspect(InvalidLocalVersion),
1637+
dependency_name: "pkg-c",
1638+
id: "pkg-c in /version of pkg-c",
1639+
actual: "",
1640+
expected: Some(""),
1641+
overridden: None,
1642+
},
1643+
// pkg-c same as pkg-b
1644+
ExpectedInstance {
1645+
state: InstanceState::fixable(DiffersToHighestOrLowestSemver),
1646+
dependency_name: "react",
1647+
id: "react in /dependencies of pkg-c",
1648+
actual: "~1.0.0",
1649+
expected: Some("^1.0.0"),
1650+
overridden: None,
1651+
},
1652+
]);
1653+
}
14731654
}
14741655

14751656
mod non_semver {

‎src/visit_packages/snapped_to.rs‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use {
55
instance_state::{FixableInstance, SemverGroupAndVersionConflict, SuspectInstance, ValidInstance},
66
},
77
log::debug,
8+
std::rc::Rc,
89
};
910

1011
#[cfg(test)]
@@ -32,7 +33,12 @@ pub fn visit(dependency: &crate::dependency::Dependency, ctx: &Context) {
3233
if !instance.descriptor.specifier.has_same_version_number_as(&snapped_to_specifier) {
3334
debug!("{L6}differs to the target version");
3435
debug!("{L7}mark as error");
35-
instance.mark_fixable(FixableInstance::DiffersToSnapTarget, &snapped_to_specifier);
36+
let fix_target = instance
37+
.preferred_semver_range
38+
.as_ref()
39+
.and_then(|range| snapped_to_specifier.with_range(range))
40+
.unwrap_or_else(|| Rc::clone(&snapped_to_specifier));
41+
instance.mark_fixable(FixableInstance::DiffersToSnapTarget, &fix_target);
3642
return;
3743
}
3844
debug!("{L6}is the same as the target version");

‎src/visit_packages/snapped_to_test.rs‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -887,3 +887,74 @@ fn workspace_tilde_identical_to_snapped_to_target() {
887887
},
888888
]);
889889
}
890+
891+
/// Same bug as preferred_semver: when version differs from snap target AND a
892+
/// semver group exists, the fix target should apply the semver group's range
893+
/// instead of inheriting the snap target's range.
894+
#[test]
895+
fn differs_to_snap_target_should_apply_semver_group_range() {
896+
let config = test::mock::config_from_mock(json!({
897+
"semverGroups": [{
898+
"packages": ["follower"],
899+
"range": ""
900+
}],
901+
"versionGroups": [{
902+
"dependencies": ["foo"],
903+
"packages": ["follower"],
904+
"snapTo": ["leader"]
905+
}]
906+
}));
907+
let packages = test::mock::packages_from_mocks(vec![
908+
json!({
909+
"name": "leader",
910+
"dependencies": {
911+
"foo": "^2.0.0"
912+
}
913+
}),
914+
json!({
915+
"name": "follower",
916+
"dependencies": {
917+
"foo": "1.0.0"
918+
}
919+
}),
920+
]);
921+
let registry_client = None;
922+
let catalogs = None;
923+
let ctx = Context::create(config, packages, registry_client, catalogs);
924+
let ctx = visit_packages(ctx);
925+
expect(&ctx).to_have_instances(vec![
926+
ExpectedInstance {
927+
state: InstanceState::suspect(InvalidLocalVersion),
928+
dependency_name: "leader",
929+
id: "leader in /version of leader",
930+
actual: "",
931+
expected: Some(""),
932+
overridden: None,
933+
},
934+
ExpectedInstance {
935+
state: InstanceState::suspect(InvalidLocalVersion),
936+
dependency_name: "follower",
937+
id: "follower in /version of follower",
938+
actual: "",
939+
expected: Some(""),
940+
overridden: None,
941+
},
942+
ExpectedInstance {
943+
state: InstanceState::valid(IsHighestOrLowestSemver),
944+
dependency_name: "foo",
945+
id: "foo in /dependencies of leader",
946+
actual: "^2.0.0",
947+
expected: Some("^2.0.0"),
948+
overridden: None,
949+
},
950+
// BUG: currently suggests ^2.0.0 (inherits ^ from snap target)
951+
ExpectedInstance {
952+
state: InstanceState::fixable(DiffersToSnapTarget),
953+
dependency_name: "foo",
954+
id: "foo in /dependencies of follower",
955+
actual: "1.0.0",
956+
expected: Some("2.0.0"),
957+
overridden: None,
958+
},
959+
]);
960+
}

0 commit comments

Comments
 (0)