Skip to content

repl: fix wrong parentheses #46987

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

repl: fix wrong parentheses #46987

wants to merge 10 commits into from

Conversation

jenthone
Copy link
Contributor

@jenthone jenthone commented Mar 7, 2023 •
edited
Loading

If the input isn't a valid syntax, don't wrap it

Fix #46877

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Mar 7, 2023
@jenthone jenthone changed the title Fix wrong parentheses repl: fix wrong parentheses Mar 7, 2023
@jenthone jenthone marked this pull request as ready for review March 7, 2023 05:48
BridgeAR
BridgeAR reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a good thought, it may have false negatives/positives though: strings, regexp and comments could contain these brackets (probably a couple more). To solve this, we would have to use acorn to check for the right tokens after parsing the input.

aduh95
aduh95 reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's always going to be false negatives and false positives whatever checks we make (e.g. currently using { openingBracket: '{' } as input interprets it as an object pattern, with this change it's going to interpret it as a code block), I don't know if it's worth the effort.

lib/internal/repl/utils.js Outdated
function isValidParentheses(input) {
const stack = [];
const pairs = {
'(': ')',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'(': ')',
__proto__: null,
'(': ')',

lib/internal/repl/utils.js Outdated
const char = input[i];

if (pairs[char]) {
stack.push(char);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stack.push(char);
ArrayPrototypePush(stack, char);

lib/internal/repl/utils.js Outdated
if (pairs[char]) {
stack.push(char);
} else if (char === ')' || char === ']' || char === '}') {
if (pairs[stack.pop()] !== char) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (pairs[stack.pop()] !== char) {
if (pairs[ArrayPrototypePop(stack)] !== char) {

lib/internal/repl/utils.js Outdated
for (let i = 0; i < input.length; i++) {
const char = input[i];

if (pairs[char]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (pairs[char]) {
if (char in pairs) {

@jenthone
Copy link
Contributor Author

jenthone commented Mar 8, 2023

Thank you for your suggestions. Let me try to use acorn.

@jenthone jenthone requested review from BridgeAR and aduh95 and removed request for BridgeAR March 8, 2023 13:56
aduh95
aduh95 reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for {'{':0} and {[Symbol.for("{")]: 0 } please?

aduh95
aduh95 reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test for {},{} and for {} //;? If we end up using acorn anyway, I don’t think checking if the string ends with a semi is still very relevant.
And do we have a test for {throw 0} already?

@jenthone
Copy link
Contributor Author

jenthone commented Mar 9, 2023 •
edited by aduh95
Loading

I don’t think checking if the string ends with a semi is still very relevant.

I think we can't do it because this case will fail:

node/test/parallel/test-repl.js

Line 470 in c733cc0

send: '{ a: 1 }.a;', // { a: 1 }.a;

@aduh95
Copy link
Contributor

aduh95 commented Mar 9, 2023

That doesn’t look like something we can’t workaround tbh, but it’s not a big deal.

aduh95
aduh95 reviewed Mar 9, 2023
View reviewed changes
test/parallel/test-repl-preview.js
'{ [\x1B[32mSymbol({)\x1B[39m]: \x1B[33m0\x1B[39m }',
],
}, {
input: '{},{}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I suggested the wrong test. We would want the following to report a syntax error:

Suggested change
input: '{},{}',
input: '{}),({}',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 078088d

aduh95
aduh95 reviewed Mar 9, 2023
View reviewed changes
test/parallel/test-repl-preview.js Outdated
'{}',
],
}, {
input: '{} //;',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try without the semi? After some more thoughts, I think that’s an even more interesting test case.

Suggested change
input: '{} //;',
input: '{} //',

@jenthone jenthone requested a review from aduh95 March 14, 2023 02:56
@jenthone jenthone closed this by deleting the head repository Jan 9, 2024
@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2025

@jenthone do you mind to recover this change? I just found this PR and I believe it would actually be a great contribution!
I am very sorry that this slipped through. It is sometimes hard to review all PRs properly.

@jenthone
Copy link
Contributor Author

jenthone commented Apr 23, 2025 •
edited
Loading

@jenthone do you mind to recover this change? I just found this PR and I believe it would actually be a great contribution! I am very sorry that this slipped through. It is sometimes hard to review all PRs properly.

I don't work much on the nodejs anymore and also deleted my fork
It seems this PR can't be restored. I think someone can pick my commits and create a new PRs ...

@meixg meixg mentioned this pull request Aug 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrong error report message for expression {})

4 participants

@jenthone @aduh95 @BridgeAR @nodejs-github-bot