From cd25c3e193f05a5e89bca2a1c706c85fdc9c9155 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Sat, 6 Jun 2026 02:13:59 +0900 Subject: refactor(repository): make read methods fallible and take &mut self Change RepositoryInterface and WritableRepositoryInterface read methods (find_package, find_packages, get_packages, load_packages, search, get_providers, get_canonical_packages) to take &mut self and return anyhow::Result, so lazy-loading repositories such as ComposerRepository can perform fallible I/O and mutate internal state on access. Update all implementors and call sites to propagate the Result and pass mutable references. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dependency_resolver/local_repo_transaction.rs | 14 +- .../src/dependency_resolver/pool_builder.rs | 21 +-- crates/shirabe/src/dependency_resolver/problem.rs | 147 +++++++++++---------- crates/shirabe/src/dependency_resolver/request.rs | 7 +- crates/shirabe/src/dependency_resolver/rule.rs | 42 +++--- crates/shirabe/src/dependency_resolver/solver.rs | 2 +- .../solver_problems_exception.rs | 24 ++-- 7 files changed, 131 insertions(+), 126 deletions(-) (limited to 'crates/shirabe/src/dependency_resolver') diff --git a/crates/shirabe/src/dependency_resolver/local_repo_transaction.rs b/crates/shirabe/src/dependency_resolver/local_repo_transaction.rs index dc71562..5feb9eb 100644 --- a/crates/shirabe/src/dependency_resolver/local_repo_transaction.rs +++ b/crates/shirabe/src/dependency_resolver/local_repo_transaction.rs @@ -11,15 +11,15 @@ pub struct LocalRepoTransaction { impl LocalRepoTransaction { pub fn new( - locked_repository: &dyn RepositoryInterface, - local_repository: &dyn InstalledRepositoryInterface, - ) -> Self { - Self { + locked_repository: &mut dyn RepositoryInterface, + local_repository: &mut dyn InstalledRepositoryInterface, + ) -> anyhow::Result { + Ok(Self { inner: Transaction::new( - local_repository.get_packages(), - locked_repository.get_packages(), + local_repository.get_packages()?, + locked_repository.get_packages()?, ), - } + }) } pub fn get_operations( diff --git a/crates/shirabe/src/dependency_resolver/pool_builder.rs b/crates/shirabe/src/dependency_resolver/pool_builder.rs index 1bbd51e..5750450 100644 --- a/crates/shirabe/src/dependency_resolver/pool_builder.rs +++ b/crates/shirabe/src/dependency_resolver/pool_builder.rs @@ -162,10 +162,12 @@ impl PoolBuilder { .into()); } - let locked_packages = CanonicalPackagesTrait::get_packages( - &*request.get_locked_repository().unwrap().borrow(), - ); - for locked_package in locked_packages { + for locked_package in request + .get_locked_repository() + .unwrap() + .borrow_mut() + .get_canonical_packages()? + { if !self.is_update_allowed(locked_package.clone()) { // Path repo packages are never loaded from lock, to force them to always remain in sync // unless symlinking is disabled in which case we probably should rather treat them like @@ -530,7 +532,7 @@ impl PoolBuilder { .collect() }) .unwrap_or_default(), - ); + )?; let names_found = result.names_found; for name in &names_found { @@ -819,9 +821,12 @@ impl PoolBuilder { let pattern_regexp = base_package::package_name_to_regexp(pattern); // update pattern matches a locked package? => all good - for package in CanonicalPackagesTrait::get_packages( - &*request.get_locked_repository().unwrap().borrow(), - ) { + for package in request + .get_locked_repository() + .unwrap() + .borrow_mut() + .get_canonical_packages()? + { if Preg::is_match3(&pattern_regexp, &package.get_name(), None).unwrap_or(false) { continue 'outer; } diff --git a/crates/shirabe/src/dependency_resolver/problem.rs b/crates/shirabe/src/dependency_resolver/problem.rs index 7312bda..874d56e 100644 --- a/crates/shirabe/src/dependency_resolver/problem.rs +++ b/crates/shirabe/src/dependency_resolver/problem.rs @@ -110,7 +110,7 @@ impl Problem { is_verbose, &package_name, constraint, - ); + )?; return Ok(format!("\n {}", implode("", &[missing.0, missing.1]))); } } @@ -126,7 +126,7 @@ impl Problem { .cmp(&self.get_sortable_string(pool, &rule2.borrow())) }); - Ok(Self::format_deduplicated_rules( + Self::format_deduplicated_rules( &reasons, " ", repository_set, @@ -135,7 +135,7 @@ impl Problem { is_verbose, installed_map, learned_pool, - )) + ) } fn get_sortable_string(&self, pool: &Pool, rule: &Rule) -> String { @@ -208,7 +208,7 @@ impl Problem { is_verbose: bool, installed_map: &IndexMap, learned_pool: &Vec>>>, - ) -> String { + ) -> anyhow::Result { let mut messages: Vec = Vec::new(); let mut templates: IndexMap>> = IndexMap::new(); @@ -224,7 +224,7 @@ impl Problem { is_verbose, installed_map, learned_pool, - ); + )?; let mut m: IndexMap = IndexMap::new(); let matched = if in_array( PhpMixed::Int(rule_ref.get_reason()), @@ -341,11 +341,11 @@ impl Problem { } } - format!( + Ok(format!( "\n{}- {}", indent, implode(&format!("\n{}- ", indent), &result) - ) + )) } pub fn is_caused_by_lock( @@ -394,7 +394,7 @@ impl Problem { is_verbose: bool, package_name: &str, constraint: Option<&AnyConstraint>, - ) -> (String, String) { + ) -> anyhow::Result<(String, String)> { if PlatformRepository::is_platform_package(package_name) { // handle php/php-*/hhvm if stripos(package_name, "php") == Some(0) || package_name == "hhvm" { @@ -413,51 +413,51 @@ impl Problem { if defined("HHVM_VERSION") || (package_name == "hhvm" && pool.what_provides(package_name, None).len() > 0) { - return ( + return Ok(( msg, "your HHVM version does not satisfy that requirement.".to_string(), - ); + )); } if package_name == "hhvm" { - return ( + return Ok(( msg, "HHVM was not detected on this machine, make sure it is in your PATH." .to_string(), - ); + )); } if version.is_none() { - return ( + return Ok(( msg, format!( "the {} package is disabled by your platform config. Enable it again with \"composer config platform.{} --unset\".", package_name, package_name ), - ); + )); } - return ( + return Ok(( msg, format!( "your {} version ({}) does not satisfy that requirement.", package_name, version.unwrap() ), - ); + )); } // handle php extensions if stripos(package_name, "ext-") == Some(0) { if strpos(package_name, " ").is_some() { - return ( + return Ok(( "- ".to_string(), format!( "PHP extension {} should be required as {}.", package_name, str_replace(" ", "-", package_name) ), - ); + )); } let ext = substr(package_name, 4, None); @@ -476,7 +476,7 @@ impl Problem { Self::get_platform_package_version(pool, package_name, &effective_version); if version.is_none() { let providers_str_opt = - Self::get_providers_list(repository_set, package_name, 5); + Self::get_providers_list(repository_set, package_name, 5)?; let providers_str = match providers_str_opt { Some(ps) => format!( "\n\n Alternatively you can require one of these packages that provide the extension (or parts of it):\n Keep in mind that the suggestions are automated and may not be valid or safe to use\n{}", @@ -486,28 +486,28 @@ impl Problem { }; if extension_loaded(&ext) { - return ( + return Ok(( msg, format!( "the {} package is disabled by your platform config. Enable it again with \"composer config platform.{} --unset\".{}", package_name, package_name, providers_str ), - ); + )); } - return ( + return Ok(( msg, format!( "it is missing from your system. Install or enable PHP's {} extension.{}", ext, providers_str ), - ); + )); } - return ( + return Ok(( msg, format!("it has the wrong version installed ({}).", version.unwrap()), - ); + )); } // handle linked libs @@ -519,17 +519,17 @@ impl Problem { "it is missing from your system, make sure the intl extension is loaded." }; - return ( + return Ok(( format!( "- Root composer.json requires linked library {}{} but ", package_name, Self::constraint_to_text(constraint) ), error.to_string(), - ); + )); } - let providers_str_opt = Self::get_providers_list(repository_set, package_name, 5); + let providers_str_opt = Self::get_providers_list(repository_set, package_name, 5)?; let providers_str = match providers_str_opt { Some(ps) => format!( "\n\n Alternatively you can require one of these packages that provide the library (or parts of it):\n Keep in mind that the suggestions are automated and may not be valid or safe to use\n{}", @@ -538,7 +538,7 @@ impl Problem { None => String::new(), }; - return ( + return Ok(( format!( "- Root composer.json requires linked library {}{} but ", package_name, @@ -548,7 +548,7 @@ impl Problem { "it has the wrong version installed or is missing from your system, make sure to load the extension providing it.{}", providers_str ), - ); + )); } } @@ -557,14 +557,14 @@ impl Problem { if package.get_name().as_str() == package_name { locked_package = Some(package.clone()); if pool.is_unacceptable_fixed_or_locked_package(package.clone()) { - return ( + return Ok(( "- ".to_string(), format!( "{} is fixed to {} (lock file version) by a partial update but that version is rejected by your minimum-stability. Make sure you list it as an argument for the update command.", package.get_pretty_name(), package.get_pretty_version() ), - ); + )); } break; } @@ -600,9 +600,9 @@ impl Problem { .into(), ), 0, - ); + )?; if packages.len() > 0 { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -619,14 +619,15 @@ impl Problem { ), str_replace("#", "+", &c.get_pretty_string()) ), - ); + )); } } } // first check if the actual requested package is found in normal conditions // if so it must mean it is rejected by another constraint than the one given here - let packages = repository_set.find_packages(package_name, constraint.map(|c| c.clone()), 0); + let packages = + repository_set.find_packages(package_name, constraint.map(|c| c.clone()), 0)?; if packages.len() > 0 { let root_reqs = repository_set.get_root_requires(); if root_reqs.contains_key(package_name) { @@ -644,7 +645,7 @@ impl Problem { }) .collect(); if filtered.len() == 0 { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -666,7 +667,7 @@ impl Problem { }, root_reqs[package_name].get_pretty_string() ), - ); + )); } } @@ -688,7 +689,7 @@ impl Problem { }) .collect(); if filtered.len() == 0 { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", name, @@ -711,7 +712,7 @@ impl Problem { name, temp_reqs[&name].get_pretty_string() ), - ); + )); } } } @@ -736,7 +737,7 @@ impl Problem { }) .collect(); if filtered.len() == 0 { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -753,7 +754,7 @@ impl Problem { ), lp.get_pretty_version() ), - ); + )); } } @@ -766,7 +767,7 @@ impl Problem { .collect(); if non_locked_packages.len() == 0 { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -782,11 +783,11 @@ impl Problem { false ) ), - ); + )); } if pool.is_abandoned_removed_package_version(package_name, constraint) { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -802,7 +803,7 @@ impl Problem { false ) ), - ); + )); } if pool.is_security_removed_package_version(package_name, constraint) { @@ -829,7 +830,7 @@ impl Problem { }) .collect(); - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -846,10 +847,10 @@ impl Problem { ), implode("\", \"", &advisories_list) ), - ); + )); } - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -864,7 +865,7 @@ impl Problem { "it conflicts" } ), - ); + )); } // check if the package is found when bypassing stability checks @@ -872,16 +873,16 @@ impl Problem { package_name, constraint.map(|c| c.clone()), RepositorySet::ALLOW_UNACCEPTABLE_STABILITIES, - ); + )?; if packages.len() > 0 { // we must first verify if a valid package would be found in a lower priority repository let all_repos_packages = repository_set.find_packages( package_name, constraint.map(|c| c.clone()), RepositorySet::ALLOW_SHADOWED_REPOSITORIES, - ); + )?; if all_repos_packages.len() > 0 { - return Self::compute_check_for_lower_prio_repo( + return Ok(Self::compute_check_for_lower_prio_repo( pool, is_verbose, package_name, @@ -889,10 +890,10 @@ impl Problem { &all_repos_packages, "minimum-stability", constraint, - ); + )); } - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -907,7 +908,7 @@ impl Problem { "it does" } ), - ); + )); } // check if the package is found when bypassing the constraint and stability checks @@ -915,16 +916,16 @@ impl Problem { package_name, None, RepositorySet::ALLOW_UNACCEPTABLE_STABILITIES, - ); + )?; if packages.len() > 0 { // we must first verify if a valid package would be found in a lower priority repository let all_repos_packages = repository_set.find_packages( package_name, constraint.map(|c| c.clone()), RepositorySet::ALLOW_SHADOWED_REPOSITORIES, - ); + )?; if all_repos_packages.len() > 0 { - return Self::compute_check_for_lower_prio_repo( + return Ok(Self::compute_check_for_lower_prio_repo( pool, is_verbose, package_name, @@ -932,7 +933,7 @@ impl Problem { &all_repos_packages, "constraint", constraint, - ); + )); } let mut suffix = String::new(); @@ -967,7 +968,7 @@ impl Problem { } } - return ( + return Ok(( format!( "- Root composer.json requires {}{}, ", package_name, @@ -983,25 +984,25 @@ impl Problem { }, suffix ), - ); + )); } if !Preg::is_match3(r"{^[A-Za-z0-9_./-]+$}", package_name, None).unwrap_or(false) { let illegal_chars = Preg::replace(r"{[A-Za-z0-9_./-]+}", "", package_name).unwrap_or_default(); - return ( + return Ok(( format!("- Root composer.json requires {}, it ", package_name), format!( "could not be found, it looks like its name is invalid, \"{}\" is not allowed in package names.", illegal_chars ), - ); + )); } - let providers_str = Self::get_providers_list(repository_set, package_name, 15); + let providers_str = Self::get_providers_list(repository_set, package_name, 15)?; if let Some(ps) = providers_str { - return ( + return Ok(( format!( "- Root composer.json requires {}{}, it ", package_name, @@ -1011,14 +1012,14 @@ impl Problem { "could not be found in any version, but the following packages provide it:\n{} Consider requiring one of these to satisfy the {} requirement.", ps, package_name ), - ); + )); } - ( + Ok(( format!("- Root composer.json requires {}, it ", package_name), "could not be found in any version, there may be a typo in the package name." .to_string(), - ) + )) } /// @internal @@ -1428,8 +1429,8 @@ impl Problem { repository_set: &RepositorySet, package_name: &str, max_providers: i64, - ) -> Option { - let providers = repository_set.get_providers(package_name); + ) -> anyhow::Result> { + let providers = repository_set.get_providers(package_name)?; if providers.len() > 0 { let provider_count = providers.len() as i64; let slice: Vec = if provider_count > max_providers + 1 @@ -1463,9 +1464,9 @@ impl Problem { )); } - return Some(providers_str); + return Ok(Some(providers_str)); } - None + Ok(None) } } diff --git a/crates/shirabe/src/dependency_resolver/request.rs b/crates/shirabe/src/dependency_resolver/request.rs index df26535..88957f2 100644 --- a/crates/shirabe/src/dependency_resolver/request.rs +++ b/crates/shirabe/src/dependency_resolver/request.rs @@ -195,11 +195,12 @@ impl Request { pub fn get_present_map( &self, package_ids: bool, - ) -> IndexMap { + ) -> anyhow::Result> { let mut present_map: IndexMap = IndexMap::new(); if let Some(ref locked_repository) = self.locked_repository { - for package in RepositoryInterface::get_packages(&*locked_repository.borrow()) { + for package in RepositoryInterface::get_packages(&mut *locked_repository.borrow_mut())? + { let key = if package_ids { package.get_id().to_string() } else { @@ -218,7 +219,7 @@ impl Request { present_map.insert(key, package.clone()); } - present_map + Ok(present_map) } pub fn get_fixed_packages_map(&self) -> IndexMap { diff --git a/crates/shirabe/src/dependency_resolver/rule.rs b/crates/shirabe/src/dependency_resolver/rule.rs index 1681509..3d7bb2d 100644 --- a/crates/shirabe/src/dependency_resolver/rule.rs +++ b/crates/shirabe/src/dependency_resolver/rule.rs @@ -347,10 +347,10 @@ impl Rule { is_verbose: bool, installed_map: &IndexMap, _learned_pool: &Vec>>>, - ) -> String { + ) -> anyhow::Result { let mut literals = self.get_literals(); - match self.get_reason() { + Ok(match self.get_reason() { r if r == RULE_ROOT_REQUIRE => { let reason_data = self.get_reason_data(); let (package_name, constraint): (&str, &AnyConstraint) = match reason_data { @@ -358,16 +358,16 @@ impl Rule { package_name, constraint, } => (package_name.as_str(), constraint), - _ => return String::new(), + _ => return Ok(String::new()), }; let packages = pool.what_provides(package_name, Some(constraint)); if 0 == packages.len() { - return format!( + return Ok(format!( "No package found to satisfy root composer.json require {} {}", package_name, constraint.get_pretty_string(), - ); + )); } let packages_non_alias: Vec = packages @@ -378,11 +378,11 @@ impl Rule { if packages_non_alias.len() == 1 { let package = &packages_non_alias[0]; if request.is_locked_package(package.clone()) { - return format!( + return Ok(format!( "{} is locked to version {} and an update of this package was not requested.", package.get_pretty_name(), package.get_pretty_version(), - ); + )); } } @@ -403,16 +403,16 @@ impl Rule { r if r == RULE_FIXED => { let package_in = match self.get_reason_data() { ReasonData::Fixed { package } => package.clone(), - _ => return String::new(), + _ => return Ok(String::new()), }; let package = self.deduplicate_default_branch_alias(package_in); if request.is_locked_package(package.clone()) { - return format!( + return Ok(format!( "{} is locked to version {} and an update of this package was not requested.", package.get_pretty_name(), package.get_pretty_version(), - ); + )); } format!( @@ -433,7 +433,7 @@ impl Rule { let link = match reason_data { ReasonData::Link(l) => l, - _ => return String::new(), + _ => return Ok(String::new()), }; // swap literals if they are not in the right order with package2 being the conflicter if link.get_source() == package1.get_name() { @@ -494,7 +494,7 @@ impl Rule { let reason_data = self.get_reason_data(); let link = match reason_data { ReasonData::Link(l) => l, - _ => return String::new(), + _ => return Ok(String::new()), }; let mut requires: Vec = vec![]; @@ -525,9 +525,9 @@ impl Rule { is_verbose, target_name, Some(link.get_constraint()), - ); + )?; - return format!("{} -> {}", text, reason.1); + return Ok(format!("{} -> {}", text, reason.1)); } } @@ -577,7 +577,7 @@ impl Rule { } if installed_packages.len() > 0 && removable_packages.len() > 0 { - return format!( + return Ok(format!( "{} cannot be installed as that would require removing {}. {}", self.format_packages_unique_from_packages( pool, @@ -594,16 +594,16 @@ impl Rule { true, ), reason, - ); + )); } - return format!( + return Ok(format!( "Only one of these can be installed: {}. {}", self.format_packages_unique_from_literals( pool, &literals, is_verbose, None, true ), reason, - ); + )); } format!( @@ -665,7 +665,7 @@ impl Rule { // avoid returning content like "9999999-dev is an alias of dev-master" as it is useless if alias_package.get_version() == VersionParser::DEFAULT_BRANCH_ALIAS { - return String::new(); + return Ok(String::new()); } let package = self.deduplicate_default_branch_alias(pool.literal_to_package(literals[1])); @@ -682,7 +682,7 @@ impl Rule { // avoid returning content like "9999999-dev is an alias of dev-master" as it is useless if alias_package.get_version() == VersionParser::DEFAULT_BRANCH_ALIAS { - return String::new(); + return Ok(String::new()); } let package = self.deduplicate_default_branch_alias(pool.literal_to_package(literals[0])); @@ -704,7 +704,7 @@ impl Rule { format!("({})", rule_text) } - } + }) } // Corresponds the variant formatPackagesUnique() that takes an array of BasePackages. diff --git a/crates/shirabe/src/dependency_resolver/solver.rs b/crates/shirabe/src/dependency_resolver/solver.rs index 281c796..d88d9bb 100644 --- a/crates/shirabe/src/dependency_resolver/solver.rs +++ b/crates/shirabe/src/dependency_resolver/solver.rs @@ -299,7 +299,7 @@ impl Solver { // LockTransaction stores PackageInterfaceHandle maps; widen the request's BasePackageHandle // maps into them. let present_map = request - .get_present_map(false) + .get_present_map(false)? .into_iter() .map(|(k, v)| (k, v.into())) .collect(); diff --git a/crates/shirabe/src/dependency_resolver/solver_problems_exception.rs b/crates/shirabe/src/dependency_resolver/solver_problems_exception.rs index 17ebaf2..7aa8a46 100644 --- a/crates/shirabe/src/dependency_resolver/solver_problems_exception.rs +++ b/crates/shirabe/src/dependency_resolver/solver_problems_exception.rs @@ -52,8 +52,8 @@ impl SolverProblemsException { pool: &mut Pool, is_verbose: bool, is_dev_extraction: bool, - ) -> String { - let installed_map = request.get_present_map(true); + ) -> anyhow::Result { + let installed_map = request.get_present_map(true)?; let mut missing_extensions: Vec = Vec::new(); let mut is_caused_by_lock = false; @@ -61,16 +61,14 @@ impl SolverProblemsException { for problem in &self.problems { problems.push(format!( "{}\n", - problem - .get_pretty_string( - repository_set, - request, - pool, - is_verbose, - &installed_map, - &self.learned_pool - ) - .unwrap_or_default() + problem.get_pretty_string( + repository_set, + request, + pool, + is_verbose, + &installed_map, + &self.learned_pool + )? )); // TODO(phase-b): get_reasons returns an IndexMap; flatten its values into Vec>. let reasons_vec: Vec>>> = problem @@ -126,7 +124,7 @@ impl SolverProblemsException { text.push_str(&hints.join("\n\n")); } - text + Ok(text) } pub fn get_problems(&self) -> &Vec { -- cgit v1.3.1