Add String.replace #190

thiagoarrais · 2019-10-24T16:28:33Z

Work in progress pull request for fixing #116

The implementation isn't complete yet, but I intend to incorporate changes based on feedback. For starters, just let me know if this is the structure you expect. I'll try to evolve the replace method to be spec-compliant.

Related question: does this project use tests? Where should I add them if so?

TODO List:

Do not interpret string pattern as regex
Support regex pattern
Support funtion replacement
~~Include function length (related: Create a macro for properly setting builtin member functions #193)~~

jasonwilliams · 2019-10-25T10:23:59Z

Related question: does this project use tests? Where should I add them if so?

Yes, there are quite a few tests in this project, each file (or module) has the tests embedded with the code. So for String, you will see the tests at the bottom of the file. Im sure there are enough tests here for you to familiar yourself with.
https://github.com/jasonwilliams/boa/blob/master/src/lib/builtins/string.rs#L781-L1044

jasonwilliams · 2019-10-25T10:31:26Z

Im not sure if using regex directly will work, as the rules may differ from rust to JS.
We have a RegExp object which has match here:
https://github.com/jasonwilliams/boa/blob/master/src/lib/builtins/regexp.rs#L257-L274

Maybe you could use that?

Tests could be added first to see which solution is more compatible

Secondly incase you're unaware your rustfmt is failing:
https://github.com/jasonwilliams/boa/pull/190/checks?check_run_id=273620005

You can run cargo fmt --verbose in the project

thiagoarrais · 2019-10-25T13:16:56Z

Nice, @jasonwilliams! I'll add tests (sorry, hadn't located them just out of Rust-noobness) and rustfmt sometime today.

About RegExp, I started with that, but Rust complained about it not being exported or something. But my final version will most likely use that.

Thanks for the feedback!

thiagoarrais · 2019-10-25T17:05:30Z

This is taking some shape now.

I have included a get_argument function inspired by but different than regexp::get_argument. It is designed to mimic JS' behaviour of interpreting missing parameters as undefined. I suspect such a function already exists somewhere in the codebase. Care to help me find it?

IovoslavIovchev · 2019-10-30T04:44:18Z

src/lib/builtins/string.rs

@@ -766,6 +798,7 @@ pub fn create_constructor(global: &Value) -> Value {
    proto.set_field_slice("substr", to_value(substr as NativeFunctionData));
    proto.set_field_slice("valueOf", to_value(value_of as NativeFunctionData));
    proto.set_field_slice("matchAll", to_value(match_all as NativeFunctionData));
+    proto.set_field_slice("replace", to_value(replace as NativeFunctionData));


You need to set the length property of the function as well.

I don't know if I understood that. Do you mean str.replace.length should return 2? If so, would you say that str.charAt.length should return 1 then?

Yes, to both your questions. A function's length represents the number of "formal" parameters, e.g. those with no default value.

Do the other functions in String already support that in boa or are you suggesting that they should? I'm locally getting str.charAt.length == undefined, but I may have an outdated branch...

Unfortunately, some functions in boa (not just in String) don't have their length set because it is easily missable.

I have suggested an approach in #193 and would very much like we have something like that, so we don't miss these things.

Nice. I'm working on other stuff here, but I'll make sure to check where that PR of yours goes.

thiagoarrais · 2019-10-31T17:07:38Z

src/lib/builtins/regexp.rs

    /// Regex matcher.
-    matcher: Regex,
+    pub matcher: Regex,


Need some help here. I'm feeling a bit uneasy about making this public. What is the idiomatic way to access regex funcionality from outside the regexp module?

Do you intend to modify the value in any way?

The most "popular" and obvious way would be to have an immutable getter (and a mutable one only if really needed).

I don't intend to modify the value. I was considering getter vs. something like adding a replace_all delegating method. The second option could be used to deal with differences between JS and Rust regex engines.

What exactly is your idea for replace_all?

Something along the lines of fbf8817. But I'm not really sure this is the intended use for the lib structs.

IovoslavIovchev · 2019-11-06T06:10:36Z

src/lib/builtins/string.rs

+    // We then do the same thing for the other args
+    // TODO: what if there are missing arguments?
+    let pattern = get_argument(args, 0);
+    if undefined() == pattern {


If I am not mistaken, there was an is_undefined method.
So you can try pattern.is_undefined() instead.

Good catch!

thiagoarrais · 2019-11-20T16:14:51Z

I think this is ready for another round of review.

My main concern here is about @IovoslavIovchev's suggestion on length. It doesn't seem to work, even after adapting for #206. I'm assuming this is unrelated to this change because some other functions in String don't return a valid length. But I'm open to adapting the code if needed.

thiagoarrais · 2019-11-26T16:41:00Z

The problem with length seems related to #210. This should be good to go. Please let me know if you need anything else.

IovoslavIovchev · 2019-11-30T08:23:37Z

src/lib/builtins/string.rs

+    // Make a Rust Regex from the first argument
+    let pattern = get_argument(args, 0);
+    if pattern.is_undefined() {
+        return Ok(to_value(this_str));


Suggested change

return Ok(to_value(this_str));

return Ok(this.clone());

This is just a small optimisation to avoid calling ctx.value_to_rust_string if the regex is undefined

IovoslavIovchev · 2019-11-30T08:24:11Z

src/lib/builtins/string.rs

+pub fn replace(this: &Value, args: &[Value], ctx: &mut Interpreter) -> ResultValue {
+    // First we get it the actual string a private field stored on the object only the engine has access to.
+    // Then we convert it into a Rust String
+    let this_str: &str = &ctx.value_to_rust_string(this);


Move this after the if statement below.

IovoslavIovchev · 2019-11-30T08:28:10Z

src/lib/builtins/string.rs

+                .call(&replacement, &undefined(), args)
+                .unwrap_or_else(|_| undefined());
+            ctx.value_to_rust_string(&rs_value)
+        }) as Box<dyn FnMut((&Captures)) -> (String)>


Remove the parentheses, so it becomes

Suggested change

}) as Box<dyn FnMut((&Captures)) -> (String)>

}) as Box<dyn FnMut(&Captures) -> String>

IovoslavIovchev · 2019-11-30T08:47:09Z

src/lib/builtins/string.rs

+    Ok(to_value(result_str.into_owned()))
+}
+
+fn get_argument(args: &[Value], idx: usize) -> Value {


This is a bit more general, so it would be better you move it somewhere, where it can be widely accessed.

I am thinking exec.rs. What do you think?

I actually wonder if this already exists somewhere. But I can't find it...

I know something similar exists in regexp.rs but nothing else.

IovoslavIovchev · 2019-11-30T08:54:39Z

src/lib/builtins/string.rs

+        "#;
+
+        forward(&mut engine, init);
+        assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the monkey reacted, was it really lazy?");


Suggested change

assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the monkey reacted, was it really lazy?");

assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the dog reacted, was it really lazy?");

IovoslavIovchev · 2019-11-30T08:58:25Z

src/lib/builtins/string.rs

+        forward(&mut engine, init);
+        assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the monkey reacted, was it really lazy?");
+        assert_eq!(forward(&mut engine, "result2"), "The quick brown fox jumps over the lazy monkey. If the dog reacted, was it really lazy?");
+        assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the monkey.reacted, was it really lazy?");


Suggested change

assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the monkey.reacted, was it really lazy?");

assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the dog reacted, was it really lazy?");

IovoslavIovchev · 2019-11-30T09:00:14Z

src/lib/builtins/string.rs

+        assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the monkey.reacted, was it really lazy?");
+        assert_eq!(
+            forward(&mut engine, "funResult1"),
+            "The quick brown fox jumps over the lazy DOG. If the DOG reacted, was it really lazy?"


Suggested change

"The quick brown fox jumps over the lazy DOG. If the DOG reacted, was it really lazy?"

"The quick brown fox jumps over the lazy DOG. If the dog reacted, was it really lazy?"

IovoslavIovchev · 2019-11-30T09:02:03Z

src/lib/builtins/string.rs

+        );
+        assert_eq!(
+            forward(&mut engine, "funResult2"),
+            "The quick brown fox jumps over the lazy DOG. If the DOG reacted, was it really lazy?"


Suggested change

"The quick brown fox jumps over the lazy DOG. If the DOG reacted, was it really lazy?"

"The quick brown fox jumps over the lazy DOG. If the dog reacted, was it really lazy?"

IovoslavIovchev · 2019-11-30T09:04:03Z

src/lib/builtins/string.rs

+            forward(&mut engine, "exceptional1"),
+            "The quick brown fox jumps over the lazy dog. If the dog reacted, was it really lazy?"
+        );
+        assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the undefined reacted, was it really lazy?");


Suggested change

assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the undefined reacted, was it really lazy?");

assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the dog reacted, was it really lazy?");

IovoslavIovchev · 2019-11-30T09:04:32Z

src/lib/builtins/string.rs

+            "The quick brown fox jumps over the lazy dog. If the dog reacted, was it really lazy?"
+        );
+        assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the undefined reacted, was it really lazy?");
+        assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the undefined reacted, was it really lazy?");


Remove this line, it's a duplicate

IovoslavIovchev · 2019-11-30T22:29:29Z

The current implementation replaces all occurrences of the first argument when it's a String, when, in fact, it should only replace the first occurrence. See the spec for more info.

IovoslavIovchev · 2019-12-11T06:17:17Z

@thiagoarrais do you need any help with this?

thiagoarrais · 2019-12-16T15:29:42Z

I think I've addressed most of your comments, @IovoslavIovchev . The one thing I couldn't tackle yet is the spec breakage regarding replacing only the first occurrence. I plan on getting back to this some time, but I'm a little short on time. If you could help on that, it would be greatly appreciated.

Maybe you can give some general direction on a good approach? I'd also include a commit of yours here if you're willing to branch from this PR.

IovoslavIovchev · 2019-12-17T05:20:27Z

@thiagoarrais no worries. I will take a look.

jasonwilliams · 2020-01-21T11:53:05Z

This has now been added

IovoslavIovchev reviewed Oct 30, 2019

View reviewed changes

thiagoarrais force-pushed the string-replace branch from a72479b to a540bd7 Compare October 31, 2019 16:32

thiagoarrais commented Oct 31, 2019

View reviewed changes

thiagoarrais force-pushed the string-replace branch from 3a674c7 to 8215057 Compare November 5, 2019 17:21

IovoslavIovchev reviewed Nov 6, 2019

View reviewed changes

thiagoarrais force-pushed the string-replace branch 2 times, most recently from 2e3b0a5 to 1899da7 Compare November 20, 2019 16:09

thiagoarrais changed the title ~~WIP: Add String.replace~~ Add String.replace Nov 20, 2019

IovoslavIovchev reviewed Nov 30, 2019

View reviewed changes

thiagoarrais added 2 commits December 3, 2019 13:53

Adds String.replace

b42a50f

Include @IovoslavIovchev's suggestions

2212b8b

thiagoarrais force-pushed the string-replace branch from 1899da7 to 2212b8b Compare December 3, 2019 17:32

rustfmt

3c383ff

jasonwilliams closed this Jan 21, 2020

	}) as Box<dyn FnMut((&Captures)) -> (String)>
	}) as Box<dyn FnMut(&Captures) -> String>

	assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the monkey reacted, was it really lazy?");
	assert_eq!(forward(&mut engine, "result1"), "The quick brown fox jumps over the lazy monkey. If the dog reacted, was it really lazy?");

	assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the monkey.reacted, was it really lazy?");
	assert_eq!(forward(&mut engine, "regexResult"), "The quick brown fox jumps over the lazy monkey. If the dog reacted, was it really lazy?");

	"The quick brown fox jumps over the lazy DOG. If the DOG reacted, was it really lazy?"
	"The quick brown fox jumps over the lazy DOG. If the dog reacted, was it really lazy?"

	assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the undefined reacted, was it really lazy?");
	assert_eq!(forward(&mut engine, "exceptional2"), "The quick brown fox jumps over the lazy undefined. If the dog reacted, was it really lazy?");

Add String.replace #190

Add String.replace #190

Conversation

thiagoarrais commented Oct 24, 2019 • edited Loading

jasonwilliams commented Oct 25, 2019 • edited Loading

jasonwilliams commented Oct 25, 2019 • edited Loading

thiagoarrais commented Oct 25, 2019

thiagoarrais commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoarrais Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

IovoslavIovchev Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoarrais Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoarrais commented Nov 20, 2019

thiagoarrais commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IovoslavIovchev commented Nov 30, 2019

IovoslavIovchev commented Dec 11, 2019

thiagoarrais commented Dec 16, 2019

IovoslavIovchev commented Dec 17, 2019

jasonwilliams commented Jan 21, 2020

thiagoarrais commented Oct 24, 2019 •

edited

Loading

jasonwilliams commented Oct 25, 2019 •

edited

Loading

jasonwilliams commented Oct 25, 2019 •

edited

Loading

thiagoarrais Oct 31, 2019 •

edited

Loading

IovoslavIovchev Oct 31, 2019 •

edited

Loading

thiagoarrais Oct 31, 2019 •

edited

Loading