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

Adds support for trailing commas in function parameters #1416

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

p-bakker
Copy link
Collaborator

@p-bakker p-bakker commented Nov 4, 2023

As the title says

BTW think this ought to be a squashed merge

@p-bakker p-bakker added this to the ES2017 milestone Nov 4, 2023
@@ -6151,8 +6145,8 @@ language/statements/generators 220/259 (84.94%)
param-dflt-yield.js {unsupported: [default-parameters]}
params-dflt-args-unmapped.js {unsupported: [default-parameters]}
params-dflt-ref-arguments.js {unsupported: [default-parameters]}
params-trailing-comma-multiple.js
params-trailing-comma-single.js
params-trailing-comma-multiple.js non-interpreted
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean it fails in compiled mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The testcase fails due to #1412 now, but the params-trailing-comma part is fixed

@tuchida
Copy link
Contributor

tuchida commented Nov 5, 2023

Do you want ast.toSource() to change?

sb.append("(");
printList(params, sb);
sb.append(") ");

@p-bakker p-bakker force-pushed the fix-trailing-comma-in-parameters branch from 20aebd4 to 7072014 Compare November 6, 2023 09:37
@p-bakker
Copy link
Collaborator Author

p-bakker commented Nov 6, 2023

Do you want ast.toSource() to change?

I guess we should? Added it now, but done so by 'tagging' the FunctionNode with an int property, but I wonder if I should instead add some getter/setter combo on FunctionNode. Thoughts?

@p-bakker p-bakker force-pushed the fix-trailing-comma-in-parameters branch from 7072014 to 3be3add Compare November 6, 2023 10:47
@tuchida
Copy link
Contributor

tuchida commented Nov 6, 2023

Thank you! Looking good!

I guess we should? Added it now, but done so by 'tagging' the FunctionNode with an int property, but I wonder if I should instead add some getter/setter combo on FunctionNode. Thoughts?

As I read the code, TRAILING_COMMA flag is set to FunctionNode and ParenthesizedExpression. So I think the current code is fine.

@p-bakker
Copy link
Collaborator Author

p-bakker commented Nov 6, 2023

As I read the code, TRAILING_COMMA flag is set to FunctionNode and ParenthesizedExpression. So I think the current code is fine.

Tnx.

Just FYI: 'TRAILING_COMMA' is only set on ParenthesizedExpression to carry it forward to where the ParenthesizedExpression is used to construct the params of an Arrow function

src/org/mozilla/javascript/Parser.java Show resolved Hide resolved
@@ -3160,16 +3171,20 @@ private AstNode parenExpr() throws IOException {
Comment jsdocNode = getAndResetJsDoc();
int lineno = ts.lineno;
int begin = ts.tokenBeg;
AstNode e = (peekToken() == Token.RP ? new EmptyExpression(begin) : expr());
AstNode e = (peekToken() == Token.RP ? new EmptyExpression(begin) : expr(true));
Copy link
Collaborator

Choose a reason for hiding this comment

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

To my points above, if this is the only place where we pass "true," does it make more sense to add a new function instead?

@gbrail gbrail merged commit ef18cb9 into mozilla:master Nov 14, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants