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

Implement String.prototype.replaceAll #6297

Open
mathiasbynens opened this issue Oct 2, 2019 · 6 comments
Open

Implement String.prototype.replaceAll #6297

mathiasbynens opened this issue Oct 2, 2019 · 6 comments

Comments

@mathiasbynens
Copy link
Contributor

The proposal reached stage 3 at today’s TC39 meeting.

Repository: https://github.com/tc39/proposal-string-replaceall

@zenparsing
Copy link
Contributor

Thanks @mathiasbynens, and congrats!

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 8, 2019

I could take this one if wanted?

A preliminary look however shows that to fully satisfy spec for this the implementation of ES6 Regex Symbols needs fixing and enabling though that could be done after the main implementation of this was done.

@zenparsing
Copy link
Contributor

@rhuanjl Of course you can! I've been meaning to do an analysis to determine what work needs to be done to get our RegExp symbols implementation working correctly, but I haven't had the time yet. Do you have some ideas about what work items are left on that?

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 8, 2019

I've not looked at the Regex symbols issue in detail, but I've noted:

  1. they're implemented without spec text so comparing the implementation to spec and checking issues is awkward - additionally they have some built in optimisations (if certain conditions are met use a faster implementation) which make comparing to spec harder.
  2. there are 2 TODOs I've noticed:
    a. To ask for a spec change in @@split - the current implementation intentional deviates from spec in the order it handles parameters apparently for ease of implementation - looking at it there is no functional reason for the change it was just a way of including a potential optimisation without duplicating some code.
    b. To "check all String and Regex built-ins which are inlined in JIT and make sure the helper sets implicit call flag before calling into script also, the corresponding helpers in JnHelperMethodList.h should be marked as being reentrant"
  3. A comment in Chakra cannot override RegExp.prototype.exec property #5187 mentions an implementation error in @@split - possibly related to 2.a above
  4. ES6 RegEx prototype properties also need enabling - unsure what the issue is there, but will likely interact with this for one or more test262 tests.

I figure I should run test 262 with the RegEx symbol flag and see what fails and work on it that way - though also need to look into 2.b above which test262 probably wouldn't hit.

As another related point we don't seem to have an issue for implementing matchAll and the related @@match - which should probably be done alongside replaceAll and RegExSymbols in general.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 12, 2019

Minor update: With the relevant flags passed there are 1036 regex related test failures including both the absense of matchAll and the missing/slightly broken earlier features. I'll endeavour to work through fixing as many of these as I can over the next week or two then aim to submit a PR with those fixes followed by a PR to implement @@matchAll and String#replaceALL and String#matchAll.

EDIT:
Of the 1036 fails:
34 - lookBehind not implemented
26 - matchIndices not implemented (stage 3)
42 - namedGroups not implemented
816 - propertyEscapes not implemented

118 to fix for existing implemented RegExp features + matchAll

@rhuanjl
Copy link
Collaborator

rhuanjl commented Mar 25, 2020

Doing this sensibly is blocked on #6390

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

3 participants