Break long string literal arguments

[Julow]

#2445 uncovered a mechanism for breaking after a multi-line string literal in a list of argument. This also remove the simulated formatting of expressions to check that they break introduced in #1360

This is hard to work with and easy to break so I suggest using the argument grouping concept based on is_simple instead.
This requires comming up with a rule and will cause regression.

I've made up arbitrary rules that should be improved:

• Heavy syntax strings ({| ... |}) are not trivial and not simple.
• Strings of length 4 or less are trivial.
• Strings of length 19 or less containing only graphical ASCII characters are trivial.
• Strings spanning only one line and that fit the margin are simple.
  "fit the margin" is an arbitrary rule introduced in Improve: breaking of applications with long literal list args #258 and used more widely since Improve: less sensitivity to concrete syntax #767

[Julow]

The regressions are huge! I can identify two main regressions: printf/fprintf style functions and operator chains.

Printf-style function not only look better with the format string on the same line as the function but it's also how it was before:

diff --git a/utils/ccomp.ml b/utils/ccomp.ml
index e21260dcbe..c517712b3d 100644
--- a/utils/ccomp.ml
+++ b/utils/ccomp.ml
@@ -92,7 +92,8 @@ let compile_file ?output ?(opt = "") ?stable_name name =
   in
   let exit =
     command
-      (Printf.sprintf "%s%s %s %s -c %s %s %s %s %s%s"
+      (Printf.sprintf
+         "%s%s %s %s -c %s %s %s %s %s%s"
          (match !Clflags.c_compiler with
          | Some cc -> cc
          | None ->
@@ -135,7 +136,9 @@ let create_archive archive file_list =
     match Config.ccomp_type with
     | "msvc" ->
         command
-          (Printf.sprintf "link /lib /nologo /out:%s %s" quoted_archive
+          (Printf.sprintf
+             "link /lib /nologo /out:%s %s"
+             quoted_archive
              (quote_files ~response_files:true file_list))
     | _ ->
         assert (String.length Config.ar > 0);

There's also a large amount of fprintf ppf "...":

diff --git a/typing/typetexp.ml b/typing/typetexp.ml
index 521777ec95..f0230ed557 100644
--- a/typing/typetexp.ml
+++ b/typing/typetexp.ml
@@ -922,10 +922,12 @@ let report_error env ppf = function
         Style.inline_code name did_you_mean (fun () ->
           Misc.spellcheck in_scope_names name)
   | No_type_wildcards ->
-      fprintf ppf "A type wildcard %a is not allowed in this type declaration."
+      fprintf ppf
+        "A type wildcard %a is not allowed in this type declaration."
         Style.inline_code "_"
   | Undefined_type_constructor p ->
-      fprintf ppf "The type constructor@ %a@ is not yet completely defined"
+      fprintf ppf
+        "The type constructor@ %a@ is not yet completely defined"
         (Style.as_inline_code path)
         p
   | Type_arity_mismatch (lid, expected, provided) ->

I can try a different grouping strategy to support function with a string on the first line.

The is_trivial change had a huge impact on operator chains:

diff --git a/utils/warnings.ml b/utils/warnings.ml
index 812f82a8ed..b5c6cd27f7 100644
--- a/utils/warnings.ml
+++ b/utils/warnings.ml
@@ -1015,7 +1015,8 @@ let message = function
   | Fragile_match "" -> "this pattern-matching is fragile."
   | Fragile_match s ->
       "this pattern-matching is fragile.\n\
-       It will remain exhaustive when constructors are added to type " ^ s ^ "."
+       It will remain exhaustive when constructors are added to type "
+      ^ s ^ "."
   | Ignored_partial_application ->
       "this function application is partial,\nmaybe some arguments are missing."
   | Labels_omitted [] -> assert false
@@ -1027,15 +1028,17 @@ let message = function
   | Method_override [ lab ] -> "the method " ^ lab ^ " is overridden."
   | Method_override (cname :: slist) ->
       String.concat " "
-        ("the following methods are overridden by the class" :: cname :: ":\n "
-       :: slist)
+        ("the following methods are overridden by the class"
+        :: cname :: ":\n " :: slist)
   | Method_override [] -> assert false

This has some advantages and at least fixes an indentation bug but I might have to revert this change.

[gpetiot]

I agree the diff is a clear improvement to me. Should we ask the opinion of the community? @samoht ?

[Julow]

My comment above is not true anymore as I made some changes.
Printf-style functions are now formatted like this:

(happens a lot)

diff --git a/lib/Conf_decl.ml b/lib/Conf_decl.ml
index 031f46ef..fb779cd1 100644
--- a/lib/Conf_decl.ml
+++ b/lib/Conf_decl.ml
@@ -156,12 +156,12 @@ let in_attributes cond = function
 let maybe_empty = function "" -> "" | x -> " " ^ x
 
 let pp_deprecated ppf { dmsg; dversion = v } =
-  Format.fprintf ppf "This option is deprecated since version %a.%s" Version.pp
-    v (maybe_empty dmsg)
+  Format.fprintf ppf "This option is deprecated since version %a.%s"
+    Version.pp v (maybe_empty dmsg)

The formatting of operator chains is mostly unchanged, with a few added breaks when the string is 30 in length or more:

(happens rarely)

diff --git a/facebook-clang-plugins/clang-ocaml/yojson_utils.ml b/facebook-clang-plugins/clang-ocaml/yojson_utils.ml
index cda08a2686..91f8f68579 100644
--- a/facebook-clang-plugins/clang-ocaml/yojson_utils.ml
+++ b/facebook-clang-plugins/clang-ocaml/yojson_utils.ml
@@ -92,9 +92,11 @@ let run_converter_tool reader writer =
   and files = ref [] in
   let add_files x = files := x :: !files
   and usage_msg =
-    "Usage: " ^ Sys.argv.(0) ^ "[OPTIONS] INPUT_FILE [OUTPUT_FILE]\n"
+    "Usage: " ^ Sys.argv.(0)
+    ^ "[OPTIONS] INPUT_FILE [OUTPUT_FILE]\n"
     ^ "Parse yojson values and convert them to another format based on the \
-       extension" ^ " of the output file (default: ${INPUT_FILE}.value.gz).\n"
+       extension"
+    ^ " of the output file (default: ${INPUT_FILE}.value.gz).\n"
   in
   let spec =
     Utils.fix_arg_spec

[samoht]

I'm not sure I like this change - this was a nice oneliner but anymore after this patch.

[gpetiot]

Just noticing that in this case, we are exceding the margin on this line (81 characters), and it seems it's the best way to split the line (in general the closing ) are treated in a way to have them get attached to the expression they close).

[samoht]

I have mixed feelings about this patch.

What I don't like:

• it turns nice one-liners into multi-lines
• the rules seem very arbitrary (and hard to remember/understand for users)

What I kind-of like:

• on some examples (like the printf ppf <string>) it leads to a clearer distinction between the format string and the parameter

So, I'm not sold on that one. What was the rational for the changes at the first place?

[Julow]

Thanks for the review! The intent of this is to make explicit a rule that existed before but was implicit.

I've added more constraint at the same time in case it makes things better as regressions were expected but I'm reverting my changes.
I agree that breaking what was a one-liner is bad.

This is obviously not finished.

[Julow]

This change is no longer needed to fix a bug since #2445 and #2453 but the new formatting is interesting.
I'll keep this open as a draft with the idea to ask for feedback later.

[Julow]

A different formatting for delimited strings has been implemented in #2480, guarded behind an internal option, using a new str_as function.

[Julow]

@gpetiot Thanks, that looks better now. The diff this causes looks good to me but is gigantic. We need to ask for feedback.