From 90c7acbd8e6eb1ddaf8680de6a1ef9f2417d50ce Mon Sep 17 00:00:00 2001 From: nsfisis Date: Fri, 26 Jun 2026 00:38:27 +0900 Subject: feat(dependency-resolver): wire operation get_package and unblock solver tests Override OperationInterface::get_package for Install/Uninstall/MarkAlias* operations so trait-object dispatch no longer hits the default todo!(), and implement Rule::is_caused_by_lock's locked-repository lookup via Request::get_locked_repository (LockArrayRepository::get_packages is infallible). Change PackageInterface::get_type to return String (matching PHP getType(): string) so AliasPackage can delegate live to its aliasOf handle across the RefCell instead of an impossible &str borrow. Un-ignores 25 SolverTest cases; 2 alias cases stay ignored pending a real solver alias-resolution discrepancy. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../operation/install_operation.rs | 4 +++ .../operation/mark_alias_installed_operation.rs | 4 +++ .../operation/mark_alias_uninstalled_operation.rs | 4 +++ .../operation/uninstall_operation.rs | 4 +++ crates/shirabe/src/dependency_resolver/rule.rs | 23 ++++++++--------- crates/shirabe/src/package/alias_package.rs | 7 ++---- crates/shirabe/src/package/complete_package.rs | 2 +- crates/shirabe/src/package/handle.rs | 2 +- crates/shirabe/src/package/package.rs | 7 ++---- crates/shirabe/src/package/package_interface.rs | 2 +- crates/shirabe/src/package/root_package.rs | 2 +- .../tests/dependency_resolver/solver_test.rs | 29 ++-------------------- 12 files changed, 37 insertions(+), 53 deletions(-) diff --git a/crates/shirabe/src/dependency_resolver/operation/install_operation.rs b/crates/shirabe/src/dependency_resolver/operation/install_operation.rs index ca3286c..ef49239 100644 --- a/crates/shirabe/src/dependency_resolver/operation/install_operation.rs +++ b/crates/shirabe/src/dependency_resolver/operation/install_operation.rs @@ -48,6 +48,10 @@ impl OperationInterface for InstallOperation { fn as_install_operation(&self) -> Option<&InstallOperation> { Some(self) } + + fn get_package(&self) -> PackageInterfaceHandle { + self.package.clone() + } } impl std::fmt::Display for InstallOperation { diff --git a/crates/shirabe/src/dependency_resolver/operation/mark_alias_installed_operation.rs b/crates/shirabe/src/dependency_resolver/operation/mark_alias_installed_operation.rs index ab07b6a..28bf511 100644 --- a/crates/shirabe/src/dependency_resolver/operation/mark_alias_installed_operation.rs +++ b/crates/shirabe/src/dependency_resolver/operation/mark_alias_installed_operation.rs @@ -44,6 +44,10 @@ impl OperationInterface for MarkAliasInstalledOperation { .get_full_pretty_version(true, crate::package::DisplayMode::SourceRefIfDev), ) } + + fn get_package(&self) -> crate::package::PackageInterfaceHandle { + self.package.clone().into() + } } impl std::fmt::Display for MarkAliasInstalledOperation { diff --git a/crates/shirabe/src/dependency_resolver/operation/mark_alias_uninstalled_operation.rs b/crates/shirabe/src/dependency_resolver/operation/mark_alias_uninstalled_operation.rs index 1b107f0..3c0de33 100644 --- a/crates/shirabe/src/dependency_resolver/operation/mark_alias_uninstalled_operation.rs +++ b/crates/shirabe/src/dependency_resolver/operation/mark_alias_uninstalled_operation.rs @@ -44,6 +44,10 @@ impl OperationInterface for MarkAliasUninstalledOperation { .get_full_pretty_version(true, crate::package::DisplayMode::SourceRefIfDev), ) } + + fn get_package(&self) -> crate::package::PackageInterfaceHandle { + self.package.clone().into() + } } impl std::fmt::Display for MarkAliasUninstalledOperation { diff --git a/crates/shirabe/src/dependency_resolver/operation/uninstall_operation.rs b/crates/shirabe/src/dependency_resolver/operation/uninstall_operation.rs index efb3610..d2dd665 100644 --- a/crates/shirabe/src/dependency_resolver/operation/uninstall_operation.rs +++ b/crates/shirabe/src/dependency_resolver/operation/uninstall_operation.rs @@ -47,6 +47,10 @@ impl OperationInterface for UninstallOperation { fn as_uninstall_operation(&self) -> Option<&UninstallOperation> { Some(self) } + + fn get_package(&self) -> PackageInterfaceHandle { + self.package.clone() + } } impl std::fmt::Display for UninstallOperation { diff --git a/crates/shirabe/src/dependency_resolver/rule.rs b/crates/shirabe/src/dependency_resolver/rule.rs index 7686a3e..ef39513 100644 --- a/crates/shirabe/src/dependency_resolver/rule.rs +++ b/crates/shirabe/src/dependency_resolver/rule.rs @@ -23,6 +23,7 @@ use crate::dependency_resolver::RuleSet; use crate::package::AliasPackage; use crate::package::BasePackage; use crate::package::BasePackageHandle; +use crate::repository::RepositoryInterface; use crate::package::Link; use crate::package::PackageInterface; use crate::package::version::VersionParser; @@ -214,12 +215,11 @@ impl Rule { if PlatformRepository::is_platform_package(link.get_target()) { return false; } - // TODO(phase-c): request.get_locked_repository() exists, but its get_packages() - // returns Result while is_caused_by_lock returns bool; resolving needs the bool - // chain (also via Problem/SolverProblemsException, itself phase-c) to carry Result. - let locked_repo: Option<()> = todo!("request.get_locked_repository()"); - if let Some(_locked_repo) = locked_repo { - let packages: Vec = todo!("locked_repo.get_packages()"); + if let Some(locked_repo) = request.get_locked_repository() { + let packages = locked_repo + .borrow_mut() + .get_packages() + .expect("LockArrayRepository::get_packages() never fails"); for package in packages { let p = package.clone(); if p.get_name() == link.get_target() { @@ -255,12 +255,11 @@ impl Rule { if PlatformRepository::is_platform_package(package_name) { return false; } - // TODO(phase-c): request.get_locked_repository() exists, but its get_packages() - // returns Result while is_caused_by_lock returns bool; resolving needs the bool - // chain (also via Problem/SolverProblemsException, itself phase-c) to carry Result. - let locked_repo: Option<()> = todo!("request.get_locked_repository()"); - if let Some(_locked_repo) = locked_repo { - let packages: Vec = todo!("locked_repo.get_packages()"); + if let Some(locked_repo) = request.get_locked_repository() { + let packages = locked_repo + .borrow_mut() + .get_packages() + .expect("LockArrayRepository::get_packages() never fails"); for package in packages { let p = package.clone(); if p.get_name() == *package_name { diff --git a/crates/shirabe/src/package/alias_package.rs b/crates/shirabe/src/package/alias_package.rs index c0e71ec..1cc899d 100644 --- a/crates/shirabe/src/package/alias_package.rs +++ b/crates/shirabe/src/package/alias_package.rs @@ -270,11 +270,8 @@ impl PackageInterface for AliasPackage { self.dev_requires.clone() } - fn get_type(&self) -> &str { - // Delegates to the shared `aliasOf` handle, whose getters yield owned - // `String`s; a borrow cannot escape the `RefCell`. Use the handle API - // (`AliasPackageHandle::get_alias_of().get_type()`) instead. - todo!("AliasPackage::get_type cannot return &str across the aliasOf handle") + fn get_type(&self) -> String { + self.alias_of.get_type() } fn get_target_dir(&self) -> Option { diff --git a/crates/shirabe/src/package/complete_package.rs b/crates/shirabe/src/package/complete_package.rs index 56d50d5..d19b041 100644 --- a/crates/shirabe/src/package/complete_package.rs +++ b/crates/shirabe/src/package/complete_package.rs @@ -187,7 +187,7 @@ impl PackageInterface for CompletePackage { self.inner.is_dev() } - fn get_type(&self) -> &str { + fn get_type(&self) -> String { PackageInterface::get_type(&self.inner) } diff --git a/crates/shirabe/src/package/handle.rs b/crates/shirabe/src/package/handle.rs index 83a5d47..e75a1a0 100644 --- a/crates/shirabe/src/package/handle.rs +++ b/crates/shirabe/src/package/handle.rs @@ -201,7 +201,7 @@ macro_rules! delegate_package_interface_to_inner { fn is_dev(&self) -> bool { self.$field.is_dev() } - fn get_type(&self) -> &str { + fn get_type(&self) -> String { self.$field.get_type() } fn get_target_dir(&self) -> Option { diff --git a/crates/shirabe/src/package/package.rs b/crates/shirabe/src/package/package.rs index e2f1f1a..a0497b2 100644 --- a/crates/shirabe/src/package/package.rs +++ b/crates/shirabe/src/package/package.rs @@ -611,11 +611,8 @@ impl PackageInterface for Package { fn is_dev(&self) -> bool { self.dev } - fn get_type(&self) -> &str { - self.r#type - .as_deref() - .filter(|s| !s.is_empty()) - .unwrap_or("library") + fn get_type(&self) -> String { + Package::get_type(self) } fn get_target_dir(&self) -> Option { Package::get_target_dir(self) diff --git a/crates/shirabe/src/package/package_interface.rs b/crates/shirabe/src/package/package_interface.rs index 285a30b..621c73f 100644 --- a/crates/shirabe/src/package/package_interface.rs +++ b/crates/shirabe/src/package/package_interface.rs @@ -62,7 +62,7 @@ pub trait PackageInterface: std::fmt::Display + std::fmt::Debug { /// Returns the package type, e.g. library /// /// @return string The package type - fn get_type(&self) -> &str; + fn get_type(&self) -> String; /// Returns the package targetDir property /// diff --git a/crates/shirabe/src/package/root_package.rs b/crates/shirabe/src/package/root_package.rs index 9cdc6e2..355f1f6 100644 --- a/crates/shirabe/src/package/root_package.rs +++ b/crates/shirabe/src/package/root_package.rs @@ -266,7 +266,7 @@ impl PackageInterface for RootPackage { fn is_dev(&self) -> bool { self.inner.is_dev() } - fn get_type(&self) -> &str { + fn get_type(&self) -> String { self.inner.get_type() } fn get_target_dir(&self) -> Option { diff --git a/crates/shirabe/tests/dependency_resolver/solver_test.rs b/crates/shirabe/tests/dependency_resolver/solver_test.rs index aad6bc2..4f24003 100644 --- a/crates/shirabe/tests/dependency_resolver/solver_test.rs +++ b/crates/shirabe/tests/dependency_resolver/solver_test.rs @@ -236,7 +236,6 @@ fn solve_expecting_problems( } } -#[ignore] #[test] fn test_solver_install_single() { let fixtures = set_up(); @@ -258,7 +257,6 @@ fn test_solver_install_single() { ); } -#[ignore] #[test] fn test_solver_remove_if_not_requested() { let fixtures = set_up(); @@ -317,7 +315,6 @@ fn test_install_non_existing_package_fails() { ); } -#[ignore] #[test] fn test_solver_install_same_package_from_different_repositories() { let fixtures = set_up(); @@ -354,7 +351,6 @@ fn test_solver_install_same_package_from_different_repositories() { ); } -#[ignore] #[test] fn test_solver_install_with_deps() { let fixtures = set_up(); @@ -399,7 +395,6 @@ fn test_solver_install_with_deps() { ); } -#[ignore] #[test] fn test_solver_install_honours_not_equal_operator() { let fixtures = set_up(); @@ -452,7 +447,6 @@ fn test_solver_install_honours_not_equal_operator() { ); } -#[ignore] #[test] fn test_solver_install_with_deps_in_order() { let fixtures = set_up(); @@ -526,7 +520,6 @@ fn test_solver_install_with_deps_in_order() { ); } -#[ignore] #[test] fn test_solver_multi_package_name_version_resolution_depends_on_require_order() { let fixtures = set_up(); @@ -619,7 +612,6 @@ fn test_solver_multi_package_name_version_resolution_depends_on_require_order() ); } -#[ignore] #[test] fn test_solver_multi_package_name_version_resolution_is_independent_of_require_order_if_ordered_descending_by_requirement() { @@ -974,7 +966,6 @@ fn test_solver_update_fully_constrained() { ); } -#[ignore] #[test] fn test_solver_update_fully_constrained_prunes_installed_packages() { let fixtures = set_up(); @@ -1009,7 +1000,6 @@ fn test_solver_update_fully_constrained_prunes_installed_packages() { ); } -#[ignore] #[test] fn test_solver_all_jobs() { let fixtures = set_up(); @@ -1073,7 +1063,6 @@ fn test_solver_all_jobs() { ); } -#[ignore] #[test] fn test_solver_three_alternative_require_and_conflict() { let fixtures = set_up(); @@ -1131,7 +1120,6 @@ fn test_solver_three_alternative_require_and_conflict() { ); } -#[ignore] #[test] fn test_solver_obsolete() { let fixtures = set_up(); @@ -1173,7 +1161,6 @@ fn test_solver_obsolete() { ); } -#[ignore] #[test] fn test_install_one_of_two_alternatives() { let fixtures = set_up(); @@ -1241,7 +1228,6 @@ fn test_install_provider() { ); } -#[ignore] #[test] fn test_skip_replacer_of_existing_package() { let fixtures = set_up(); @@ -1340,7 +1326,6 @@ fn test_no_install_replacer_of_missing_package() { ); } -#[ignore] #[test] fn test_skip_replaced_package_if_replacer_is_selected() { let fixtures = set_up(); @@ -1397,7 +1382,6 @@ fn test_skip_replaced_package_if_replacer_is_selected() { ); } -#[ignore] #[test] fn test_pick_older_if_newer_conflicts() { let fixtures = set_up(); @@ -1516,7 +1500,6 @@ fn test_pick_older_if_newer_conflicts() { ); } -#[ignore] #[test] fn test_install_circular_require() { let fixtures = set_up(); @@ -1572,7 +1555,6 @@ fn test_install_circular_require() { ); } -#[ignore] #[test] fn test_install_alternative_with_circular_require() { let fixtures = set_up(); @@ -1684,7 +1666,6 @@ fn test_install_alternative_with_circular_require() { ); } -#[ignore] #[test] fn test_use_replacer_if_necessary() { let fixtures = set_up(); @@ -1908,7 +1889,6 @@ fn test_issue265() { ); } -#[ignore = "get_pretty_string path reaches an unimplemented stub (in_array non-strict in php-shim)"] #[test] fn test_conflict_result_empty() { let fixtures = set_up(); @@ -1966,7 +1946,6 @@ fn test_conflict_result_empty() { ); } -#[ignore = "get_pretty_string path reaches an unimplemented stub (in_array non-strict in php-shim)"] #[test] fn test_unsatisfiable_requires() { let fixtures = set_up(); @@ -2014,7 +1993,6 @@ fn test_unsatisfiable_requires() { ); } -#[ignore = "get_pretty_string path reaches an unimplemented stub (Intervals::is_subset_of)"] #[test] fn test_require_mismatch_exception() { let fixtures = set_up(); @@ -2108,7 +2086,6 @@ fn test_require_mismatch_exception() { ); } -#[ignore] #[test] fn test_learn_literals_with_sorted_rule_literals() { let fixtures = set_up(); @@ -2174,7 +2151,7 @@ fn test_learn_literals_with_sorted_rule_literals() { ); } -#[ignore] +#[ignore = "solver emits fewer operations than PHP for recursive aliasOf deps: expects install b + install a + markAliasInstalled a, but only install a is produced; real solver alias-resolution discrepancy, not a stub"] #[test] fn test_install_recursive_alias_dependencies() { let fixtures = set_up(); @@ -2241,7 +2218,6 @@ fn test_install_recursive_alias_dependencies() { ); } -#[ignore] #[test] fn test_install_dev_alias() { let fixtures = set_up(); @@ -2294,7 +2270,7 @@ fn test_install_dev_alias() { ); } -#[ignore] +#[ignore = "solver omits the leading markAliasInstalled for an already-installed package's root alias (expects markAliasInstalled a after install a); real solver alias-resolution discrepancy, not a stub"] #[test] fn test_install_root_aliases_if_alias_of_is_installed() { let fixtures = set_up(); @@ -2368,7 +2344,6 @@ fn test_install_root_aliases_if_alias_of_is_installed() { ); } -#[ignore] #[test] fn test_learn_positive_literal() { let fixtures = set_up(); -- cgit v1.3.1