Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String.prototype.replace works incorrectly with RegExp assertion ?= #412

Open
YLeight opened this issue Oct 13, 2016 · 7 comments
Open

String.prototype.replace works incorrectly with RegExp assertion ?= #412

YLeight opened this issue Oct 13, 2016 · 7 comments
Assignees

Comments

@YLeight
Copy link

YLeight commented Oct 13, 2016

In case when we have regular expression with assertion ?=, the wrappedReplaceValue, which decorate the original replaceValue function, can't define correct arguments, because the coincidence which was passed to replaceValue function don't match the RegExp pattern and the final arguments have just only two values - the offset of the matched sub-string within the whole string being examined, and the whole string being examined.
There is a certain problem, when whole RegExp don't match with found coincidence, but in fact, there is a coincidence StringPrototype.replace polyfill

@ljharb
Copy link
Member

ljharb commented Oct 13, 2016

Can you provide specific code (like a specific regex that has a lookahead)?

@YLeight
Copy link
Author

YLeight commented Oct 14, 2016

/(\n)?(^[ \t]*)([*+-]|\d+[.])[ \t]+((\[(x|X| )?])?[ \t]*[^\r]+?(\n{1,2}))(?=\n*(~0|\2([*+-]|\d+[.])[ \t]+))/gm

@YLeight
Copy link
Author

YLeight commented Oct 14, 2016

StringPrototype.replace polyfill
If the searchValue equal /(\n)?(^[ \t]*)([*+-]|\d+[.])[ \t]+((\[(x|X| )?])?[ \t]*[^\r]+?(\n{1,2}))(?=\n*(~0|\2([*+-]|\d+[.])[ \t]+))/gm and match equal '* some list item;' we get null in 1892 code line but not the groups.

@ljharb
Copy link
Member

ljharb commented Oct 14, 2016

var str = '* some list item;';
var re = /(\n)?(^[ \t]*)([*+-]|\d+[.])[ \t]+((\[(x|X| )?])?[ \t]*[^\r]+?(\n{1,2}))(?=\n*(~0|\2([*+-]|\d+[.])[ \t]+))/gm;
[str.match(re), re.exec(str), str.replace(re, function () { console.log(arguments); })]

in the native browser console implies there should be no match. Can you provide a specific test that passes on a compliant browser but fails on a browser where the es5-shim has polyfilled String#replace?

@YLeight
Copy link
Author

YLeight commented Oct 17, 2016

I mean next case:

var reg = /(\n)?(^[ \t]*)([*+-]|\d+[.])[ \t]+((\[(x|X| )?])?[ \t]*[^\r]+?(\n{1,2}))(?=\n*(~0|\2([*+-]|\d+[.])[ \t]+))/gm;
var str = "+ first item in list;\n\n+ second item in list;\n~0";

str.replace(reg, function () {
    console.log(arguments.length);
});

For example, firefox version 24:

  • with es5-shim: arguments.length will be 2;
  • without es5-shim: arguments.length will be 12;

@ljharb
Copy link
Member

ljharb commented Oct 17, 2016

Thanks, are there specific browsers you're getting 2 in?

@ljharb ljharb self-assigned this Oct 17, 2016
@YLeight
Copy link
Author

YLeight commented Oct 17, 2016

I am shure, firefox version 24 is one of them.
And I assume, all firefox versions untill 33 act the same.
Gecko-specific notes (in bottom of article)
RegExp.$N mozilla bug - checking in

YLeight pushed a commit to YLeight/es5-shim that referenced this issue Oct 21, 2016
Pass arguments to the original function
YLeight pushed a commit to YLeight/es5-shim that referenced this issue Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants