Bug ? In 0.28.1, nested matchings with begin end have one less indentation level

[voodoos]

In 0.28.1, nested matching with begin end have one less indentation level, even if the begin and match keywords are not on the same line. This means the end keyword is not indented as it should be as a part of the right hand-side of a match branch:

Is that the intended behavior ? It look wrong to me.

`--print-config`

❯ ocamlformat --print-config
comment-check=true
debug=false
disable=false (file .ocamlformat:2)
margin-check=false
max-iters=10
ocaml-version=4.04.0
quiet=false
disable-conf-attrs=false
version-check=true
assignment-operator=end-line
break-before-in=fit-or-vertical
break-cases=fit-or-vertical (file .ocamlformat:4)
break-collection-expressions=fit-or-vertical
break-colon=after
break-fun-decl=wrap
break-fun-sig=wrap
break-infix=wrap
break-infix-before-func=false
break-separators=after
break-sequences=true
break-string-literals=auto
break-struct=force
cases-exp-indent=2 (file .ocamlformat:6)
cases-matching-exp-indent=normal
disambiguate-non-breaking-match=false
doc-comments=before (file .ocamlformat:5)
doc-comments-padding=2
doc-comments-tag-only=default
dock-collection-brackets=false (file .ocamlformat:7)
exp-grouping=preserve (file .ocamlformat:9)
extension-indent=2
field-space=loose
function-indent=2
function-indent-nested=never
if-then-else=compact
indent-after-in=0
indicate-multiline-delimiters=no
indicate-nested-or-patterns=unsafe-no
infix-precedence=indent
leading-nested-match-parens=false
let-and=compact
let-binding-indent=2
let-binding-deindent-fun=true
let-binding-spacing=compact
let-module=compact
line-endings=lf
margin=80
match-indent=0
match-indent-nested=never
max-indent=68
module-indent=2
module-item-spacing=preserve (file .ocamlformat:10)
nested-match=wrap
ocp-indent-compat=false
parens-ite=false
parens-tuple=always
parens-tuple-patterns=multi-line-only
parse-docstrings=false (file .ocamlformat:11)
parse-toplevel-phrases=false
sequence-blank-line=preserve-one
sequence-style=terminator
single-case=compact
space-around-arrays=true
space-around-lists=true
space-around-records=true
space-around-variants=true
stritem-extension-indent=0
type-decl=compact
type-decl-indent=2
wrap-comments=false
wrap-fun-args=true
profile=default

[voodoos]

For reference, here is the (quite large) diff in Merlin: ocaml/merlin#1984

[voodoos]

There are other indentation changes that are a bit surprising, like here:

Or here:

[EmileTrotignon]

Yeah I think its not intended.
The intended formatting was

begin match x with
| ... -> ...
| ... -> ...
end 

Which does not happen in context where begin end already had a special behaviour.

Regarding the two other screenshots, the first is bit surprising, it makes me think of #2706 which is not merged. What is the behaviour without the label ? In my opinion, if the new behaviour only changes the case with a label and makes it closer to the label-less case its an improvement, otherwise it should be reverted.

In the case of the second screenshot, its hard to see on the screen, but doesnt it remove a case of odd numbered indentation ?
I think that change was intended to improve

( match x with
| x -> x )

into

( match x with
  | x -> x )

maybe it spilled over a little.

Could you send text snippets by the way ? Its hard to investigate when there is nothing to copy paste

[EmileTrotignon]

Looking at the surrounding code, this has the advantage that the sub-match has the same indentation as Some (Sig_module (i, pr, m, Trec_not, Exported)).

[EmileTrotignon]

For some weird reason I cant reproduce

Even formatting with 0.27 gives the new behaviour

[voodoos]

Looking at the surrounding code, this has the advantage that the sub-match has the same indentation as Some (Sig_module (i, pr, m, Trec_not, Exported)).

Not sure that this is an advantage, but one thing is sure is that the end is wrongly indented.

For some weird reason I cant reproduce

Ah my bad, we had to live without ocamlformat for a while with the lack of release for 5.4, this must comes from code that had never been formatted, you can simply ignore it.

[voodoos]

Could you send text snippets by the way ? Its hard to investigate when there is nothing to copy paste

For the begin match:

let () =
  match () with
  | () -> begin
    match () with
    | () -> ()
  end

[EmileTrotignon]

This is only an issue with case-exp-indent=2, with the default of 4 its fine (I am guessing because the end kw gets indentation -2 no matter what)

I do believe only the indentation of end should be modified:

let () =
  match () with
  | () -> begin
    match a with
    | () -> ()
  end
  | () -> (
    match b with
    | () -> ())

let c =
  begin match () with
  | () -> begin
    match () with
    | () -> ()
  end
  end

using ( ... ) or begin ... end should not affect indentation here, and its good that it doesnt, but the c block is very ugly.

[voodoos]

Yes, definitely an issue with the end.

[EmileTrotignon]

Are you this is a regression and not a case of un-formatted code ?
with ocamlformat 0.27:

> ocamlformat -g --exp-grouping=preserve test.ml
[@@@ocamlformat "break-cases=fit-or-vertical"]

let () =
  begin
    match () with
    | () -> begin
      match () with
      | () -> ()
    end
  end

This has the same issue with end as the new formatting. Also it seems that --cases-exp-indent does not affect cases inside begin-end, indentation is always 2.

[EmileTrotignon]

I made a series of PRs #2742, #2744 and #2745 which should really improve match () with () -> begin match` in general.

[Julow]

I merged #2742. Do you think we should do a release with some or all of these changes ?

[voodoos]

Just tried it: the diff is still very large, but the end keywords are correctly indented now.
We had a lot (I mean, a lot) of these in Merlin: ocaml/merlin@7327450

So of course we would appreciate a release. But that's also something we can live a bit longer with 🙂