Skip trailing comma in explicit partial application #6949
Conversation
shulhi
force-pushed
the
fix-trailing-comma-in-partial-function-dotdotdot
branch
from
9aa6b1a to
b8428cf
Compare
| @@ -4730,6 +4730,20 @@ and print_arguments ~state ?(partial = false) | |||
| in | |||
| Doc.concat [Doc.lparen; arg_doc; Doc.rparen] | |||
| | args -> | |||
| (* Avoid printing trailing comma when there is ... in function application *) | |||
There was a problem hiding this comment.
I don't think it matters whether to check if ... happens at the end because that's already considered as syntax error.
|
I think this looks good. @cristianoc any comments? |
jscomp/syntax/src/res_printer.ml
Outdated
| @@ -4730,6 +4730,20 @@ and print_arguments ~state ?(partial = false) | |||
| in | |||
| Doc.concat [Doc.lparen; arg_doc; Doc.rparen] | |||
| | args -> | |||
| (* Avoid printing trailing comma when there is ... in function application *) | |||
| let hasDotDotDot, printed_args = | |||
There was a problem hiding this comment.
would you do a little search if this functionality already exists somewhere else?
I kind of don't even remember using ... verbatim as label, instead of some other attribute.
There was a problem hiding this comment.
This is what I see in the parser let attrs = if is_partial then [res_partial_attr] else [] in
so I'm not sure your code runs? In any case, adding a test will answer this.
There was a problem hiding this comment.
I wonder which is better, pattern match on the label or the expression like so,
There was a problem hiding this comment.
The label does not exist. In the ast.
There was a problem hiding this comment.
Think of it as app1(f, args) vs app2(f, args)
One being total and the other partial.
Internally the distinction is only the presence of the attribute.
There was a problem hiding this comment.
So "..." only exists as string in the source, but that's not how it's represented in the AST.
| @@ -4146,7 +4146,13 @@ and print_pexp_apply ~state expr cmt_tbl = | |||
| let partial, attrs = ParsetreeViewer.process_partial_app_attribute attrs in | |||
| let args = | |||
| if partial then | |||
| let dummy = Ast_helper.Exp.constant (Ast_helper.Const.int 0) in | |||
| let loc = | |||
| {Asttypes.txt = "res.partial"; Asttypes.loc = expr.pexp_loc} | |||
There was a problem hiding this comment.
For the dummy attribute, I added res.partial so it can be pattern matched with the attribute rather than the label.
There was a problem hiding this comment.
Looking at previous code, it doesn't look like the choice of the expression here matters. It only cares about the label, so tagging partial attribute here allows us to pattern match on the attribute rather than the label.
|
@shulhi Could you add a CHANGELOG entry? |
Done. Should we also backport this to v11? |
Yes, I think so. I will do cherry picking to v11 in bulk at a later time. |
* Check for dotdotdot in args * Check on attribute rather than label * Add tests * Fix naming convention * Update CHANGELOG
* Check for dotdotdot in args * Check on attribute rather than label * Add tests * Fix naming convention * Update CHANGELOG
* Check for dotdotdot in args * Check on attribute rather than label * Add tests * Fix naming convention * Update CHANGELOG
Fix #6948