Consistent formatting of module types

[Julow]

This PR refactors the function for formatting module types. It doesn't use the block concept, which is hard to work with and brings bugs that are too hard to solve.
The new function is based on the proven model of ~box ~pro ~epi. The output is much easier to tweak and generally more consistent.

This affects module declarations too.

Some changes are considered bug fixes:

• Less parentheses around some module types.

-module rec A : (sig
+module rec A : sig
   type t
-end
-with type t = int) = struct
+end with type t = int = struct
   type t = int
 end

 module type S = sig
-  include ((module type of M [@foo]) [@foo] with type t := M.t [@foo]) [@@foo]
+  include (module type of M [@foo]) [@foo] with type t := M.t [@foo] [@@foo]

-  and ModSet : (Set.S with type elt = Mod.t) = Set.Make (Mod)
+  and ModSet : Set.S with type elt = Mod.t = Set.Make (Mod)

• Breaking behavior consistent with the rest of the language.

-module type S = functor [@foo] (M : S) -> functor (_ : (module type of M) [@foo]) -> sig end
-                                                                                     [@foo]
+module type S = functor [@foo] (M : S) ->
+  functor (_ : (module type of M) [@foo]) -> sig end [@foo]

• Weird boxing of with constraints. Spotted in Upgrade to ocamlformat 0.26.0 mirage/irmin#2262 (comment)

 module type T = sig
-  module Make
-      (Args : Gc_args.S) : sig
+  module Make (Args : Gc_args.S) : sig
     module Args : Gc_args.S
   end
   with module Args = Args

This PR is not pure refactoring but also bring changes to be discussed:

• Dock the LHS of a with type into the previous line. The with-constraints are less indented.

-module type KV_MAKER = functor (G : Irmin_git.G) (C : Irmin.Contents.S) ->
-  S
-    with type key = string list
-     and type step = string
-     ...
+module type KV_MAKER = functor (G : Irmin_git.G) (C : Irmin.Contents.S) -> S
+  with type key = string list
+   and type step = string
+   ...

 include (
   Ast_407 :
-    module type of struct
-      include Ast_407
-    end
-    with module Location := Ast_407.Location )
+    module type of struct include Ast_407 end
+      with module Location := Ast_407.Location )

 module Make
     (TT : TableFormat.TABLES)
     (ET : EngineTypes.TABLE
-            with type terminal = int
-             and type semantic_value = Obj.t)
+       with type terminal = int
+        and type semantic_value = Obj.t)

• Break after long list of module arguments. There's one module argument per line for easier reading. The same is applied to the type of the module, that would come after the last module argument.

       (Foo : T)
-      (Bar : T) : sig end
+      (Bar : T) :
+  sig end

This can be ambiguous and might require breaking before a struct:

 module Bootstrap
     (MakeH : functor (Element : ORDERED) -> HEAP with module Elem = Element)
-    (Element : ORDERED) : HEAP with module Elem = Element = struct
+    (Element : ORDERED) :
+  HEAP with module Elem = Element = struct
   type t

• Dock only-argument of a module at the same level as for structures.

 module Coerce4 (A : sig
-    val f : int -> int
-  end) =
+  val f : int -> int
+end) =
 struct

• Consistent formatting between module M ... and module type T = functor ....

 module type M = functor
-  (SSSSS : SSSSSSSSSSSSSS)
-  (TTTTT : TTTTTTTTTTTTTTTT) -> sig
+    (SSSSS : SSSSSSSSSSSSSS)
+    (TTTTT : TTTTTTTTTTTTTTTT) -> sig
   val t1 : a

[gpetiot]

Are there regressions blocking this PR? or just the merge? Because the output looks like a net improvement and I would really like to have it merged!

[Julow]

I would like to review the diff one more time.

Since #2440, I plan to not dock empty signatures to avoid breaking the margin.

There's also diffs like that, where I would like to get the community's opinion:

 (** Lift a set to a powerset domain ordered by subset. The elements of the set should be drawn from
     a *finite* collection of possible values, since the widening operator here is just union. *)
-module FiniteSet (Element : PrettyPrintable.PrintableOrderedType) :
-  FiniteSetS with type elt = Element.t
+module FiniteSet (Element : PrettyPrintable.PrintableOrderedType) : FiniteSetS
+  with type elt = Element.t

-module type Make = functor (TransferFunctions : TransferFunctions.SIL) ->
-  S with module TransferFunctions = TransferFunctions
+module type Make = functor (TransferFunctions : TransferFunctions.SIL) -> S
+  with module TransferFunctions = TransferFunctions

test_branch finds a few bugs like this:

diff --git a/test/helpers/test_container.mli b/test/helpers/test_container.mli
index 6dbab81..648c1b1 100644
--- a/test/helpers/test_container.mli
+++ b/test/helpers/test_container.mli
@@ -7,7 +7,8 @@ module Test_S1_allow_skipping_tests (Container : sig
   include Container.S1 with type 'a t := 'a t
 
   val of_list : 'a list -> [ `Ok of 'a t | `Skip_test ]
-end) : sig
+end) :
+  sig
   type 'a t [@@deriving sexp]
 
   include Generic with type ('a, _) t := 'a t