FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

repl: fix wrong parentheses by jenthone · Pull Request #46987 · nodejs/node · GitHub

/ node Public

repl: fix wrong parentheses - #46987

Closed
jenthone wants to merge 10 commits into
nodejs:mainfrom
jenthone:fix_wrong_parentheses
ConversationCommits (10)ChecksFiles changed
Closed

repl: fix wrong parentheses#46987
jenthone wants to merge 10 commits into
nodejs:mainfrom
jenthone:fix_wrong_parentheses

Conversation

jenthone commented Mar 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Fix #46877

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 changed the title Fix wrong parentheses repl: fix wrong parentheses Mar 7, 2023
jenthone marked this pull request as ready for review March 7, 2023 05:48
BridgeAR reviewed Mar 7, 2023
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Mar 7, 2023
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

aduh95 Mar 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
'(': ')',
__proto__: null,
'(': ')',

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

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

aduh95 Mar 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
stack.push(char);
ArrayPrototypePush(stack, char);

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

aduh95 Mar 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (pairs[stack.pop()] !== char) {
if (pairs[ArrayPrototypePop(stack)] !== char) {

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

if (pairs[char]) {

aduh95 Mar 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (pairs[char]) {
if (char in pairs) {

jenthone commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

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

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

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aduh95 reviewed Mar 9, 2023
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Mar 9, 2023 •
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Mar 9, 2023

Copy link
Copy Markdown
Contributor

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

aduh95 reviewed Mar 9, 2023
View reviewed changes
Comment thread test/parallel/test-repl-preview.js Outdated

aduh95 Mar 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

jenthone Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Addressed in 078088d

aduh95 reviewed Mar 9, 2023
View reviewed changes
Comment thread test/parallel/test-repl-preview.js Outdated

aduh95 Mar 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 requested a review from aduh95 March 14, 2023 02:56
jenthone closed this by deleting the head repository Jan 9, 2024

BridgeAR commented Apr 3, 2025

Copy link
Copy Markdown
Member

@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 commented Apr 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 mentioned this pull request Aug 24, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR left review comments
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

Back | FazBrowse Home | New Git URL