stdlib/string.js issue(s) and testing

[bFraley]

V8, and browser consoles won't concat or double reference values on the example below. Hopefully this helps with #111

Higgs REPL

h> a ="A";                                                                
A
h> b = "B";
B
h> c = b.replace(b, a); // c is now "A"...or rather return of b.replace    
A
h> c.replace(b, c) // c is "A", but holds b reference....replace with itself is "AA"
AA
h> d = c.replace(b, c);
AA

Example Test

 a = "A", b = "B", c = b.replace(b, a), d = c.replace(b, c);
 assert(d === "A")
'String.replace unexpected result'

See: string.js

[maximecb]

Good find.

For clarity, the second replace returns an incorrect result: c = "B".replace("B", "A"); d = c.replace("B", "A"); print("c=" + c); print("d=" + d);

Under Higgs: c=A d=AA

Under V8: c=A d=A

I'm not sure why the second concat returns any incorrect result. Did you look at the code in stdlib/string.js? Any ideas?

[bFraley]

Part of the bug is in the search. If the search value isn't found, the replace value still prepends the searched string.

"a".replace("y", "z") will return "za"

Now, it seems it gets more complex if one or more, but not all chars are found. I'll keep looking. Is this on purpose for some reason? for (; i < this.length; ++i) Line 320

"a".replace("ab", "zz") returns "zz"

It gets messy when strings and search values are longer:

h> "123".replace("456", "789");
7893

@maximecb do you happen to recall failing tests like this during Tachyon development?

[bFraley]

string.concat(integers)

Any integer parameter passed to string.concat(0...9), deletes any char values and the string length becomes 0, zero. V8 does toString and then concatenates the strings.

h> "helloworld".concat(264);

h> hw = "helloworld".concat(264);

h> hw.length
0

[maximecb]

Tachyon development... That was years ago!

So, you think the problem is with concat? Amazing that such a simple bug made it so far without being found. I'll look into that.

Clearly, we should add failing tests for both concat and for replace.

[maximecb]

As a side note, you should try to always find the simplest failing example if you can, to narrow it down as much as possible. In this case, just this fails:

"h".concat(2);

[bFraley]

I'll try to test all the String functions and put together a simple list of assertions.

I've been probing the String object. If you actually call new String(), without an initial value, then the length of that String Object is 9. Is this expected? new String().length is 9

[maximecb]

Ah! That's because it sees the argument to new String() as being the undefined value... 9 is the length of "undefined". I'll fix it :)

[maximecb]

Fixed the issue with new String()

[bFraley]

Sweet:-) I haven't had a chance to try, do the concatenation or new string length fixes affect the search n replace issue?

These bugs weren't found because most code wouldn't contain or use calls that expose them. Perhaps v8Regex does. It surely makes a lot of replace calls and string methods.

[maximecb]

The replace bug is still there. I might take a look at it tonight.

It is indeed the case. We've tested the more common use cases fairly thoroughly, but the more unusual edge cases are still fuzzy. Another argument for keeping language semantics as simple as possible IMO. JavaScript has way too many corner cases!

[maximecb]

Fixed the issue in String.replace(). It was just a silly bug, whoever wrote the code didn't check that the search string was actually found.

[bFraley]

Cheers!