Consistent break after string constant argument (#2453)

A break is added after wrapping string constants in argument lists. This
break was missing for string constants containing explicit line breaks
or format hints.

Author: Julow

CHANGES.md

@@ -15,6 +15,7 @@ profile. This started with version 0.26.0.
 
 - Remove trailing space inside a wrapping empty signature (#2443, @Julow)
 - Fix extension-point spacing in structures (#2450, @Julow)
+- \* Consistent break after string constant argument (#2453, @Julow)
 
 ## 0.26.1 (2023-09-15)
 

lib/Fmt_ast.ml

@@ -1417,7 +1417,7 @@ and fmt_fun ?force_closing_paren
     $ body $ closing
     $ Cmts.fmt_after c ast.pexp_loc )
 
-and fmt_label_arg ?(box = true) ?epi ?eol c (lbl, ({ast= arg; _} as xarg)) =
+and fmt_label_arg ?(box = true) ?eol c (lbl, ({ast= arg; _} as xarg)) =
   match (lbl, arg.pexp_desc) with
   | (Labelled l | Optional l), Pexp_ident {txt= Lident i; loc}
     when String.equal l.txt i && List.is_empty arg.pexp_attributes ->
@@ -1435,23 +1435,21 @@ and fmt_label_arg ?(box = true) ?epi ?eol c (lbl, ({ast= arg; _} as xarg)) =
         | Optional _ -> str "?"
         | Nolabel -> noop
       in
-      lbl $ fmt_expression c ~box ?epi xarg
+      lbl $ fmt_expression c ~box xarg
   | (Labelled _ | Optional _), _ when Cmts.has_after c.cmts xarg.ast.pexp_loc
     ->
       let cmts_after = Cmts.fmt_after c xarg.ast.pexp_loc in
       hvbox_if box 2
         ( hvbox_if box 0
-            (fmt_expression c
-               ~pro:(fmt_label lbl ":@;<0 2>")
-               ~box ?epi xarg )
+            (fmt_expression c ~pro:(fmt_label lbl ":@;<0 2>") ~box xarg)
         $ cmts_after )
   | (Labelled _ | Optional _), (Pexp_fun _ | Pexp_newtype _) ->
       fmt_fun ~box ~label:lbl ~parens:true c xarg
   | _ ->
       let label_sep : s =
         if box || c.conf.fmt_opts.wrap_fun_args.v then ":@," else ":"
       in
-      fmt_label lbl label_sep $ fmt_expression c ~box ?epi xarg
+      fmt_label lbl label_sep $ fmt_expression c ~box xarg
 
 and expression_width c xe =
   String.length
@@ -1467,15 +1465,15 @@ and fmt_args_grouped ?epi:(global_epi = noop) c ctx args =
       | Pexp_fun _ | Pexp_function _ -> Some false
       | _ -> None
     in
-    let epi =
-      match (lbl, last) with
-      | _, true -> None
-      | Nolabel, _ -> Some (fits_breaks "" ~hint:(1000, -1) "")
-      | _ -> Some (fits_breaks "" ~hint:(1000, -3) "")
+    let break_after =
+      match (ast.pexp_desc, c.conf.fmt_opts.break_string_literals.v) with
+      | Pexp_constant _, `Auto when not last ->
+          fits_breaks "" ~hint:(1000, -2) ""
+      | _ -> noop
     in
     hovbox
       (Params.Indent.fun_args_group c.conf ~lbl ast)
-      (fmt_label_arg c ?box ?epi (lbl, xarg))
+      (fmt_label_arg c ?box (lbl, xarg) $ break_after)
     $ fmt_if_k (not last) (break_unless_newline 1 0)
   in
   let fmt_args ~first ~last args =

test/passing/tests/break_string_literals-never.ml.err

@@ -3,3 +3,5 @@ Warning: tests/break_string_literals.ml:7 exceeds the margin
 Warning: tests/break_string_literals.ml:11 exceeds the margin
 Warning: tests/break_string_literals.ml:48 exceeds the margin
 Warning: tests/break_string_literals.ml:51 exceeds the margin
+Warning: tests/break_string_literals.ml:63 exceeds the margin
+Warning: tests/break_string_literals.ml:68 exceeds the margin

test/passing/tests/break_string_literals-never.ml.ref

@@ -58,3 +58,13 @@ let _ = "abc@,def\n\n ghi"
 let _ = "abc@,def\n\n"
 
 let _ = "abc@,def@\n\n"
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s\nPermitted values: if-exists always never\nDefault: %s"
+    var v (to_string default)
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s Permitted values: if-exists always never Default: %s"
+    var v (to_string default)

test/passing/tests/break_string_literals.ml

@@ -69,3 +69,17 @@ let _ = "abc@,def\n\nghi"
 let _ = "abc@,def\n\n ghi"
 let _ = "abc@,def\n\n"
 let _ = "abc@,def@\n\n"
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s\n\
+     Permitted values: if-exists always never\n\
+     Default: %s"
+    var v (to_string default)
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s \
+     Permitted values: if-exists always never \
+     Default: %s"
+    var v (to_string default)

test/passing/tests/break_string_literals.ml.ref

@@ -92,3 +92,16 @@ let _ = "abc@,def\n\n ghi"
 let _ = "abc@,def\n\n"
 
 let _ = "abc@,def@\n\n"
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s\n\
+     Permitted values: if-exists always never\n\
+     Default: %s"
+    var v (to_string default)
+
+let _ =
+  Pp.textf
+    "Failed to parse environment variable: %s=%s Permitted values: \
+     if-exists always never Default: %s"
+    var v (to_string default)

test/passing/tests/js_args.ml.ref

@@ -33,7 +33,8 @@ let () =
   messages :=
     Message_store.create (Session_id.of_string "")
       (* Tuareg indents these lines too far to the left. *)
-      "herd-retransmitter" Message_store.Message_size.Byte
+      "herd-retransmitter"
+      Message_store.Message_size.Byte
 
 let () =
   raise