Add module indent option

[HPRIOR]

Adds indent option for module structs, types and signatures.

[EmileTrotignon]

Thanks for the PR !

The implementation looks very clean, I don't think this will cause a maintenance burden in the future.

I think the option should be named struct-indent, and the documentation should make clear that this affects the inside of sig ... end and struct ... end blocks, currently the explanation is not precise enough. I know struct-indent feels a bit weird because it also affects sig, but there is precedent with break-struct also affecting sig.

The tests could include attributes and comments to check that everything works well when they are present.

dune build @gen_manpage --auto-promote should be run to regenerate the manpage.

There also need to be an entry in CHANGES.md.
The CI will stay red because test-branch is currently broken, but the Build on Linux should become green once the manpage is up to date.

[Julow]

I know struct-indent feels a bit weird because it also affects sig, but there is precedent with break-struct also affecting sig.

I don't think struct-indent is a good name. I understand that module in the option name means struct .. end and sig .. end and I don't think can be misunderstood for other module expressions. struct seems to explicitly exclude signatures, when the option doesn't from its conception.
The way to go for consistency, I think, is to rename the other options instead.

The CI will stay red because test-branch is currently broken

test-branch shouldn't fail the CI, even when there are errors. It should be fixed because it's an extremely important tool.

[Julow]

OCamlformat has been widely used for many years, and I've never seen this option being requested until last month (#2677).
We generally don't want to add options to OCamlformat because it makes the code less consistent and harder to read across the community. It also creates an exponentially increasing number of possible configurations that we can't test and that contain countless bugs.

The implementation looks good and I won't object to merging. I wonder what the community feels about this PR ?

[EmileTrotignon]

It was requested in the past here: #1613

My opinion on introducing options is that it really depends on the option. This one is very simple, should not cause maintenance issues nor bugs.

That is not the case for other options that have a complex implementation and a hard to explain behavior (the worst one of course being ocp-indent-compat, but there are others in between).

[EmileTrotignon]

As for the name, in the discussions leading up the PR, the question of this option affecting functor applications came up, so its at least a little confusing. Also, module excludes sig, because sig .. end is the syntax for module types.

What about struct-sig-indent ?

[HPRIOR]

Thank you for reviewing @Julow.

We generally don't want to add options to OCamlFormat because it makes the code less consistent and harder to read across the community. It also creates an exponentially increasing number of possible configurations that we can't test and that contain countless bugs.

I understand the concern about adding too many options. I think my PR does contribute to this problem. There are several syntax elements that I’ve left out of the implementation because it’s unclear whether they should follow the module-indent setting.

For example, extension nodes:

 module M =
-  [%demo
-  module Foo = Bar
+    [%demo
+    module Foo = Bar

-  type t]
+    type t]

And anonymous modules:

 module Subst = Map.Make (struct
-    type t = string
+        type t = string

-    let compare = compare
-  end)
+        let compare = compare
+    end)

If these aren't covered by module-indent, we’d need separate options like module-extension-node-indent or anon-module-indent, which would add to the option/maintenance bloat.

@Julow would you be supportive of a global indent option instead? Without being complemented by additional options, the current implementation is insufficient for my needs since I'd like all module items to be consistently indented. @EmileTrotignon mentioned in the discussion here #2677 (comment) that there may be some hidden edge cases when increasing the indentation levels above the default. Given the formatter already supports wrapping/breaking at arbitrary widths it seems like the existing logic is capable of supporting the feature otherwise. While it may slightly increase the developement cost where edge cases are found by the minority of users using non-default indentation, it would not cause configuration/option bloat.

I don't think it's worth arguing for the merits of different indentation levels. What matters is that people have strong preferences, and there’s currently no way to reflect them in ocaml. As @ELLIOTTCABLE argued in #1613, even very opinionated formatters like Prettier allow users to choose indentation width. The widespread use of the default isn’t surprising when there’s never been a choice.

[HPRIOR]

Thanks for reviewing @EmileTrotignon,

The tests could include attributes and comments to check that everything works well when they are present.

dune build @gen_manpage --auto-promote should be run to regenerate the manpage.

There also need to be an entry in CHANGES.md. The CI will stay red because test-branch is currently broken, but the Build on Linux should become green once the manpage is up to date.

If I decide to proceed with the PR I'll make these changes, otherwise it's useful to know for the next one.

[EmileTrotignon]

While it may slightly increase the developement cost where edge cases are found by the minority of users using non-default indentation, it would not cause configuration/option bloat.

In my opinion, such a widespread configuration option would be way harder to merge than the current, or even a slightly extented version of the current PR. The reason being that this PR has a defined scope which is very important for a bug-free implementation. To me, the biggest issue with option bloat is the implementation. In this perspective, a small, clearly bugfree PR like is easy to merge, but a PR with bigger changes not so much.

That being said, I understand the appeal of the feature. A plan to get it would be to have more small indentation options
in the future, and a master option to set them all at once (the logic for that already exists in the option for ocp-indent configuration).

[ELLIOTTCABLE]

For whatever it's worth, I strongly support a simple, global tab-width setting that applies to all types of indent. For existing category-specific configuration values, they could override the global default when explicitly set.

That said, my original issue, which this PR addresses, feels like the wrong approach if a global setting is on the table. The bloat of granular settings, as far as I'm concerned, is unnecessary maintenance burden.

If there's good reason not to include a global option, then I do support this PR as a minimal substitute. (=

[EmileTrotignon]

That said, my original issue, which this PR addresses, feels like the wrong approach if a global setting is on the table. The bloat of granular settings, as far as I'm concerned, is unnecessary maintenance burden.

I think a global setting is going to be way harder to implement and therefore maintain. The number of option isn't really what makes the maintenance difficult, having options with a large scope is much worse. That being said, it is indeed a nice feature, and implementing in a maintainable way would probably make the code much cleaner.

If there's good reason not to include a global option, then I do support this PR as a minimal substitute. (=

Its unlikely to be trivial to implement, can be a step in a larger project.

Consider the following code:

  let fun_type_annot c = if ocp c then 2 else 4

  let fun_args c = if ocp c then 6 else 4

  let docked_function_after_fun (c : Conf.t) ~parens ~ctx0 ~ctx =
    match ctx0 with
    | Str _ | Lb _ ->
        (* Cases must be 2-indented relative to the [let], even when
           [let_binding_deindent_fun] is on. *)
        if c.fmt_opts.let_binding_deindent_fun.v then 1 else 0
    | _ when ctx_is_infix ctx0 -> 0
    | _ when ocp c -> (
      match Exp.ctx_is_apply_and_exp_is_arg ~ctx ~ctx0 with
      | Some (_, _, true) -> (* Last argument *) 2
      | _ -> if parens then 3 else 2 )
    | _ -> 2

if parens then 3 else 2 probably is because you want the | to be indented by 2 relative to function and not relative to (function. But how to be sure ? There are probably other place in the code were the indentation is a magic constant computed by keyword length + indentation where the keyword in question is implicit. In some places you might also get indentation - keyword length which is even worse to understand and change correctly.

Or the following option:

--let-binding-deindent-fun
  Deindent a line beginning with `fun`. The flag is set by default.

This option produces and indentation of 1 btw. What is it supposed to do with the global indentation option ?

This is just stuff I had on top of my head, there is most likely multiple other weird spots. To find them you would have to try to implement the option.

All of this is doable, but I don't think its reasonable to do it all at once.

[HPRIOR]

I'm currently working on a provisional PR for a global indent option. There are a few complicated areas, but majority of changes are just a trivial replacement of 2 with a configuration option.

if parens then 3 else 2 probably is because you want the | to be indented by 2 relative to function and not relative to (function. But how to be sure ?

Changing 3 to 2 gives this output in the tests:

 let _ =
   { foo =
       (fun _ -> function
-         | _ ->
-           let _ = 42 in
-           ()
-         | () -> ())
+        | _ ->
+          let _ = 42 in
+          ()
+        | () -> ())
   }
 ;;

In this case the logic would change to:

| _ when ocp c -> (
      match Exp.ctx_is_apply_and_exp_is_arg ~ctx ~ctx0 with
      | Some (_, _, true) -> (* Last argument *) 2
      | _ -> if parens then c.fmt_opts.indent.v + 1 else c.fmt_opts.indent.v )

Another question would be whether changing the default indentation level breaks ocp-indent compatibility. I'm not sure about the rest but I imagine some simple maths around the configured indent level will be required. let_binding_deindent_fun is an interesting option and would need some thought.

All of this is doable, but I don't think its reasonable to do it all at once.

Aren't the tests comprehensive enough to ensure regressions aren't introduced for the default indentation level? Splitting out the indentation options could result is 20/30+ indentation options. Is this desirable?

[EmileTrotignon]

There are a few complicated areas, but majority of changes are just a trivial replacement of 2 with a configuration option.

I am not surprised by that, but the complicated areas might be rabbit holes.
Also some spots have very poor code quality, like the indentation of function is very hard to understand, I would think it would be better to simplify it before making the code more general. Maybe its fine, I haven't tried.

let_binding_deindent_fun is an interesting option and would need some thought.

I don't really get why we need this option, I would rather delete it. Having an indentation of 1 is so weird. Maybe I can start by making the default false instead of true.

Aren't the tests comprehensive enough to ensure regressions aren't introduced for the default indentation level?

Probably not.

Splitting out the indentation options could result is 20/30+ indentation options. Is this desirable?

20 to 30 is too many, maybe there would be a way to group them so that it gets to 10.

Another issue I have with a global option is how do you unsure its complete ? With limited scope option its easy, you can test the whole scope, but if its supposed to be for everything then its more complicated.

Also keep in mind that no one is working on ocamlformat full-time, so a huge PR might just be impossible to review.

[HPRIOR]

Any chance this can be merged as a stopgap between a more comprehensive indentation option @EmileTrotignon @Julow ?

[EmileTrotignon]

I am in favour of merging. The CI needs to be green (maybe a rebase will take care of it), and I will rereview next week to check for anything that might have slipped in.

I think @Julow was reluctant but did not want to veto.

[EmileTrotignon]

I had one issue with documentation, apart from that I am happy with the PR. If @Julow does not object I want to go forward and merge.

[EmileTrotignon]

I don't understand the CI failures, they seem unrelated to this PR, I am merging.