Make RegExp#[Symbol.*] methods call exec #411

nicolo-ribaudo · 2018-06-13T18:21:17Z

zloirock · 2018-06-16T18:43:07Z

Sorry, some more days I'll be unavailable. I'll review it later.

zloirock · 2018-06-30T04:54:49Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

      // 21.2.5.8 RegExp.prototype[@@replace](string, replaceValue)
      // 21.2.5.11 RegExp.prototype[@@split](string, limit)
      ? function (string, arg) { return regexMethod.call(string, this, arg); }
      // 21.2.5.6 RegExp.prototype[@@match](string)
      // 21.2.5.9 RegExp.prototype[@@search](string)
      : function (string) { return regexMethod.call(string, this); }
    );
+    // TODO: This line makes the tests fail:
+    //       ReferenceError: Can't find variable: Reflect
+    // hide(RegExp.prototype[SYMBOL], 'name', '[Symbol.' + KEY + ']');


In many engines, functions .name property non-writable non-configurable, so, I think, it would be better just remove it.

zloirock · 2018-06-30T05:09:09Z

packages/core-js/modules/es.string.match.js

+    },
+    // `RegExp.prototype[@@match]` method
+    // https://tc39.github.io/ecma262/#sec-regexp.prototype-@@match
+    function Symbol$match(regexp) {


For better compression, use named functions only when they should have the same explicit .name property (like match above).

zloirock · 2018-06-30T06:19:28Z

tests/helpers/helpers.js

+    };
+    try {
+      return run(assert);
+    } finally {


IIRC in very old IE try / finally does not work without catch.

zloirock · 2018-06-30T06:41:52Z

packages/core-js/internals/advance-string-index.js

@@ -0,0 +1,5 @@
+'use strict';


Please, add a comment that it's AdvanceStringIndex abstract operation and the link to the spec.

zloirock · 2018-06-30T06:50:57Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

@@ -16,13 +16,17 @@ module.exports = function (KEY, length, exec) {
    return ''[KEY](O) != 7;
  })) {


Are you sure that this test also covers all engines without internal execution of .exec method? Maybe makes sense extend it?

zloirock · 2018-06-30T08:36:28Z

packages/core-js/modules/es.string.match.js

+      var A = [];
+      var n = 0;
+      var result;
+      while ((result = rx.exec(S)) !== null) {


In RegExpExec, own .exec should be called only if it's callable. Maybe makes sense to implement RegExpExec abstract operation?

zloirock · 2018-07-02T10:08:17Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

+    var execCalled = false;
+    var re = /a/;
+    re.exec = function () { execCalled = true; return null; };
+    return re[SYMBOL] && (re[SYMBOL](''), execCalled);


It should return a negative result for the existent feature. Use, for example,

re[SYMBOL](''); return !execCalled;

Isn't it faster if it avoid throwing an error?

It's just an example. The main idea of the comment - the result should be inverted.

nicolo-ribaudo · 2018-07-03T21:32:15Z

packages/core-js/internals/fix-regexp-well-known-symbol-logic.js

    var O = {};
    O[SYMBOL] = function () { return 7; };
    return ''[KEY](O) != 7;
+  }) || fails(function () {
+    // Symbol-named RegExp methods call .exec


This test throws an uncaught stack overflow error in the promises tests and I'm not sure why.

nicolo-ribaudo · 2018-08-13T20:04:21Z

@zloirock I manages to make the tests pass. Could you review this PR again?

DmitrySoshnikov · 2018-08-22T08:08:23Z

Hey guys, is this PR good to go yet? Is there still anything blocking?

nicolo-ribaudo · 2018-08-28T10:51:29Z

@zloirock Since Babel uses core-js@2, can I backport this PR?

zloirock · 2018-08-28T18:20:40Z

@nicolo-ribaudo feel free to add the same PR to v2 branch and I'll publish a new core-js@2 version. But let's land this PR at first.

zloirock · 2018-08-28T18:21:48Z

I'll review it today or tomorrow. Could you rebase it?

zloirock · 2018-08-28T18:33:06Z

At first sight, LGTM.

zloirock · 2018-09-25T08:13:10Z

@nicolo-ribaudo RegExp#@@replace correctly handles substitutions tests fails in many browsers - for example, in modern FF. Could you fix it?

zloirock · 2018-09-25T08:34:54Z

’tests/tests.html’25.09.2018, 12:18, "Nicolò Ribaudo" <notifications@github.com>:How do I run the tests in a browser? —You are receiving this because you modified the open/close state.Reply to this email directly, view it on GitHub, or mute the thread.

zloirock · 2018-09-27T10:28:38Z

@nicolo-ribaudo
Chrome 51:

Chrome 50:

nicolo-ribaudo · 2018-09-27T10:32:12Z

I will fix it later.
Just for curiosity, do you have multiple versions of chrome installed on the same machine?

zloirock · 2018-09-27T10:33:56Z

I use Saucelabs and run tests in all available browsers before releases.

zloirock · 2018-09-27T10:50:16Z

IE8:

Safari 9:

nicolo-ribaudo · 2018-11-09T23:06:26Z

I'm working on this now.

EDIT: Ok, I'm downloading windows, ie8, chrome 50 and chrome 51. Hopefully I will be able to debug the issue 😆

nicolo-ribaudo · 2018-11-11T10:46:09Z

@nicolo-ribaudo RegExp#@@replace correctly handles substitutions tests fails in many browsers - for example, in modern FF. Could you fix it?

Which firefox version? I can't reproduce it.

I'm writing down some notes for myself

The tests are failing on IE8 because this code:
```
var re = /a/;
re.exec("a");
re.lastIndex;
```
Should evaluate to 0 (it works in Chrome) but evaluates to 1 in IE8.

Another reason:
```
/^(?:(.)?(.)?)/g.exec(".")[2];
```
should be undefined but returns "".

The tests are failing on Chrome 51 because native split doesn't work with patched
RegExp#exec: these two snippets should produce the same output but they don't.

"ab".split(/a*?/);

> ["a", "b"]

var exec = RegExp.prototype.exec;
RegExp.prototype.exec = function () { return exec.apply(this, arguments); };
"ab".split(/a*?/);

> ["ab"]

The tests are failing on Chrome 50 because the polyfilled RegExp#@@match,
since .exec hasn't been modified, calls String#match assuming that it
won't call RegExp#@@match again (this assumption is wrong).
Safari - TODO

Conclusion: I also need to polyfill .exec 🤦‍♂️

zloirock · 2018-11-11T11:04:40Z

@nicolo-ribaudo

Which firefox version? I can't reproduce it.

IIRC it was fixed in #434

nicolo-ribaudo · 2018-11-12T00:28:47Z

@zloirock In order to fix some IE8 failing tests, I need to fix this IE bug:

> /^(a?)/.exec("")
["", ""] // correct

> /^(a)?/.exec("")
> ["", ""] // wrong, should be ["", undefined]

I don't think that it is possible, unless we re-implement the regexp engine. Could I disable an assertion on IE8?

PS. For now I got it working in Chrome 50 & 51 and I'm trying to install Safari in a VM.

EDIT: Fixing the IE bug is possible

zloirock · 2018-12-04T07:18:35Z

Available in core-js@3.0.0-beta.4.

Make RegExp#[Symbol.*] methods call exec

d09a96e

zloirock added es6 enhancement labels Jun 16, 2018

zloirock reviewed Jun 30, 2018

View reviewed changes

Review

c0a3a5a

dotnetCarpenter mentioned this pull request Jul 2, 2018

Named group is not transformed DmitrySoshnikov/babel-plugin-transform-modern-regexp#7

Open

zloirock reviewed Jul 2, 2018

View reviewed changes

Review

5c4bce9

nicolo-ribaudo commented Jul 3, 2018

View reviewed changes

Fix error

bee5c81

nicolo-ribaudo mentioned this pull request Aug 18, 2018

Add @babel/plugin-transform-named-capturing-groups-regex babel/babel#7105

Merged

Merge branch 'master' into regexp-es2015

5c0cb8f

zloirock merged commit 6addfef into zloirock:master Aug 30, 2018

nicolo-ribaudo deleted the regexp-es2015 branch August 30, 2018 12:35

nicolo-ribaudo mentioned this pull request Sep 4, 2018

Make RegExp#[Symbol.*] methods call exec #428

Merged

nicolo-ribaudo mentioned this pull request Nov 12, 2018

Fix RegExp methods in old browsers #453

Merged

4 tasks

waynelai614 mentioned this pull request Jan 22, 2019

Syntax Regular Expression Error in IE #479

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make RegExp#[Symbol.*] methods call exec #411

Make RegExp#[Symbol.*] methods call exec #411

nicolo-ribaudo commented Jun 13, 2018

zloirock commented Jun 16, 2018

zloirock Jun 30, 2018

zloirock Jun 30, 2018

zloirock Jun 30, 2018

zloirock Jun 30, 2018

zloirock Jun 30, 2018

zloirock Jun 30, 2018 •

edited

Loading

zloirock Jul 2, 2018

nicolo-ribaudo Jul 2, 2018

zloirock Jul 3, 2018

nicolo-ribaudo Jul 3, 2018

nicolo-ribaudo commented Aug 13, 2018

DmitrySoshnikov commented Aug 22, 2018

nicolo-ribaudo commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Sep 25, 2018

zloirock commented Sep 25, 2018 via email

zloirock commented Sep 27, 2018

nicolo-ribaudo commented Sep 27, 2018

zloirock commented Sep 27, 2018

zloirock commented Sep 27, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 9, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 11, 2018 •

edited

Loading

zloirock commented Nov 11, 2018

nicolo-ribaudo commented Nov 12, 2018 •

edited

Loading

zloirock commented Dec 4, 2018

		@@ -16,13 +16,17 @@ module.exports = function (KEY, length, exec) {
		return ''[KEY](O) != 7;
		})) {

Make RegExp#[Symbol.*] methods call exec #411

Make RegExp#[Symbol.*] methods call exec #411

Conversation

nicolo-ribaudo commented Jun 13, 2018

zloirock commented Jun 16, 2018

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

zloirock Jun 30, 2018 • 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

nicolo-ribaudo commented Aug 13, 2018

DmitrySoshnikov commented Aug 22, 2018

nicolo-ribaudo commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Aug 28, 2018

zloirock commented Sep 25, 2018

zloirock commented Sep 25, 2018 via email

zloirock commented Sep 27, 2018

nicolo-ribaudo commented Sep 27, 2018

zloirock commented Sep 27, 2018

zloirock commented Sep 27, 2018 • edited Loading

nicolo-ribaudo commented Nov 9, 2018 • edited Loading

nicolo-ribaudo commented Nov 11, 2018 • edited Loading

zloirock commented Nov 11, 2018

nicolo-ribaudo commented Nov 12, 2018 • edited Loading

zloirock commented Dec 4, 2018

zloirock Jun 30, 2018 •

edited

Loading

zloirock commented Sep 27, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 9, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 11, 2018 •

edited

Loading

nicolo-ribaudo commented Nov 12, 2018 •

edited

Loading