952898 - Compliance issues with `String.prototype.startsWith` and `String.prototype.endsWith` due to spec changes after we implemented them

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mathias Bynens]

>> 'ab/c/'.endsWith(/c/);
    returns `true`, but should throw a TypeError
    
    >> '/a/bc'.startsWith(/a/)
    returns `true`, but should throw a TypeError

See step 4 of http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.startswith and http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.endswith.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like the spec changed here.  The current SpiderMonkey implementation is based on http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_drafts&cache=cache&media=harmony:working_draft_ecma-262_edition_6_9-27-12-nomarkup.pdf section 15.5.4.22, which didn't have the regexp object check that the current draft has.

Comment 2

[Till Schneidereit [:till]]

Attachment: String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Simple fix

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8351184 [details] [diff] [review]
String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Review of attachment 8351184 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsstr.cpp
@@ +1421,5 @@
>      args.rval().setInt32(-1);
>      return true;
>  }
>  
> +/* ES6 20121108 draft 21.1.3.18. */

2013, I think

@@ +1433,5 @@
>      if (!str)
>          return false;
>  
> +    // Step 4
> +    if (args.hasDefined(0) && args.get(0).isObject() && args.get(0).toObject().is<RegExpObject>()) {

After the hasDefined check, you can use [0] rather than .get(0); and as you're going to check .isObject() anyway, you can do a length check rather than hasDefined

Comment 4

[Mathias Bynens]

FWIW, feel free to reuse any of my tests: https://github.com/mathiasbynens/String.prototype.endsWith/blob/master/tests/tests.js (I noticed some edge cases, such as `String.prototype.endsWith.call({ 'toString': function() { throw RangeError(); } }, /./)` are not being tested currently.)

Comment 5

[Till Schneidereit [:till]]

Attachment: String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Thanks Mathias and Ms2ger for the comments. Incorporated those and fixed a stupid bug in the error message.

Mathias, I just imported your tests wholesale, I hope that's ok with you. Thanks for writing them!

Comment 6

[Mathias Bynens]

(In reply to Till Schneidereit [:till] from comment #5)
> Created attachment 8351187 [details] [diff] [review]
> Mathias, I just imported your tests wholesale, I hope that's ok with you.

Of course, that’s exactly why I wrote them! Yay :)

Comment 7

[Mathias Bynens]

Comment on attachment 8351187 [details] [diff] [review]
String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Review of attachment 8351187 [details] [diff] [review]:
-----------------------------------------------------------------

feedback- as I’m fine with my tests being re-used for this.

Comment 8

[Mathias Bynens]

Comment on attachment 8351187 [details] [diff] [review]
String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Review of attachment 8351187 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, I meant feedback+, of course. For some reason I can’t seem to clear the `feedback` flag here.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8351187 [details] [diff] [review]
String.prototype.startsWith and .endsWith should throw when called with a regexp as first argument

Review of attachment 8351187 [details] [diff] [review]:
-----------------------------------------------------------------

I added editbugs/canconfirm to your account, Mathias, so you should be able to set and change flags in the future.  :-)

::: js/src/jit-test/tests/basic/string-endswith.js
@@ +63,5 @@
> +
> +assertEq('abc'.endsWith('', false), true);
> +assertEq('abc'.endsWith('\0', false), false);
> +assertEq('abc'.endsWith('c', false), false);
> +assertEq('abc'.endsWith('b', false), false);

assertEq('abc'.endsWith('a', false), false);

@@ +85,5 @@
> +
> +assertEq('abc'.endsWith('', null), true);
> +assertEq('abc'.endsWith('\0', null), false);
> +assertEq('abc'.endsWith('c', null), false);
> +assertEq('abc'.endsWith('b', null), false);

assertEq('abc'.endsWith('a', null), false);

@@ +96,5 @@
> +
> +assertEq('abc'.endsWith('', -Infinity), true);
> +assertEq('abc'.endsWith('\0', -Infinity), false);
> +assertEq('abc'.endsWith('c', -Infinity), false);
> +assertEq('abc'.endsWith('b', -Infinity), false);

assertEq('abc'.endsWith('a', -Infinity), false);

@@ +107,5 @@
> +
> +assertEq('abc'.endsWith('', -1), true);
> +assertEq('abc'.endsWith('\0', -1), false);
> +assertEq('abc'.endsWith('c', -1), false);
> +assertEq('abc'.endsWith('b', -1), false);

assertEq('abc'.endsWith('a', -1), false);

@@ +118,5 @@
> +
> +assertEq('abc'.endsWith('', -0), true);
> +assertEq('abc'.endsWith('\0', -0), false);
> +assertEq('abc'.endsWith('c', -0), false);
> +assertEq('abc'.endsWith('b', -0), false);

assertEq('abc'.endsWith('a', -0), false);

@@ +129,5 @@
> +
> +assertEq('abc'.endsWith('', +0), true);
> +assertEq('abc'.endsWith('\0', +0), false);
> +assertEq('abc'.endsWith('c', +0), false);
> +assertEq('abc'.endsWith('b', +0), false);

assertEq('abc'.endsWith('a', +0), false);

@@ +140,5 @@
> +
> +assertEq('abc'.endsWith('', 1), true);
> +assertEq('abc'.endsWith('\0', 1), false);
> +assertEq('abc'.endsWith('c', 1), false);
> +assertEq('abc'.endsWith('b', 1), false);

assertEq('abc'.endsWith('a', 1), true);

@@ +151,5 @@
> +
> +assertEq('abc'.endsWith('', 2), true);
> +assertEq('abc'.endsWith('\0', 2), false);
> +assertEq('abc'.endsWith('c', 2), false);
> +assertEq('abc'.endsWith('b', 2), true);

assertEq('abc'.endsWith('a', 2), false);

@@ +173,5 @@
> +
> +assertEq('abc'.endsWith('', true), true);
> +assertEq('abc'.endsWith('\0', true), false);
> +assertEq('abc'.endsWith('c', true), false);
> +assertEq('abc'.endsWith('b', true), false);

assertEq('abc'.endsWith('a', true), true);

::: js/src/jsstr.cpp
@@ +1433,5 @@
>      if (!str)
>          return false;
>  
> +    // Step 4
> +    if (args.get(0).isObject() && args[0].toObject().is<RegExpObject>()) {

This would break with RegExp instances from other globals.  Use IsObjectWithClass, jsobjinlines.h, instead.  And please add a test for this case.

@@ +1493,5 @@
>      RootedString str(cx, ThisToStringForStringProto(cx, args));
>      if (!str)
>          return false;
>  
> +    // Step 4

Uh, this is step 3 in the spec.  Because the list of steps begins at 0.  Presumably a spec bug.  Send mail?

@@ +1494,5 @@
>      if (!str)
>          return false;
>  
> +    // Step 4
> +    if (args.get(0).isObject() && args[0].toObject().is<RegExpObject>()) {

Same IsObjectWithClass.

Comment 10

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71bb7391184e

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> > +    // Step 4
> 
> Uh, this is step 3 in the spec.  Because the list of steps begins at 0. 
> Presumably a spec bug.  Send mail?

Weirdly, it's correct in jorendorff's version. Gotta love Word. I filed https://bugs.ecmascript.org/show_bug.cgi?id=2412

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71bb7391184e