Skip to content

fix(ecmascript): support $ substitution patterns in String.prototype.replace/replaceAll #991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chiliec wants to merge 1 commit into
base: main
Choose a base branch
from
+123 −52
Open

fix(ecmascript): support $ substitution patterns in String.prototype.replace/replaceAll #991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 121 additions & 33 deletions nova_vm/src/ecmascript/builtins/text_processing/string_objects/string_prototype.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,21 @@ use unicode_normalization::{
};
use wtf8::{CodePoint, Wtf8Buf};

#[cfg(feature = "regexp")]
use crate::ecmascript::{Object, get_object_method, invoke, reg_exp_create};
use crate::{
ecmascript::{
Agent, ArgumentsList, Array, BUILTIN_STRING_MEMORY, Behaviour, Builtin, BuiltinIntrinsic,
ExceptionType, JsResult, Number, Primitive, PrimitiveObjectData, PrimitiveObjectRecord,
PropertyKey, Realm, String, StringIterator, Value, builders::OrdinaryObjectBuilder,
call_function, create_array_from_list, is_callable, is_reg_exp, is_trimmable_whitespace,
require_object_coercible, to_integer_or_infinity, to_integer_or_infinity_number, to_length,
to_number, to_string, to_string_primitive, to_uint32, try_result_into_js,
try_to_integer_or_infinity, try_to_length, try_to_string,
call_function, create_array_from_list, get, is_callable, is_reg_exp,
is_trimmable_whitespace, require_object_coercible, to_integer_or_infinity,
to_integer_or_infinity_number, to_length, to_number, to_string, to_string_primitive,
to_uint32, try_result_into_js, try_to_integer_or_infinity, try_to_length, try_to_string,
},
engine::{Bindable, GcScope, NoGcScope, Scopable},
engine::{Bindable, GcScope, NoGcScope, Scopable, Scoped},
heap::{ArenaAccess, HeapIndexHandle, IntrinsicFunctionIndexes, WellKnownSymbols},
};
#[cfg(feature = "regexp")]
use crate::{
ecmascript::{Object, get, get_object_method, invoke, reg_exp_create},
engine::Scoped,
};

pub(crate) struct StringPrototype;

Expand Down Expand Up @@ -1754,13 +1751,47 @@ impl StringPrototype {
// 6. If functionalReplace is false, Set replaceValue to ? ToString(replaceValue).
let replace_string = to_string(agent, replace_value.get(agent), gc.reborrow())
.unbind()?
.bind(gc.nogc());
.scope(agent, gc.nogc());

// Everything are strings: `"foo".replace("o", "a")` => use rust's replace
let result = s.to_string_lossy(agent).into_owned().replacen(
search_string_root.to_string_lossy(agent).deref(),
&replace_string.to_string_lossy_(agent),
1,
// 8. Let position be StringIndexOf(s, searchString, 0).
let subject = s.get(agent).to_string_lossy(agent).into_owned();
let search_str = search_string_root
.get(agent)
.to_string_lossy(agent)
.into_owned();
let utf8_position = match subject.find(search_str.deref()) {
Some(position) => position,
// 9. If position is not-found, return s.
None => return Ok(s.get(agent).into()),
};
// 10. Let preceding be the substring of s from 0 to position.
let preceding = subject[..utf8_position].to_string();
// 11. Let following be the substring of s from position + searchLength.
let following = subject[utf8_position + search_str.len()..].to_string();
let position = s.get(agent).utf16_index_(agent, utf8_position);

// 13. Else,
// a. Let captures be a new empty List.
// b. Let replacement be ? GetSubstitution(searchString, s, position, captures, undefined, replaceValue).
let replacement = get_substitution(
agent,
search_string_root.clone(),
s.clone(),
position,
vec![],
None,
replace_string,
gc.reborrow(),
)
.unbind()?
.bind(gc.nogc());

// 14. Return the string-concatenation of preceding, replacement, and following.
let result = format!(
"{}{}{}",
preceding,
replacement.to_string_lossy_(agent),
following
);
Ok(String::from_string(agent, result, gc.into_nogc()).into())
}
Expand Down Expand Up @@ -1857,7 +1888,7 @@ impl StringPrototype {
.scope(agent, gc.nogc());

// 4. Let searchString be ? ToString(searchValue).
let mut search_string = to_string(agent, scoped_search_value.get(agent), gc.reborrow())
let search_string = to_string(agent, scoped_search_value.get(agent), gc.reborrow())
.unbind()?
.bind(gc.nogc());
let search_string_root = search_string.scope(agent, gc.nogc());
Expand Down Expand Up @@ -1944,15 +1975,71 @@ impl StringPrototype {

// 6. If functionalReplace is false, Set replaceValue to ? ToString(replaceValue).
let replace_string = to_string(agent, replace_value.get(agent), gc.reborrow())
.unbind()?
.scope(agent, gc.nogc());

// 7. Let searchLength be the length of searchString.
let search_str = search_string_root
.get(agent)
.to_string_lossy(agent)
.into_owned();
let search_length = search_str.len();
// 8. Let advanceBy be max(1, searchLength).
let advance_by = max(1, search_length);

// 9-11. Collect all match positions (as UTF-8 byte offsets).
let subject = s.get(agent).to_string_lossy(agent).into_owned();
let mut match_positions: Vec<usize> = vec![];
let mut position = 0;
while let Some(pos) = subject
.split_at_checked(position)
.and_then(|(_, str)| str.find(search_str.deref()))
{
match_positions.push(position + pos);
position += advance_by + pos;
}

// If none has found, return s.
if match_positions.is_empty() {
return Ok(s.get(agent).into());
}

// 12. Let endOfLastMatch be 0.
let mut end_of_last_match = 0;
// 13. Let result be the empty String.
let mut result = std::string::String::with_capacity(subject.len());
// 14. For each element p of matchPositions, do
for p in match_positions {
// b. Else, let replacement be
// ? GetSubstitution(searchString, string, p, «», undefined, replaceValue).
let utf16_position = s.get(agent).utf16_index_(agent, p);
let replacement = get_substitution(
agent,
search_string_root.clone(),
s.clone(),
utf16_position,
vec![],
None,
replace_string.clone(),
gc.reborrow(),
)
.unbind()?
.bind(gc.nogc());
// Everything are strings: `"foo".replaceAll("o", "a")` => use rust's replace
search_string = search_string_root.get(agent).bind(gc.nogc());
let s = s.get(agent).bind(gc.nogc());
let result = s.to_string_lossy_(agent).into_owned().replace(
search_string.to_string_lossy_(agent).deref(),
&replace_string.to_string_lossy_(agent),
);
// a. Let preserved be the substring of string from endOfLastMatch to p.
let preserved = &subject[end_of_last_match..p];
// d. Set result to the string-concatenation of result, preserved, and replacement.
let replacement_str = replacement.to_string_lossy_(agent);
result.reserve(preserved.len() + replacement_str.len());
result.push_str(preserved);
result.push_str(&replacement_str);
end_of_last_match = p + search_length;
}

// 15. If endOfLastMatch < the length of string, append the remainder.
if end_of_last_match < subject.len() {
result.push_str(&subject[end_of_last_match..]);
}
// 16. Return result.
Ok(String::from_string(agent, result, gc.into_nogc()).into())
}

Expand Down Expand Up @@ -3337,7 +3424,6 @@ pub(crate) fn to_zero_padded_decimal_string(
/// abstract operation, a decimal digit is a code unit in the inclusive
/// interval from 0x0030 (DIGIT ZERO) to 0x0039 (DIGIT NINE).
#[allow(clippy::too_many_arguments)]
#[cfg(feature = "regexp")]
pub(crate) fn get_substitution<'gc, 'scope>(
agent: &mut Agent,
scoped_matched: Scoped<'scope, String>,
Expand Down Expand Up @@ -3376,13 +3462,15 @@ pub(crate) fn get_substitution<'gc, 'scope>(
// a. NOTE: The following steps isolate ref (a prefix of
// templateRemainder), determine refReplacement (its replacement),
// and then append that replacement to result.
let mut r#ref = template_remainder;
let mut ref_replacement = std::borrow::Cow::Borrowed(template_remainder);
if template_remainder_bytes.len() == 1 {
// h. Else,
// i. Let ref be the substring of templateRemainder from 0 to 1.
// ii. Let refReplacement be ref.
} else if template_remainder_bytes[0] == b'$' {
// h. Else, let ref be the first code point of templateRemainder and
// let refReplacement be ref.
let first_char_len = template_remainder
.chars()
.next()
.map_or(1, |c| c.len_utf8());
let mut r#ref = &template_remainder[..first_char_len];
let mut ref_replacement = std::borrow::Cow::Borrowed(r#ref);
if template_remainder_bytes[0] == b'$' && template_remainder_bytes.len() > 1 {
if template_remainder_bytes[1] == b'$' {
// b. If templateRemainder starts with "$$", then
// i. Let ref be "$$".
Expand All @@ -3409,7 +3497,7 @@ pub(crate) fn get_substitution<'gc, 'scope>(
// ii. Let matchLength be the length of matched.
let match_length = matched.len();
// iii. Let tailPos be position + matchLength.
let tail_pos = position.saturating_add(match_length);
let tail_pos = utf8_position.saturating_add(match_length);
// iv. Let refReplacement be the substring of str from
// min(tailPos, stringLength).
ref_replacement = str[tail_pos.min(utf8_string_length)..].into();
Expand Down
17 changes: 0 additions & 17 deletions tests/expectations.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1184,7 +1184,6 @@
"built-ins/RegExp/S15.10.2.8_A3_T10.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T13.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T14.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T18.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T7.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T8.js": "FAIL",
"built-ins/RegExp/S15.10.2.8_A3_T9.js": "FAIL",
Expand Down Expand Up @@ -1250,7 +1249,6 @@
"built-ins/RegExp/named-groups/non-unicode-property-names.js": "FAIL",
"built-ins/RegExp/named-groups/non-unicode-references.js": "FAIL",
"built-ins/RegExp/named-groups/string-replace-get.js": "FAIL",
"built-ins/RegExp/named-groups/string-replace-nocaptures.js": "FAIL",
"built-ins/RegExp/named-groups/unicode-match.js": "FAIL",
"built-ins/RegExp/named-groups/unicode-property-names-valid.js": "FAIL",
"built-ins/RegExp/named-groups/unicode-property-names.js": "FAIL",
Expand All @@ -1274,15 +1272,6 @@
"built-ins/RegExp/prototype/Symbol.replace/named-groups-fn.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/named-groups.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/poisoned-stdlib.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/result-coerce-capture.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/result-coerce-groups-prop.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/result-coerce-groups.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/result-coerce-matched.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/subst-after.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/subst-before.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-1.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-2.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/subst-matched.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/u-advance-after-empty.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.replace/y-fail-lastindex-no-write.js": "FAIL",
"built-ins/RegExp/prototype/Symbol.search/u-lastindex-advance.js": "FAIL",
Expand Down Expand Up @@ -1676,14 +1665,8 @@
"built-ins/String/prototype/match/regexp-prototype-match-v-u-flag.js": "FAIL",
"built-ins/String/prototype/matchAll/regexp-prototype-matchAll-v-u-flag.js": "FAIL",
"built-ins/String/prototype/replace/S15.5.4.11_A5_T1.js": "FAIL",
"built-ins/String/prototype/replace/regexp-capture-by-index.js": "FAIL",
"built-ins/String/prototype/replace/regexp-prototype-replace-v-u-flag.js": "CRASH",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024-0x0024.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024-0x0026.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024-0x0027.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024-0x003C.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024-0x0060.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024N.js": "FAIL",
"built-ins/String/prototype/replaceAll/getSubstitution-0x0024NN.js": "FAIL",
"built-ins/String/prototype/replaceAll/searchValue-replacer-RegExp-call-fn.js": "FAIL",
Expand Down
4 changes: 2 additions & 2 deletions tests/metrics.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"results": {
"crash": 52,
"fail": 6785,
"pass": 40515,
"fail": 6768,
"pass": 40532,
"skip": 3326,
"timeout": 18,
"unresolved": 37
Expand Down