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

Subclassing RegExp makes no progress if the global flag is set. #2277

Open
mgaudet opened this issue Jan 15, 2021 · 4 comments
Open

Subclassing RegExp makes no progress if the global flag is set. #2277

mgaudet opened this issue Jan 15, 2021 · 4 comments

Comments

@mgaudet
Copy link

mgaudet commented Jan 15, 2021

Consider the following test case:

class GlobalRegExp extends RegExp { global = true; }
let greg = new GlobalRegExp("hi");
"hi".match(greg);

On the three engines I have tried (SM, V8, JSC), this produces out-of-memory situations unexpectedly.

I've been investigating the subclassing removal proposal. This failure seems to be an error in the implementation of Type IV subclassing of RegExp.

Specifically, the @@match method uses Type IV subclassing to decide if we're a global regexp; and so loops over the input. However, RegExpBuiltinExec does not use Type IV subclassing, and instead uses [[OriginalFlags]]. As such the algorithms are getting confused around lastIndex. Specifically, 22.2.5.2.2 Step 15.a of RegExpBuiltinExec isn't run, and so as a result, @@match simply loops forever with the same lastIndex.

It appears that the change to RegExpBuiltinExec in order to use [[OriginalFlags]] was done explicitly in 2016, to resolve an issue, but I think that resolution ends up being the root cause of this one.

@ljharb
Copy link
Member

ljharb commented Jan 15, 2021

This is because your subclass is incorrect. You need to either call super() so the internal slots are set properly, or you need to also override the Symbol.match method (and presumably, the other Symbol methods).

In other words, a simpler subclass that does what you expect might be:

class GlobalRegExp extends RegExp {
  constructor(source, flags = 'g') {
    super(source, String(flags || '').replace(/g/g, 'g'));
  }
}

@mgaudet
Copy link
Author

mgaudet commented Jan 18, 2021

So this isn't about building a successful implementation with the spec we have, but rather ensuring the specification is consistent and cohesive around this subclassing type.

To review a couple of points:

  1. Internal slots on that class are already set (there's an implicit constructor(...args) { super(...args); } associated with this class )

  2. Without digging too far into the history, there's a partial support for subclassing in RegExp. The root cause of this bug is that the partial support is logically splinched, producing an unexpected infinite loop by specification. The complexity behind this partial support is part of the justification for the rm-builtin-subclassing proposal.

    As an example of how support is partial,Regexp.prototype.flags is defined as a Get of the various flag names; yet, in other cases internal algorithms refer directly to the [[OriginalFlags]] internal slot.

The inconsistency of use between Get(rx, "global") and "g" in [[OriginalFlags]] is the root cause of this bug.

A bandaid fix for this specific bug would be to change @@match to use [[OriginalFlags]] to be consistent with RegExpBuiltinExec.

Having said that, I suspect similar problems exist around the fullUnicode and sticky flags as well, and have not fully audited them.

@codehag
Copy link
Contributor

codehag commented Jan 18, 2021

@syg Regarding the rm-subclassing proposal, I think this is an interesting case to look at.

This might also be interesting to @msaboff?

@aduh95
Copy link
Contributor

aduh95 commented Aug 2, 2021

It's also causing infinite loop and OOM errors if we try to overwrite the built-in getter:

Object.defineProperty(RegExp.prototype, 'global', { value: true });
'foo'.replace(/o/, 'a')

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

4 participants