Improve printing of non-callback arguments in Pexp_apply with callbacks

Fix #212

Sometimes one of the non-callback arguments will break in a function application where the first or last argument is a callback. There might be a single line comment in there, or a multiline string. We want to break all the arguments in this case for readability.

**before**
```rescript
showDialog(
  `
  Do you really want to leave this workspace?
  Some more text with detailed explanations...
  `, danger=true, ~confirmText="Yes, I am sure!", ~onConfirm={() => ()},
)
```

**after**
```rescript
showDialog(
  `
  Do you really want to leave this workspace?
  Some more text with detailed explanations...
  `,
  ~danger=true,
  ~confirmText="Yes, I am sure!",
  ~onConfirm={() => ()},
)
```

Author: Iwan

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## Unreleased
 
+* Improve printing of non-callback arguments in call expressions with callback in [#241](https://github.com/rescript-lang/syntax/pull/241/files)
 * Fix printing of constrained expressions in rhs of js objects [#240](https://github.com/rescript-lang/syntax/pull/240)
 * Improve printing of trailing comments under lhs of "pipe" expression in [#329](https://github.com/rescript-lang/syntax/pull/239/files)
 * Improve printing of jsx children and props with leading line comment in [#236](https://github.com/rescript-lang/syntax/pull/236)

src/res_printer.ml

@@ -3991,6 +3991,7 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
       lblDoc;
       printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl
     ] in
+    let callback = printComments callback cmtTbl expr.pexp_loc in
     let printedArgs =
       Doc.join ~sep:(Doc.concat [Doc.comma; Doc.line]) (
         List.map (fun arg -> printArgument arg cmtTbl) args
@@ -3999,6 +4000,7 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
     (callback, printedArgs)
   | _ -> assert false
   in
+
   (* Thing.map((arg1, arg2) => MyModuleBlah.toList(argument), foo) *)
   (* Thing.map((arg1, arg2) => {
    *   MyModuleBlah.toList(argument)
@@ -4021,10 +4023,29 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
    * )
    *)
   let breakAllArgs = printArguments ~uncurried args cmtTblCopy in
-  Doc.customLayout [
-    fitsOnOneLine;
-    breakAllArgs;
-  ]
+
+  (* Sometimes one of the non-callback arguments will break.
+   * There might be a single line comment in there, or a multiline string etc.
+   * showDialog(
+   *   ~onConfirm={() => ()},
+   *   `
+   *   Do you really want to leave this workspace?
+   *   Some more text with detailed explanations...
+   *   `,
+   *   ~danger=true,
+   *   // comment   --> here a single line comment
+   *   ~confirmText="Yes, I am sure!",
+   *  )
+   * In this case, we always want the arguments broken over multiple lines,
+   * like a normal function call.
+   *)
+  if Doc.willBreak printedArgs then
+    breakAllArgs
+  else
+    Doc.customLayout [
+      fitsOnOneLine;
+      breakAllArgs;
+    ]
 
 and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
   (* Because the same subtree gets printed twice, we need to copy the cmtTbl.
@@ -4047,13 +4068,19 @@ and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
       ]
     in
     let callbackFitsOnOneLine =
-      printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl in
-    let callbackArgumetnsFitsOnOneLine =
-      printPexpFun ~inCallback:ArgumentsFitOnOneLine expr cmtTblCopy in
+      let pexpFunDoc = printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl in
+      let doc = Doc.concat [lblDoc; pexpFunDoc] in
+      printComments doc cmtTbl expr.pexp_loc
+    in
+    let callbackArgumentsFitsOnOneLine =
+      let pexpFunDoc = printPexpFun ~inCallback:ArgumentsFitOnOneLine expr cmtTblCopy in
+      let doc = Doc.concat [lblDoc; pexpFunDoc] in
+      printComments doc cmtTblCopy expr.pexp_loc
+    in
     (
       Doc.concat (List.rev acc),
-      Doc.concat [lblDoc; callbackFitsOnOneLine],
-      Doc.concat [lblDoc; callbackArgumetnsFitsOnOneLine]
+      callbackFitsOnOneLine,
+      callbackArgumentsFitsOnOneLine
     )
   | arg::args ->
     let argDoc = printArgument arg cmtTbl in
@@ -4090,11 +4117,30 @@ and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
    * )
    *)
   let breakAllArgs = printArguments ~uncurried args cmtTblCopy2 in
-  Doc.customLayout [
-    fitsOnOneLine;
-    arugmentsFitOnOneLine;
-    breakAllArgs;
-  ]
+
+  (* Sometimes one of the non-callback arguments will break.
+   * There might be a single line comment in there, or a multiline string etc.
+   * showDialog(
+   *   `
+   *   Do you really want to leave this workspace?
+   *   Some more text with detailed explanations...
+   *   `,
+   *   ~danger=true,
+   *   // comment   --> here a single line comment
+   *   ~confirmText="Yes, I am sure!",
+   *   ~onConfirm={() => ()},
+   *  )
+   * In this case, we always want the arguments broken over multiple lines,
+   * like a normal function call.
+   *)
+  if Doc.willBreak printedArgs then
+    breakAllArgs
+  else
+    Doc.customLayout [
+      fitsOnOneLine;
+      arugmentsFitOnOneLine;
+      breakAllArgs;
+    ]
 
 and printArguments ~uncurried (args : (Asttypes.arg_label * Parsetree.expression) list) cmtTbl =
   match args with
@@ -4357,23 +4403,20 @@ and printExprFunParameters ~inCallback ~uncurried ~hasConstraint parameters cmtT
     let printedParamaters = Doc.concat [
       if shouldHug || inCallback then Doc.nil else Doc.softLine;
       Doc.join
-        ~sep:(
-          Doc.concat [
-            Doc.comma; if inCallback then Doc.line else Doc.line
-          ]
-        )
+        ~sep:(Doc.concat [Doc.comma; Doc.line])
         (List.map (fun p -> printExpFunParameter p cmtTbl) parameters)
     ] in
     Doc.group (
       Doc.concat [
         lparen;
         if shouldHug || inCallback then
           printedParamaters
-        else Doc.indent printedParamaters;
-        if shouldHug || inCallback then
-          Doc.nil
         else
-          Doc.concat [Doc.trailingComma; Doc.softLine];
+          Doc.concat [
+            Doc.indent printedParamaters;
+            Doc.trailingComma;
+            Doc.softLine;
+          ];
         Doc.rparen;
       ]
     )

tests/printer/expr/__snapshots__/render.spec.js.snap

@@ -1580,6 +1580,81 @@ let /* a */ decoratorTags /* b */ = items->Js.Array2.filter(items => {
   items.category === ChristmasLighting ||
   items.category === Unknown
 })
+
+// callback in last position
+showDialog(
+  \`
+  Do you really want to leave this workspace?
+  Some more text with detailed explanations...
+  \`,
+  ~danger=true,
+  ~confirmText=\\"Yes, I am sure!\\",
+  ~onConfirm={() => ()},
+)
+
+// callback in first position
+showDialog(
+  ~onConfirm={() => ()},
+  ~danger=true,
+  ~confirmText=\\"Yes, I am sure!\\",
+  \`
+  Do you really want to leave this workspace?
+  Some more text with detailed explanations...
+  \`,
+)
+
+// callback in last position, with comment in between args
+showDialog(
+  dialogMessage,
+  // comment
+  ~danger=true,
+  ~confirmText=\\"Yes, I am sure!\\",
+  ~onConfirm=() => (),
+)
+
+// callback in last position, with comment in between args
+showDialog(
+  dialogMessage,
+  ~danger=true,
+  /* comment below */
+  ~confirmText=\\"Yes, I am sure!\\",
+  ~onConfirm=() => (),
+)
+
+// callback in first position, with single line comment in between args
+showDialog(
+  ~onConfirm=() => (),
+  dialogMessage,
+  // comment
+  ~danger=true,
+  ~confirmText=\\"Yes, I am sure!\\",
+)
+
+// callback in first position, with comment in between args
+showDialog(
+  ~onConfirm=() => (),
+  dialogMessage,
+  ~danger=true,
+  /* comment below */
+  ~confirmText=\\"Yes, I am sure!\\",
+)
+
+React.useEffect5(
+  (
+    context.activate,
+    context.chainId,
+    dispatch,
+    setTriedLoginAlready,
+    triedLoginAlready,
+  ),
+  () => {
+    doThings()
+    None
+  }, // intentionally only running on mount (make sure it's only mounted once :))
+)
+
+apply(a, b, c, /* before */ () => () /* after */)
+apply(/* before */ () => () /* after */, a, b, c)
 "
 `;
 

tests/printer/expr/callback.js

@@ -319,3 +319,72 @@ let /* a */ decoratorTags /* b */ = items->Js.Array2.filter(items => {
   || items.category === ChristmasLighting
   || items.category === Unknown
 })
+
+// callback in last position
+showDialog(
+  `
+  Do you really want to leave this workspace?
+  Some more text with detailed explanations...
+  `,
+  ~danger=true,
+  ~confirmText="Yes, I am sure!",
+  ~onConfirm={() => ()},
+)
+
+// callback in first position
+showDialog(
+  ~onConfirm={() => ()},
+  ~danger=true,
+  ~confirmText="Yes, I am sure!",
+  `
+  Do you really want to leave this workspace?
+  Some more text with detailed explanations...
+  `,
+)
+
+// callback in last position, with comment in between args
+showDialog(dialogMessage,
+// comment
+ ~danger=true, ~confirmText="Yes, I am sure!", ~onConfirm=() => ())
+ 
+// callback in last position, with comment in between args
+showDialog(
+  dialogMessage,
+ ~danger=true, 
+ /* comment below */
+ ~confirmText="Yes, I am sure!",
+ ~onConfirm=() => ())
+
+// callback in first position, with single line comment in between args
+showDialog(
+  ~onConfirm=() => (),
+  dialogMessage,
+  // comment
+  ~danger=true,
+  ~confirmText="Yes, I am sure!"
+ )
+ 
+// callback in first position, with comment in between args
+showDialog(
+  ~onConfirm=() => (),
+  dialogMessage,
+  ~danger=true, 
+  /* comment below */
+  ~confirmText="Yes, I am sure!",
+)
+
+React.useEffect5((
+  context.activate,
+  context.chainId,
+  dispatch,
+  setTriedLoginAlready,
+  triedLoginAlready,
+),
+() => {
+  doThings()
+  None
+}, // intentionally only running on mount (make sure it's only mounted once :))
+)
+
+apply(a, b, c, /* before */ () => () /* after */)
+apply(/* before */ () => () /* after */, a, b, c)