-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
repl: no RegExp side effects for static properties #20549
Conversation
Disallow global RegExp object usage inside repl.js, readline.js and tty.js. Use RegExp from internal context to prevent updating global.RegExp static properties tools/eslint-rules/no-regex-literal-for-repl.js linter is added to prevent regex literal usage in repl code path Fixes: nodejs#18931
I think this is basically what |
@addaleax Adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@princejwesley The PR looks mostly good to me but a) could you elaborate why makeSafe
wouldn't work here? I've only briefly skimmed the related issue.
Also, is there any way to add a test for 1) the eslint rule, and 2) the behaviour of the new code not adding any globals?
I am not a fan of this since AFAIK it takes longer to generate the RegExp like this and it becomes more difficult to read the RegExp. I think we should use a different approach. |
I am not sure how we should progress here. I personally do not think this is the solution to our issue. Is this still WIP? |
ping @princejwesley, will you be working on this or can we close it? |
we can close it |
I looked into this issue and it seems like this is the only halfway reliable way to really solve this. Sorry for originally being sceptical. |
This PR looks mostly fine to me, the only worry I have is that it might introduce performance regressions. Specifically, people sometimes use |
One way to reduce the performance overhead is to create the RegExp once up front and safe it as top level variable. That should be always possible besides for the cases where we have to create the regular expression based upon input and those will also need the constructor right now. I wonder if it's possible to use the primordials RegExp constructor. |
Disallow global RegExp object usage inside repl.js,
readline.js and tty.js. Use RegExp from internal context
to prevent updating global.RegExp static properties
tools/eslint-rules/no-regex-literal-for-repl.js linter is added
to prevent regex literal usage in repl code path
Fixes: #18931
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes