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

Skip trailing comma in explicit partial application #6949

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
- Removed empty line at the end of `switch` statement
- Removed empty `default` case from `switch` statement in the generated code

#### :bug: Bug Fix
- Fix issue where long layout break added a trailing comma in partial application `...`. https://github.com/rescript-lang/rescript-compiler/pull/6949

# 12.0.0-alpha.1

#### :rocket: New Feature
Expand Down
7 changes: 7 additions & 0 deletions jscomp/syntax/src/res_parsetree_viewer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@ let process_partial_app_attribute attrs =
in
process false [] attrs

let has_partial_attribute attrs =
List.exists
(function
| {Location.txt = "res.partial"}, _ -> true
| _ -> false)
attrs

type function_attributes_info = {async: bool; attributes: Parsetree.attributes}

let process_function_attributes attrs =
Expand Down
2 changes: 2 additions & 0 deletions jscomp/syntax/src/res_parsetree_viewer.mli
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ val functor_type :
val process_partial_app_attribute :
Parsetree.attributes -> bool * Parsetree.attributes

val has_partial_attribute : Parsetree.attributes -> bool

type function_attributes_info = {async: bool; attributes: Parsetree.attributes}

(* determines whether a function is async and/or uncurried based on the given attributes *)
Expand Down
28 changes: 21 additions & 7 deletions jscomp/syntax/src/res_printer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Member Author

Choose a reason for hiding this comment

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

For the dummy attribute, I added res.partial so it can be pattern matched with the attribute rather than the label.

Copy link
Member Author

@shulhi shulhi Aug 13, 2024

Choose a reason for hiding this comment

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

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.

in
let attr = (loc, Parsetree.PTyp (Ast_helper.Typ.any ())) in
let dummy =
Ast_helper.Exp.constant ~attrs:[attr] (Ast_helper.Const.int 0)
in
args @ [(Asttypes.Labelled "...", dummy)]
else args
in
Expand Down Expand Up @@ -4730,6 +4736,18 @@ 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 *)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it matters whether to check if ... happens at the end because that's already considered as syntax error.

let has_partial_attr, printed_args =
List.fold_right
(fun arg (flag, acc) ->
let _, expr = arg in
let has_partial_attr =
ParsetreeViewer.has_partial_attribute expr.Parsetree.pexp_attributes
in
let doc = print_argument ~state arg cmt_tbl in
(flag || has_partial_attr, doc :: acc))
args (false, [])
in
Doc.group
(Doc.concat
[
Expand All @@ -4738,13 +4756,9 @@ and print_arguments ~state ?(partial = false)
(Doc.concat
[
Doc.soft_line;
Doc.join
~sep:(Doc.concat [Doc.comma; Doc.line])
(List.map
(fun arg -> print_argument ~state arg cmt_tbl)
args);
Doc.join ~sep:(Doc.concat [Doc.comma; Doc.line]) printed_args;
]);
(if partial then Doc.nil else Doc.trailing_comma);
(if partial || has_partial_attr then Doc.nil else Doc.trailing_comma);
Doc.soft_line;
Doc.rparen;
])
Expand Down
34 changes: 34 additions & 0 deletions jscomp/syntax/tests/printer/expr/apply.res
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,37 @@ f(. {

resolve(.)
resolve(. ())

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c
)

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c,
...
)

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(d, ...),
...
)

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a, ...),
b,
c => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(d),
...
)


let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(d, ...)
)
51 changes: 51 additions & 0 deletions jscomp/syntax/tests/printer/expr/expected/apply.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,54 @@ f({

resolve()
resolve()

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c,
)

let g =
f(
a =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c,
...
)

let g =
f(
a =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(
d,
...
),
...
)

let g =
f(
a =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(
a,
...
),
b,
c =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(d),
...
)

let g = f(
a => LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(a),
b,
c =>
LongModuleName.functionWithAlongNameThatWrapsTheEditorToTheNextLinexxxxxxxxxxxxxxxxxxxxxx(
d,
...
),
)