repl: no RegExp side effects for static properties

[princejwesley]

Preserve all RegExp static properties

• RegExp.$1 to RegExp.9
• input, lastMatch, lastParen
• leftContext and rightContext

Fixes: #18931

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

repl

cc: @bnoordhuis

[bnoordhuis]

Just food for thought but a simpler solution would be to use RegExp from a different context:

const RE = vm.runInNewContext('RegExp');
// ...
if (RE('pattern').test(input)) {
  // ...
}
// RegExp.$_, RegExp.$_, etc. unaltered here.

[princejwesley]

@bnoordhuis Yes, I thought about it! but we can't use global context (useGlobal flag), right? we may have used RegExp anywhere in our code execution path( I mean, not necessarily in repl.js) and all shares the same static properties.

[bnoordhuis]

@princejwesley vm.runInNewContext() always creates a new context. It won't fix everything but it should be enough to eradicate direct side effects from the REPL. Everything in small steps, right? :-)

[princejwesley]

@bnoordhuis User input from terminal to REPL eval call, we may have many regex calls - for instance emit key functions has to handle varies terminal character combinations with regular expression. if we try to fix side effect from REPL when the useGlobal is true, we suppose to save/restore RegExp states right?

We can create new context and use it inside repl.js but we can't do the same for the whole code execution path (I mean from user key stroke to run code in vm).

Am I wrong? BTW, why do we need useGlobal ? any special use case for it ?

Also whatever we do to avoid side effects is only applicable when useGlobal is true.

[princejwesley]

why don't we set useGlobal as false by default and when the user set it to true, its user's call to accept the side effect, right?

[bnoordhuis]

@princejwesley Sorry, missed your replies.

We can create new context and use it inside repl.js but we can't do the same for the whole code execution path (I mean from user key stroke to run code in vm).

We could if all of core switched over to the new technique. You could start with repl.js, readline.js and tty.js. Cache the result of vm.runInNewContext() though! :-)

why don't we set useGlobal as false by default

It caused ecosystem fallout last time we tried that, IIRC.

[princejwesley]

@bnoordhuis
What about doing the reverse like below?

  // Cache global and contextual RegExp binding 
  const regExpInstances = {
    global: RegExp,
    current: vm.runInNewContext('RegExp', vm.createContext())
  };

  ...
  try {
    const scriptOptions = {
      displayErrors: false,
      breakOnSigint: self.breakEvalOnSigint
    };

    if (self.useGlobal) {
      global.RegExp = regExps.context;	// switch
      result = script.runInThisContext(scriptOptions);
    } else {
      result = script.runInContext(context, scriptOptions);
    }
  } finally {
    if (self.useGlobal) {
      global.RegExp = regExps.global;	// reset
    }
    ...

its not working for regex literal! :(

[bnoordhuis]

That's too much of a hack. Too many ways it can go wrong.

[BridgeAR]

@bnoordhuis are you fine with landing this as is? As I see it, it will already improve the current situation, no matter that the solution is not elegant.

[jdalton]

Is simply deleting these props with Reflect.deleteProperty or setting them to undefined before entering the repl an option?

Update:
Ah, I see it's at every turn of the REPL.

Update:
You could create the regexes of a new context (once) and grab references to them.

[BridgeAR]

I am unsure how to move forward here. Any further recommendations / notes from anyone?

@bnoordhuis would you be fine with landing it as is or do you definitely want this to be reworked?
@princejwesley would you also please take another look?

[bnoordhuis]

isInsideNodeModules() creates a new VM context to do something with error stacks. Since we're already using a different VM context for one thing, we might as well go all the way.

[princejwesley]

I'll start with repl.js, tty.js and readline.js and update the PR.

[ChALkeR]

Perhaps add a new block instead of changing this one?

[jdalton]

Related to #18795 (comment).
@benjamingr will be experimenting with isolating internals in its own context.

[princejwesley]

closing this PR.
New PR: #20549