1102219 - Rename String.prototype.contains to String.prototype.includes

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Till Schneidereit [:till]]

In bug 1075059 we discovered that `Array.prototype.contains` breaks the web. Not only is it desirable to have the equivalent String method have the same name, the IE team also reported web-compat issues with `String.prototype.contains`.

For these reasons, TC39 decided to rename `String.prototype.contains` to `includes`. (See bug 1069063 comment 29.)

This is going to be really hard to pull of: we've shipped `String.prototype.contains` since Firefox 18 and have lots of chrome and, presumably, addon usage. We might have to keep `contains` around as an alias with a warning for some releases, after cleaning up chrome code, of course.

Ziyunfei, would you be interested in taking this?

Comment 1

[ziyunfei]

Attachment: Add `includes`; keep `contains` around as an alias with a warning

Comment 2

[ziyunfei]

Attachment: part 2 - Replace `contains` with `includes` in JS code

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8527269 [details] [diff] [review]
Add `includes`; keep `contains` around as an alias with a warning

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

Huh, I would've thought that we had JIT optimizations for String#contains. Not so, apparently; which is nice here, because otherwise we'd have to change lots more code.

This looks a  good except for the tiny nit below and slight changes to the commit message required. Please make it say
"Bug 1102219 - Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a warning. r=till"
(I.e., change the format slightly, be explicit about which builtin's methods you're talking about.)

::: js/src/jsstr.cpp
@@ +1598,5 @@
>      args.rval().setBoolean(StringMatch(text, searchStr, start) != -1);
>      return true;
>  }
>  
> +/* XXX TODO: remove String.prototype.contains (bug 1102219) */

Please file a follow-up bug depending on this one for the actual removal. Then, mention that here without the "XXX" at the beginning.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8527270 [details] [diff] [review]
part 2 - Replace `contains` with `includes` in JS code

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

Looks great!

Please change the commit message to match the pattern from part 1.

Before we land these patches, it's important to clean up as much platform JS as possible, too. I think the easiest way to go about that is doing a full try run with three platforms: one desktop, one Android, one B2G. Then look at the full log files (which are huge :() and search for the warning. I can fully understand if you're not all that interested in doing that, of course. Every bit would help, though. Also, if you do the try run and post the link here, say which logs you're looking at and I'll pick some others to wade through, too.

Comment 5

[ziyunfei]

Attachment: change the commit message

Comment 6

[ziyunfei]

Attachment: part 2 - Replace `contains` with `includes` in JS code

change the commit message

Comment 7

[ziyunfei]

Attachment: part 3 - Replace `contains` with `includes` in chrome code

Comment 8

[ziyunfei]

https://tbpl.mozilla.org/?tree=Try&rev=7557b861b52a

Comment 9

[Tomislav Jovanovic :zombie]

warning spew on "perfectly good, feature-tested, best practices code that polyfills only if the method doesn't exist" in firefox, until we actually remove this is also.. suboptimal (i know, nothing about this is close to "optimal").

maybe we can warn only in nightly+dev+beta, and hope that blog posts about .includes that mention .contains are going to get lots of pickup -- which, given the popularity of array.contains, looks likely.

Comment 10

[ziyunfei]

Try run for "Part 1: Add `String.prototype.includes`; remove `String.prototype.contains` totally":

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=30947b0fcfb8

to see if I've cleaned them up enough.

Comment 11

[ziyunfei]

Attachment: Part 1 - Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning

only warn in non-release builds

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Tomislav Jovanovic [:zombie] from comment #9)
> warning spew on "perfectly good, feature-tested, best practices code that
> polyfills only if the method doesn't exist" in firefox, until we actually
> remove this is also.. suboptimal

Maybe I'm missing something but isn't warning *before* we remove it the whole point of the warning? If we have plans to remove the non-standard .contains, I don't see the problem with a console warning. It's not like the warning is going to break websites or annoy normal users...

Comment 13

[Emanuel Hoogeveen [:ehoogeveen]]

IIUC the problem is that we can't differentiates between 'code that uses .contains' and 'code that supplies its own version of .contains but uses the native version if available', even though the latter case will continue to work just fine after the native version is removed.

But perhaps, getting people to update their code to check for .includes instead wouldn't be a bad thing? After all if they keep their existing checks, they'll never get the native versions that browsers implement.

Comment 14

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Maybe I'm missing something but isn't warning *before* we remove it the
> whole point of the warning? If we have plans to remove the non-standard
> .contains, I don't see the problem with a console warning. It's not like the
> warning is going to break websites or annoy normal users...

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #13)
> But perhaps, getting people to update their code to check for .includes
> instead wouldn't be a bad thing? After all if they keep their existing
> checks, they'll never get the native versions that browsers implement.

The idea as discussed on IRC yesterday is to only warn in non-release builds for now, and start warning in those later, after we have taken some (to be added) telemetry and know that there isn't *too* much content out there anymore that would trigger the warning.

Comment 15

[Till Schneidereit [:till]]

Comment on attachment 8527303 [details] [diff] [review]
part 3 - Replace `contains` with `includes` in chrome code

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

Sorry for taking so long on this. I just realized that that's because I feel uneasy about reviewing this at all. One question on that: did you instrument String#contains to dump all callers and then went and fixed them, or did you do a search and replace over the tree? I hope/assume it's the former, because the latter would quite likely mean that some of the replacements are invalid because they really call some other, content-provided `contains` method. If it's the former, though, I don't think a thorough review of all callsites is even necessary and a green try run should be proof enough for the correctness of the patch.

Comment 16

[ziyunfei]

(In reply to Till Schneidereit [:till] from comment #15)
> Comment on attachment 8527303 [details] [diff] [review]
> part 3 - Replace `contains` with `includes` in chrome code
> 
> Review of attachment 8527303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for taking so long on this. I just realized that that's because I feel
> uneasy about reviewing this at all. One question on that: did you instrument
> String#contains to dump all callers and then went and fixed them, or did you
> do a search and replace over the tree? I hope/assume it's the former,
> because the latter would quite likely mean that some of the replacements are
> invalid because they really call some other, content-provided `contains`
> method. If it's the former, though, I don't think a thorough review of all
> callsites is even necessary and a green try run should be proof enough for
> the correctness of the patch.

Neither of them. I did a full try run (without String#contains) about 6 times and eliminated the related failures (.contains is not a function) as much as possible. The last try run was in comment 10.

Comment 17

[Till Schneidereit [:till]]

Comment on attachment 8527303 [details] [diff] [review]
part 3 - Replace `contains` with `includes` in chrome code

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

Ok, that sounds absolutely like it should just work; let's land this. Thanks for the hard work on this!

Comment 18

[Till Schneidereit [:till]]

Comment on attachment 8527338 [details] [diff] [review]
Part 1 - Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning

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

Thanks!

Comment 19

[ziyunfei]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7557b861b52a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Part 3 needs rebasing.

Comment 21

[Jean-Yves Perrier [:teoli]]

Setting dev-doc-needed back; for tracking purpose we shouldn't consider a doc finished before a bug is closed: it can miss a train or not land at all. This has to be reflected in the doc.

Comment 22

[ziyunfei]

Attachment: patch rebased

Comment 23

[ziyunfei]

Attachment: Increment XDR_BYTECODE_VERSION_SUBTRAHEND

see bug 1107416

Comment 24

[Till Schneidereit [:till]]

Comment on attachment 8534812 [details] [diff] [review]
Increment XDR_BYTECODE_VERSION_SUBTRAHEND

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

Oh, right. Good thing we have that assert :)

Please merge this patch into the one that actually changes js.msg before landing.

Comment 25

[ziyunfei]

Attachment: merged part 4 into part 1

Comment 26

[ziyunfei]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=13a7f1199d7d

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac15ba8568c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/32db3daa20fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a66a625618

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia unit test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/57725f8e7aa1

https://treeherder.mozilla.org/logviewer.html#?job_id=4555778&repo=mozilla-inbound

Comment 29

[ziyunfei]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #28)
> Backed out for Gaia unit test failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57725f8e7aa1
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=4555778&repo=mozilla-
> inbound

I don't know what cause this failure! does anyone know anyone who can help me?

Comment 30

[Till Schneidereit [:till]]

(In reply to ziyunfei from comment #29)
> I don't know what cause this failure! does anyone know anyone who can help
> me?

Hum, I'm not sure why this particular test fails, really. That said, there are lots of usages of `contains` in gaia: https://github.com/mozilla-b2g/gaia/search?q=.contains

Can you do a new try push with the b2g-related tests all activated? Something like "try: -b o -p linux32_gecko -u all -t none" should do the trick.

I'm not sure what to do about the `contains` usages in gaia: ideally we'd fix them all, but I don't want to ask you to wade through them. Particularly because the vast majority will be false-positives: they're calls to the DOMTokenList#contains method (https://dom.spec.whatwg.org/#interface-domtokenlist).

Anyway, a new try run would be great so we can take a look at the logs.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Push to Try from an hour ago (only 1st patch applied but that shouldn't matter): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fe363da06d6

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

So it fails here:

http://mxr.mozilla.org/gaia/source/apps/communications/contacts/test/unit/views/list_test.js#429


    test('adding one at the beginning', function() {
      var newContact = new MockContactAllFields();
      newContact.id = '4';
      newContact.familyName = ['AA'];
      newContact.name = [newContact.givenName + ' ' + newContact.familyName];
      newContact.category = null;
      doRefreshContact(subject, newContact);

      assertNoGroup(groupFav, containerFav);
      var aContacts = assertGroup(groupA, containerA, 2);
      assert.isTrue(noContacts.classList.contains('hide'));
      assert.isTrue(aContacts[0].querySelector('p').innerHTML.indexOf('AA') >
                    -1);
      assert.isTrue(aContacts[1].querySelector('p').innerHTML.indexOf('AD') >
                    -1);
      assertTotal(3, 4);
    });

Comment 33

[Chris Peterson [:cpeterson]]

If this bug is just making String#contains an alias for String#includes, do the Gaia uses of String#contains need to block landing this patch? Separate bugs could be filed for Gaia devs to fix their code.

Comment 34

[Tom S]

Is somebody going to finish this?

Comment 35

[rsuelzer]

Is this going to be finished soon? It would be nice to have native ES6 support for the feature so we don't have to use the non-standard "contains" prototype.

Comment 36

[Chris Peterson [:cpeterson]]

- ziyunfei: do you have time to fix the Gaia test failures?

- evilpie: if ziyunfei is not available, would you like to carry this patch across the finish line? :)

Comment 37

[rsuelzer]

Thanks!  Also, I'm sure you are all aware, but Chromium and Chrome dropped .contains from the string prototype at some point and replaced it with 'includes'.  Which should make your path forward on depreciating this less painful.

Comment 38

[Tom S]

I am busy with other stuff, maybe arai can take a look?

Comment 39

[Tooru Fujisawa [:arai]]

sure, but let's wait for ziyunfei's response first :)

Comment 40

[Tooru Fujisawa [:arai]]

okay, taking over :)

I don't see any gaia related test failure with rebased patches + some more patches for recent changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3044e0117bdd (linux64)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b833ab870bd8 (others)
all red/orange seem to be intermittent.

I'll post extra patches after brushing them up.

Comment 41

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

somehow non-trivial change from previous one.

in previous patch, WARNED_STRING_CONTAINS_DEPRECATED has same value as FOR_OF_PIC_CHAIN
diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h
>     static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
>+    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = INT32X4_TYPE_DESCR + 1;

but it should be sequential, right?
diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h
>     static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
>+    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;
> 
>     /* Total reserved-slot count for global objects. */
>-    static const unsigned RESERVED_SLOTS = FOR_OF_PIC_CHAIN + 1;
>+    static const unsigned RESERVED_SLOTS = WARNED_STRING_CONTAINS_DEPRECATED + 1;

it requires following change also.
diff --git a/js/public/Class.h b/js/public/Class.h
>-#define JSCLASS_GLOBAL_SLOT_COUNT      (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 3 + 31)
>+#define JSCLASS_GLOBAL_SLOT_COUNT      (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 3 + 32)

Comment 42

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Replace `String.prototype.contains` with `String.prototype.includes` in JS code. r=till

only trivial rebase.

Comment 43

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Replace more `String.prototype.contains` with `String.prototype.includes` in JS code.

change for recent JS code in addition to previous Part 2.

Comment 44

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Replace `String.prototype.contains` with `String.prototype.includes` in chrome code. r=till

trivial rebase of previous Part 3.

Comment 45

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Replace more `String.prototype.contains` with `String.prototype.includes` in chrome code.

non-trivial rebase and recent change for chrome code in addition to previous Part 3 (current Part 4)

Comment 46

[Till Schneidereit [:till]]

Comment on attachment 8595756 [details] [diff] [review]
Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

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

F+ instead of r+ because I think it'd be good to change warn-once to use a bitset instead of adding more and more fields to the global.

::: js/src/vm/GlobalObject.h
@@ +107,5 @@
>      static const unsigned FLOAT32X4_TYPE_DESCR    = INTRINSICS + 1;
>      static const unsigned FLOAT64X2_TYPE_DESCR    = FLOAT32X4_TYPE_DESCR + 1;
>      static const unsigned INT32X4_TYPE_DESCR      = FLOAT64X2_TYPE_DESCR + 1;
>      static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
> +    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;

It would be good to combine this with WARNED_PROTO_SETTINGS_SLOW and turn it into a bitset. Then you wouldn't even need to increment the subtrahend and all that, and we could use the same mechanism for lots of other warnings. Perhaps WARNED_ONCE_FLAGS?

Comment 47

[Till Schneidereit [:till]]

Comment on attachment 8595758 [details] [diff] [review]
Part 3: Replace more `String.prototype.contains` with `String.prototype.includes` in JS code.

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

Nice.

Comment 48

[Till Schneidereit [:till]]

Comment on attachment 8595760 [details] [diff] [review]
Part 5: Replace more `String.prototype.contains` with `String.prototype.includes` in chrome code.

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

Nice. Isn't it great that "contains" and "includes" are the same length? :)

Comment 49

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Combine WARNED_* slots in GlobalObject and turn it into a bitset.

Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #46)
> Comment on attachment 8595756 [details] [diff] [review]
> Part 1: Add `String.prototype.includes`; keep `String.prototype.contains`
> around as an alias with a (non-release builds only) warning. r=till
> 
> Review of attachment 8595756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> F+ instead of r+ because I think it'd be good to change warn-once to use a
> bitset instead of adding more and more fields to the global.
> 
> ::: js/src/vm/GlobalObject.h
> @@ +107,5 @@
> >      static const unsigned FLOAT32X4_TYPE_DESCR    = INTRINSICS + 1;
> >      static const unsigned FLOAT64X2_TYPE_DESCR    = FLOAT32X4_TYPE_DESCR + 1;
> >      static const unsigned INT32X4_TYPE_DESCR      = FLOAT64X2_TYPE_DESCR + 1;
> >      static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
> > +    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;
> 
> It would be good to combine this with WARNED_PROTO_SETTINGS_SLOW and turn it
> into a bitset. Then you wouldn't even need to increment the subtrahend and
> all that, and we could use the same mechanism for lots of other warnings.
> Perhaps WARNED_ONCE_FLAGS?

Currently there is 2 WARNED_* slot, so combined them in Part 0.

Comment 50

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

And added flag for String.prototype.contains.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e6dbe90af7

Comment 51

[Till Schneidereit [:till]]

Comment on attachment 8596673 [details] [diff] [review]
Part 0: Combine WARNED_* slots in GlobalObject and turn it into a bitset.

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

The changes to Xdr.h need to move from part 1 to this patch, because this is where the xdr format changes lie. r=me with that + nits addressed.

::: js/src/vm/GlobalObject.h
@@ +120,5 @@
>                    "global object slot counts are inconsistent");
>  
> +    enum WarnOnceFlag : int32_t {
> +        WARN_WATCH_DEPRECATED = 0x00000001,
> +        WARN_PROTO_SETTING_SLOW = 0x00000002

Nit: align the flags, perhaps a few spaces to the right so longer names don't break the alignment.

@@ +128,5 @@
>      // true, then set the slot to true.  Thus calling this method warns once
>      // for each global object it's called on, and every other call does
>      // nothing.
>      static bool
> +    warnOnceAbout(JSContext* cx, HandleObject obj, WarnOnceFlag slot, unsigned errorNumber);

Nit: s/slot/flag/.

Comment 52

[Till Schneidereit [:till]]

Comment on attachment 8596675 [details] [diff] [review]
Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

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

Nice. As just mentioned in the other review, move the Xdr.h changes t part 0. r=me with that.

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/754814561bc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/745c95ce46b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c376c4157a85
https://hg.mozilla.org/integration/mozilla-inbound/rev/39659c916ff9
https://hg.mozilla.org/integration/mozilla-inbound/rev/61e6cb9215a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e81e12215951

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/754814561bc1
https://hg.mozilla.org/mozilla-central/rev/745c95ce46b2
https://hg.mozilla.org/mozilla-central/rev/c376c4157a85
https://hg.mozilla.org/mozilla-central/rev/39659c916ff9
https://hg.mozilla.org/mozilla-central/rev/61e6cb9215a4
https://hg.mozilla.org/mozilla-central/rev/e81e12215951

Comment 55

[Tooru Fujisawa [:arai]]

Sorry, I forgot to address the review comment for alignment and argument name.
I'll land the patch after the tree open.

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5b9aed2c24

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d5b9aed2c24

Comment 58

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6cc6777fbc6e762bf4c29837bc2ff595fcdf0b09
Bug 1102219: Rename String.prototype.contains to String.prototype.includes. r=till

Comment 59

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/includes

Comment 60

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b01262fe8b1a8a8a3f6e1e35fde74673792cb989
Bug 1102219: Rename String.prototype.contains to String.prototype.includes. r=till

Comment 61

[Florian Scholz (Open Web Docs)]

Thanks for the doc updates, :arai!
Site compat has a note as well: https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#String.prototype.contains_was_renamed_to_includes