From c7aa10384a2548167466b4c61f9eff29ed8611f6 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Mon, 17 Aug 2026 07:13:02 +0900 Subject: refactor(preg): report unmatched groups as null throughout The shim carried two reporting modes for the preg_* $matches maps: PHP's default (trailing unmatched groups dropped, interior ones ""), and the PREG_UNMATCHED_AS_NULL form, picked by calling a *_unmatched_as_null() variant. The regex crate hands out Option, which maps onto the null form directly, and no caller distinguished a dropped group from a null one -- preg_match() and preg_match_all2() already reported nulls unconditionally. Keep only the null form; the shim API no longer mirrors PHP's flag set, which is intended. Preg::is_match_with_indexed_captures() modelled PHP's "unset" as a truncated Vec, and now returns Vec>. That is what Composer actually does: Preg::isMatch() always sets PREG_UNMATCHED_AS_NULL, and its callers test groups with `!== null`. preg_match_all(), preg_match_all_set_order() and preg_split_delim_capture() still hand back Vec and keep the "" form. Co-Authored-By: Claude Opus 5 (1M context) --- crates/shirabe-pcre/src/preg.rs | 50 +++++-------- crates/shirabe-php-shim/src/preg.rs | 87 ++-------------------- crates/shirabe/src/command/fund_command.rs | 2 +- crates/shirabe/src/command/update_command.rs | 6 +- crates/shirabe/src/installer/binary_installer.rs | 6 +- .../shirabe/src/package/version/version_guesser.rs | 6 +- crates/shirabe/src/repository/vcs/svn_driver.rs | 12 +-- crates/shirabe/src/util/hg.rs | 2 +- crates/shirabe/src/util/platform.rs | 4 +- 9 files changed, 41 insertions(+), 134 deletions(-) (limited to 'crates') diff --git a/crates/shirabe-pcre/src/preg.rs b/crates/shirabe-pcre/src/preg.rs index 3a997ad3..aa986ae8 100644 --- a/crates/shirabe-pcre/src/preg.rs +++ b/crates/shirabe-pcre/src/preg.rs @@ -13,9 +13,8 @@ pub use shirabe_php_shim::{CaptureKey, PregMatches, PregMatchesAll, PregMatchesAllWithOffsets}; use shirabe_php_shim::{ - PregPattern, preg_grep, preg_match_all_offset_capture_unmatched_as_null, preg_match_all2, - preg_match_map, preg_match2, preg_match2_unmatched_as_null, preg_replace_callback, - preg_replace2, + PregPattern, preg_grep, preg_match_all_offset_capture, preg_match_all2, preg_match_map, + preg_match2, preg_replace_callback, preg_replace2, }; preg_match_map! { @@ -48,7 +47,7 @@ impl Preg { offset: usize, ) -> bool { let mut internal = PregMatches::new(); - let result = preg_match2_unmatched_as_null(pattern, subject, &mut internal, offset); + let result = preg_match2(pattern, subject, &mut internal, offset); if let Some(out) = matches { *out = drop_null_matches(internal); @@ -76,8 +75,7 @@ impl Preg { matches: Option<&mut PregMatchesAllWithOffsets>, ) -> usize { let mut internal = PregMatchesAllWithOffsets::new(); - let result = - preg_match_all_offset_capture_unmatched_as_null(pattern, subject, &mut internal); + let result = preg_match_all_offset_capture(pattern, subject, &mut internal); if let Some(out) = matches { *out = internal; @@ -153,7 +151,7 @@ impl Preg { matches: &mut PregNamedGroups, ) -> bool { let mut internal = PregMatches::new(); - let result = preg_match2_unmatched_as_null(pattern, subject, &mut internal, 0); + let result = preg_match2(pattern, subject, &mut internal, 0); matches.clear(); for (key, value) in internal { @@ -165,38 +163,26 @@ impl Preg { result } + /// `is_match3` with the groups positioned by number rather than keyed, for callers that only + /// read numbered groups. Index 0 is the full match; an unmatched group is `None`. pub fn is_match_with_indexed_captures( pattern: impl PregPattern, subject: &str, - ) -> Option> { - // Classic preg_match semantics (no PREG_UNMATCHED_AS_NULL): trailing - // unmatched groups are truncated, interior unmatched groups become "". + ) -> Option>> { let mut internal = PregMatches::new(); - let result = preg_match2(pattern, subject, &mut internal, 0); - - if !result { + if !preg_match2(pattern, subject, &mut internal, 0) { return None; } - let max_index = internal - .keys() - .filter_map(|key| match key { - CaptureKey::ByIndex(index) => Some(*index), - CaptureKey::ByName(_) => None, - }) - .max() - .unwrap_or(0); - - let mut captures = Vec::with_capacity(max_index + 1); - for index in 0..=max_index { - let value = internal - .get(&CaptureKey::ByIndex(index)) - .and_then(|value| value.clone()) - .unwrap_or_default(); - captures.push(value); - } - - Some(captures) + Some( + internal + .into_iter() + .filter_map(|(key, value)| match key { + CaptureKey::ByIndex(_) => Some(value), + CaptureKey::ByName(_) => None, + }) + .collect(), + ) } pub fn is_match_all( diff --git a/crates/shirabe-php-shim/src/preg.rs b/crates/shirabe-php-shim/src/preg.rs index 6b53bb59..0aa2c43e 100644 --- a/crates/shirabe-php-shim/src/preg.rs +++ b/crates/shirabe-php-shim/src/preg.rs @@ -36,10 +36,6 @@ macro_rules! preg_match_map { self.0.insert(key, value) } - pub fn keys(&self) -> ::indexmap::map::Keys<'_, $key, $value> { - self.0.keys() - } - pub fn iter(&self) -> ::indexmap::map::Iter<'_, $key, $value> { self.0.iter() } @@ -84,7 +80,7 @@ macro_rules! preg_match_map { preg_match_map! { /// A single match's `$matches`, keyed by both the named and the numbered form of each capture - /// group. A `None` value is a group the caller's flags reported as unmatched. + /// group. A `None` value is a group that did not participate in the match. pub struct PregMatches(CaptureKey => Option); } @@ -148,26 +144,6 @@ pub fn preg_match2( subject: &str, matches: &mut PregMatches, offset: usize, -) -> bool { - preg_match2_impl(pattern, subject, matches, offset, false) -} - -// PREG_UNMATCHED_AS_NULL counterpart of preg_match2(). -pub fn preg_match2_unmatched_as_null( - pattern: impl PregPattern, - subject: &str, - matches: &mut PregMatches, - offset: usize, -) -> bool { - preg_match2_impl(pattern, subject, matches, offset, true) -} - -fn preg_match2_impl( - pattern: impl PregPattern, - subject: &str, - matches: &mut PregMatches, - offset: usize, - unmatched_as_null: bool, ) -> bool { let __resolved = pattern.resolve(); let (re, anchored) = __resolved.parts(); @@ -184,11 +160,7 @@ fn preg_match2_impl( matches.clear(); if let Some(caps) = &caps { let names: Vec> = re.capture_names().collect(); - *matches = if unmatched_as_null { - single_match_map_unmatched_as_null(caps, &names) - } else { - single_match_map(caps, &names) - }; + *matches = single_match_map(caps, &names); } caps.is_some() @@ -263,30 +235,11 @@ pub fn preg_match_all_set_order( count } -// A non-participating group is reported at offset -1, holding "". +// A non-participating group is reported as None, at offset -1. pub fn preg_match_all_offset_capture( pattern: impl PregPattern, subject: &str, matches: &mut PregMatchesAllWithOffsets, -) -> usize { - preg_match_all_offset_capture_impl(pattern, subject, matches, false) -} - -// PREG_UNMATCHED_AS_NULL counterpart of preg_match_all_offset_capture(): a -// non-participating group holds null instead of "". -pub fn preg_match_all_offset_capture_unmatched_as_null( - pattern: impl PregPattern, - subject: &str, - matches: &mut PregMatchesAllWithOffsets, -) -> usize { - preg_match_all_offset_capture_impl(pattern, subject, matches, true) -} - -fn preg_match_all_offset_capture_impl( - pattern: impl PregPattern, - subject: &str, - matches: &mut PregMatchesAllWithOffsets, - unmatched_as_null: bool, ) -> usize { let __resolved = pattern.resolve(); let (re, _anchored) = __resolved.parts(); @@ -300,8 +253,7 @@ fn preg_match_all_offset_capture_impl( for (g, column) in groups.iter_mut().enumerate() { let entry = match caps.get(g) { Some(m) => (Some(m.as_str().to_string()), m.start() as i64), - None if unmatched_as_null => (None, -1), - None => (Some(String::new()), -1), + None => (None, -1), }; column.push(entry); } @@ -706,37 +658,10 @@ fn php_match_row(caps: ®ex::Captures) -> Vec { } // Builds a single match's `$matches` map with both named and numbered keys -// (the named key precedes its number). Trailing unmatched groups are dropped -// and interior ones become "". +// (the named key precedes its number). Every group is present; a +// non-participating one is None. fn single_match_map(caps: ®ex::Captures, names: &[Option<&str>]) -> PregMatches { let mut out = PregMatches::new(); - let group_count = caps.len(); - let last_participating = (0..group_count).rev().find(|&i| caps.get(i).is_some()); - - for i in 0..group_count { - let m = caps.get(i); - if m.is_none() - && let Some(last) = last_participating - && i > last - { - break; - } - let value = Some(m.map(|m| m.as_str().to_string()).unwrap_or_default()); - if let Some(Some(name)) = names.get(i) { - out.insert(CaptureKey::ByName((*name).to_string()), value.clone()); - } - out.insert(CaptureKey::ByIndex(i), value); - } - out -} - -// PREG_UNMATCHED_AS_NULL counterpart of single_match_map(): every group is -// present and non-participating ones are None. -fn single_match_map_unmatched_as_null( - caps: ®ex::Captures, - names: &[Option<&str>], -) -> PregMatches { - let mut out = PregMatches::new(); for i in 0..caps.len() { let value = caps.get(i).map(|m| m.as_str().to_string()); diff --git a/crates/shirabe/src/command/fund_command.rs b/crates/shirabe/src/command/fund_command.rs index 7f2dd24c..109943d6 100644 --- a/crates/shirabe/src/command/fund_command.rs +++ b/crates/shirabe/src/command/fund_command.rs @@ -67,7 +67,7 @@ impl FundCommand { php_regex!(r"{^https://github.com/([^/]+)$}"), &url, ) - && let Some(sponsor) = matches.into_iter().nth(1) + && let Some(sponsor) = matches.into_iter().nth(1).flatten() { url = format!("https://github.com/sponsors/{}", sponsor); } diff --git a/crates/shirabe/src/command/update_command.rs b/crates/shirabe/src/command/update_command.rs index 38a79e30..2eca6db7 100644 --- a/crates/shirabe/src/command/update_command.rs +++ b/crates/shirabe/src/command/update_command.rs @@ -466,10 +466,8 @@ impl Command for UpdateCommand { let Some(matches) = matches else { continue; }; - let constraint = parser.parse_constraints(&format!( - "~{}", - matches.get(1).cloned().unwrap_or_default() - ))?; + let constraint = parser + .parse_constraints(&format!("~{}", matches[1].clone().unwrap_or_default()))?; if let Some(existing) = temporary_constraints.get(&package.get_name()) { temporary_constraints.insert( package.get_name(), diff --git a/crates/shirabe/src/installer/binary_installer.rs b/crates/shirabe/src/installer/binary_installer.rs index d5ece82f..606e943f 100644 --- a/crates/shirabe/src/installer/binary_installer.rs +++ b/crates/shirabe/src/installer/binary_installer.rs @@ -328,10 +328,10 @@ impl BinaryInstaller { &bin_contents, ) { // carry over the existing shebang if present, otherwise add our own - let proxy_code = if m.get(1).is_none() { + let proxy_code = if m[1].is_none() { "#!/usr/bin/env php".to_string() } else { - trim(m.get(1).map(|s| s.as_str()).unwrap_or(""), None) + trim(m[1].as_deref().unwrap_or(""), None) }; let bin_path_exported = self .filesystem @@ -377,7 +377,7 @@ impl BinaryInstaller { $data = str_replace('__FILE__', var_export($this->realpath, true), $data);" .to_string(); } - if trim(m.first().map(|s| s.as_str()).unwrap_or(""), None) != "\w+)|%(?P\w+)%)(?P.*)#"), |matches: &PregMatchedGroups| -> String { let var = matches .get(&CaptureKey::ByName("dvar".to_string())) - .filter(|dvar| !dvar.is_empty()) .or_else(|| matches.get(&CaptureKey::ByName("pvar".to_string()))) .map(|s| s.as_str()) .unwrap_or(""); -- cgit v1.3.1-4-g156e