From 8b57e90f5c563bbc1a6060d9f5e94b5d3cd91324 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Mon, 17 Aug 2026 03:09:04 +0900 Subject: fix(pcre): preserve unmatched groups in Preg::match_all*() PHP's Preg::matchAll() and matchAllWithOffsets() always set PREG_UNMATCHED_AS_NULL, so a non-participating group is `null` and its offset is -1. The Rust wrappers collapsed those to "" and 0, so callers could not tell a group that did not participate from one that matched an empty string at offset 0, and the offset value matched no PHP mode at all. Hand the shim's representation through unchanged and let each caller mirror what the PHP original does with it: `isset()` and `(string)` casts stay lenient, while `assert(is_string(...))` and the *StrictGroups() variants become `expect()`. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/php_file_parser.rs | 18 ++++---- crates/shirabe-pcre/src/preg.rs | 50 +++------------------- crates/shirabe/src/command/init_command.rs | 19 ++++---- crates/shirabe/src/downloader/git_downloader.rs | 15 ++++--- .../shirabe/src/package/version/version_bumper.rs | 9 ++-- 5 files changed, 43 insertions(+), 68 deletions(-) (limited to 'crates') diff --git a/crates/shirabe-class-map-generator/src/php_file_parser.rs b/crates/shirabe-class-map-generator/src/php_file_parser.rs index 0c82a558..e6eeb028 100644 --- a/crates/shirabe-class-map-generator/src/php_file_parser.rs +++ b/crates/shirabe-class-map-generator/src/php_file_parser.rs @@ -99,13 +99,12 @@ impl PhpFileParser { let ns = matches .get(&CaptureKey::ByName("ns".to_owned())) .and_then(|v| v.get(i)) - .map(|s| s.as_str()) - .unwrap_or(""); - if !ns.is_empty() { + .and_then(|s| s.as_deref()); + if ns.is_some_and(|ns| !ns.is_empty()) { let nsname = matches .get(&CaptureKey::ByName("nsname".to_owned())) .and_then(|v| v.get(i)) - .map(|s| s.as_str()) + .and_then(|s| s.as_deref()) .unwrap_or(""); namespace = str_replace_array( &[ @@ -121,8 +120,8 @@ impl PhpFileParser { let name = matches .get(&CaptureKey::ByName("name".to_owned())) .and_then(|v| v.get(i)) - .map(|s| s.as_str()) - .unwrap_or(""); + .and_then(|s| s.as_deref()) + .expect("the `name` group participates whenever `ns` does not"); // skip anon classes extending/implementing if name == "extends" { continue; @@ -142,9 +141,10 @@ impl PhpFileParser { } else if matches .get(&CaptureKey::ByName("type".to_owned())) .and_then(|v| v.get(i)) - .map(|s| s.to_lowercase()) - .as_deref() - == Some("enum") + .and_then(|s| s.as_deref()) + .unwrap_or("") + .to_lowercase() + == "enum" { // something like: // enum Foo: int { HERP = '123'; } diff --git a/crates/shirabe-pcre/src/preg.rs b/crates/shirabe-pcre/src/preg.rs index a761608e..822609a9 100644 --- a/crates/shirabe-pcre/src/preg.rs +++ b/crates/shirabe-pcre/src/preg.rs @@ -63,7 +63,7 @@ impl Preg { pub fn match_all3( pattern: impl PregPattern, subject: &str, - matches: Option<&mut IndexMap>>, + matches: Option<&mut IndexMap>>>, ) -> usize { Self::match_all5(pattern, subject, matches) } @@ -71,13 +71,13 @@ impl Preg { fn match_all5( pattern: impl PregPattern, subject: &str, - matches: Option<&mut IndexMap>>, + matches: Option<&mut IndexMap>>>, ) -> usize { let mut internal: IndexMap>> = IndexMap::new(); let result = preg_match_all2(pattern, subject, &mut internal); if let Some(out) = matches { - *out = null_to_empty_match_all(internal); + *out = internal; } result @@ -86,13 +86,13 @@ impl Preg { fn match_all_with_offsets5( pattern: impl PregPattern, subject: &str, - matches: Option<&mut IndexMap>>, + matches: Option<&mut IndexMap, i64)>>>, ) -> usize { let mut internal: IndexMap, i64)>> = IndexMap::new(); let result = preg_match_all_offset_capture2(pattern, subject, &mut internal); if let Some(out) = matches { - *out = null_to_empty_offset_match_all(internal); + *out = internal; } result @@ -243,7 +243,7 @@ impl Preg { pub fn is_match_all3( pattern: impl PregPattern, subject: &str, - matches: Option<&mut IndexMap>>, + matches: Option<&mut IndexMap>>>, ) -> bool { Self::match_all5(pattern, subject, matches) > 0 } @@ -251,7 +251,7 @@ impl Preg { pub fn is_match_all_with_offsets3( pattern: impl PregPattern, subject: &str, - matches: Option<&mut IndexMap>>, + matches: Option<&mut IndexMap, i64)>>>, ) -> bool { Self::match_all_with_offsets5(pattern, subject, matches) > 0 } @@ -291,39 +291,3 @@ fn drop_null_matches_ref( .filter_map(|(key, value)| value.clone().map(|value| (key.clone(), value))) .collect() } - -// In the `Vec`-valued maps a per-iteration `null` cannot be stored, so -// unmatched groups collapse to "" (the classic non-PREG_UNMATCHED_AS_NULL form). -fn null_to_empty_match_all( - matches: IndexMap>>, -) -> IndexMap> { - matches - .into_iter() - .map(|(key, values)| { - ( - key, - values - .into_iter() - .map(|value| value.unwrap_or_default()) - .collect(), - ) - }) - .collect() -} - -fn null_to_empty_offset_match_all( - matches: IndexMap, i64)>>, -) -> IndexMap> { - matches - .into_iter() - .map(|(key, values)| { - ( - key, - values - .into_iter() - .map(|(value, offset)| (value.unwrap_or_default(), offset.max(0) as usize)) - .collect(), - ) - }) - .collect() -} diff --git a/crates/shirabe/src/command/init_command.rs b/crates/shirabe/src/command/init_command.rs index c348fa6f..1ca3e428 100644 --- a/crates/shirabe/src/command/init_command.rs +++ b/crates/shirabe/src/command/init_command.rs @@ -175,17 +175,20 @@ impl InitCommand { ) == 0 { *self.git_config.borrow_mut() = Some(IndexMap::new()); - let mut m: IndexMap> = IndexMap::new(); + let mut m: IndexMap>> = IndexMap::new(); if Preg::is_match_all3(php_regex!(r"{^([^=]+)=(.*)$}m"), &output, Some(&mut m)) { - let keys: Vec = m.get(&CaptureKey::ByIndex(1)).cloned().unwrap_or_default(); - let values: Vec = + let keys: Vec> = + m.get(&CaptureKey::ByIndex(1)).cloned().unwrap_or_default(); + let values: Vec> = m.get(&CaptureKey::ByIndex(2)).cloned().unwrap_or_default(); for (key, value) in keys.iter().zip(values.iter()) { - self.git_config - .borrow_mut() - .as_mut() - .unwrap() - .insert(key.clone(), value.clone()); + self.git_config.borrow_mut().as_mut().unwrap().insert( + key.clone() + .expect("group 1 participates whenever the pattern matches"), + value + .clone() + .expect("group 2 participates whenever the pattern matches"), + ); } } diff --git a/crates/shirabe/src/downloader/git_downloader.rs b/crates/shirabe/src/downloader/git_downloader.rs index 4394cc95..b34f7800 100644 --- a/crates/shirabe/src/downloader/git_downloader.rs +++ b/crates/shirabe/src/downloader/git_downloader.rs @@ -109,7 +109,7 @@ impl GitDownloader { .cloned() .unwrap_or_default(); - let mut branches_match: IndexMap> = IndexMap::new(); + let mut branches_match: IndexMap>> = IndexMap::new(); if !Preg::is_match_all3( format!("{{^{} refs/heads/(.+)$}}mi", preg_quote(&head_ref, None)), &refs, @@ -121,7 +121,10 @@ impl GitDownloader { let candidate_branches: Vec = branches_match .get(&CaptureKey::ByIndex(1)) .cloned() - .unwrap_or_default(); + .unwrap_or_default() + .into_iter() + .map(|branch| branch.expect("group 1 participates whenever the pattern matches")) + .collect(); // use the first match as branch name for now let mut branch = candidate_branches[0].clone(); @@ -134,7 +137,7 @@ impl GitDownloader { // try to find matching branch names in remote repos for candidate in &candidate_branches { - let mut m: IndexMap> = IndexMap::new(); + let mut m: IndexMap>> = IndexMap::new(); if Preg::is_match_all3( format!( "{{^[a-f0-9]+ refs/remotes/((?:[^/]+)/{})$}}mi", @@ -143,11 +146,13 @@ impl GitDownloader { &refs, Some(&mut m), ) { - let matches: Vec = + let matches: Vec> = m.get(&CaptureKey::ByIndex(1)).cloned().unwrap_or_default(); for match_ in matches { branch = candidate.clone(); - remote_branches.push(match_); + remote_branches.push( + match_.expect("group 1 participates whenever the pattern matches"), + ); } break; } diff --git a/crates/shirabe/src/package/version/version_bumper.rs b/crates/shirabe/src/package/version/version_bumper.rs index fcdbf246..83df211a 100644 --- a/crates/shirabe/src/package/version/version_bumper.rs +++ b/crates/shirabe/src/package/version/version_bumper.rs @@ -78,7 +78,7 @@ impl VersionBumper { major = major ); - let mut matches: IndexMap> = IndexMap::new(); + let mut matches: IndexMap, i64)>> = IndexMap::new(); if Preg::is_match_all_with_offsets3(&pattern, &pretty_constraint, Some(&mut matches)) { let mut modified = pretty_constraint.clone(); let constraint_matches = matches @@ -86,8 +86,11 @@ impl VersionBumper { .cloned() .unwrap_or_default(); for match_ in constraint_matches.iter().rev() { - let match_str = &match_.0; - let match_offset = match_.1 as i64; + let match_str = match_ + .0 + .as_deref() + .expect("the `constraint` group participates whenever the pattern matches"); + let match_offset = match_.1; let suffix = if match_str.matches('.').count() == 2 && version_without_suffix.matches('.').count() == 1 { -- cgit v1.3.1-4-g156e