Closing `*)` of comments does not respect margin

[jberdine]

With the margin set to 20, we see the following output:

(* 00000011111111112
 * 45678901234567890
 *)

(* 1. xxxx x xxxx *)
(* 2. xxxx xx xxxx *)
(* 3. xxxx xxx xxxx *)
(* 4. xxxx xxxx
   xxxx *)

Note that cases 1. and 4. are handled correctly, but cases 2. and 3. do not respect the margin and should break.

[gpetiot]

I have no idea what's wrong here, the margin is correctly set, and each word of the comment is correctly separated by @ , so the line should be split before the last xxxx word. Setting a different margin for the formatter on-the-fly messes up the whole output (and flushing or not does not help).

[jberdine]

Yes, changing the margin on-the-fly is a no-go, Format has way too much internal state to make that close to feasible.

I suspect that this issue is due to how the comments are wrapped and formatted, and then in Cmts.fmt_cmts we add the ending *) without a break and more importantly without having earlier taken its length into account.

[gpetiot]

Related problem: let's say the margin is limited to 80 characters, it currently only allows to print 79 characters (the 80th being \n), is this intended or do we want to write set_margin (n + 1) to really be able to write 80 characters and then the \n ?

[jberdine]

Do you mean that e.g.:

(* 5. xxxx xxxx xxxx
   xxxx xxxx *)

should be preserved, while currently it formats to:

(* 5. xxxx xxxx
   xxxx xxxx xxxx *)

That does look like a related issue.

This is limited to comments as far as I'm aware, or have you seen other cases?

[gpetiot]

No I mean:

(* 00000011111111112
 * 45678901234567890
 *)

;;
xx xx xx xx xx xx x

;;
xx xx xx xx xx xx xx

formats to:

(* 00000011111111112
 * 45678901234567890
 *)

;;
xx xx xx xx xx xx x

;;
xx xx xx xx xx xx
  xx

[jberdine]

I see, thanks. Some lines do reach the margin, but perhaps that is due to other bugs rather than the newline not being included in the line length, I'm not sure. There is another possible explanation, which involves the Format module's max_indent value. AFAIU, if a box is opened after this indentation, the line is broken, even if the opened box ends up being empty. It does not seem possible to set max_indent to the margin, so ocamlformat sets it to one less. It could be that we bump into that.

[gpetiot]

I know it's not high priority but I'm looking into this issue from time to time. Do you think it could be an issue from the vendored Format we are using? It could be a bug introduced with the fork or a bug in the original code but I didn't find any related bug in the ocaml mantis and I think someone would have noticed.

[jberdine]

It is possible that there are bugs in the patched Format, but I strongly suspect that this bug is in the code we have that ignores the length of *) when wrapping the comments.

[trefis]

I have some examples where ocamlformated code goes beyond 80c, and it's not related to comments:

$ cat lambda/.ocamlformat
profile=conventional
if-then-else=k-r
indicate-multiline-delimiters=closing-on-separate-line
break-cases=all
$ ocamlformat lambda/ocamlformat.ml
let inline_lazy_force_cond arg loc =
  Llet
    ( Strict,
      Pgenval,
      idarg,
      arg,
      Llet
        ( Alias,
          Pgenval,
          tag,
          Lprim (Pccall prim_obj_tag, [ varg ], loc),
          Lifthenelse
            ( Lprim
                ( Pintcomp Ceq,
                  [ Lvar tag; Lconst (Const_base (Const_int Obj.forward_tag)) ],
                  loc ),
              Lprim (Pfield 0, [ varg ], loc),
              Lifthenelse
                ( Lprim
                    ( Pintcomp Ceq,
                      [ Lvar tag; Lconst (Const_base (Const_int Obj.lazy_tag)) ],
                      loc ),
                  Lapply
                    {
                      ap_should_be_tailcall = false;
                      ap_loc = loc;
                      ap_func = force_fun;
                      ap_args = [ varg ];
                      ap_inlined = Default_inline;
                      ap_specialised = Default_specialise;
                    },
                  varg ) ) ) )

[gpetiot]

~I opened this issue: https://github.com/ocaml/ocaml/issues/8854~ So we have indeed boxes without breaks (an automatic check would be great).

[Julow]

This has been fixed by https://github.com/ocaml-ppx/ocamlformat/pull/957

[gpetiot]

I can't check right now but I thought I was still seeing the issue for comment on master.

[Julow]

Indeed, I tested too fast. I get this, which is wrong:

(* 00000011111111112
 * 45678901234567890
 *)

(* 1. xxxx x xxxx *)
(* 2. xxxx xx xxxx *)
(* 3. xxxx xxx xxxx *)
(* 4. xxxx xxxx xxxx *)