Skip to content

Commit 5efba0c

Browse files
JulowJulow
authored
Fix dropped comment in 'if then begin .. end' (#2734)
* Fix dropped comment in 'if then begin .. end' * Update CHANGES
1 parent 43cfa9b commit 5efba0c

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

57 files changed

+427
-19
lines changed

‎CHANGES.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ profile. This started with version 0.26.0.
1010

1111
- Fix crash with tuples of the form `~lbl:(1 + 2)` (#2732, #2733, @cod1r, @Julow)
1212

13+
- Fix dropped comment in `if then (* comment *) begin .. end` (#2734, @Julow)
14+
1315
## 0.28.0
1416

1517
### Highlight

‎lib/Fmt_ast.ml‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2550,16 +2550,22 @@ and fmt_expression c ?(box = true) ?(pro = noop) ?eol ?parens
25502550
~fmt_cond:(fmt_expression ~box:false c)
25512551
~cmts_before_kw ~cmts_after_kw
25522552
in
2553+
let wrap_beginend =
2554+
match p.beginend_loc with
2555+
| Some loc -> Cmts.fmt c loc
2556+
| None -> Fn.id
2557+
in
25532558
parens_prev_bch := parens_bch ;
25542559
p.box_branch
25552560
( p.cond
25562561
$ p.box_keyword_and_expr
25572562
( p.branch_pro
2558-
$ p.wrap_parens
2559-
( fmt_expression c ?box:p.box_expr
2560-
~parens:false ?pro:p.expr_pro
2561-
?eol:p.expr_eol p.branch_expr
2562-
$ p.break_end_branch ) ) )
2563+
$ wrap_beginend
2564+
(p.wrap_parens
2565+
( fmt_expression c ?box:p.box_expr
2566+
~parens:false ?pro:p.expr_pro
2567+
?eol:p.expr_eol p.branch_expr
2568+
$ p.break_end_branch ) ) ) )
25632569
$ fmt_if (not last) p.space_between_branches ) ) )
25642570
$ fmt_atrs ) )
25652571
| Pexp_let (lbs, body, loc_in) ->

‎lib/Params.ml‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -816,6 +816,7 @@ type if_then_else =
816816
; box_keyword_and_expr: Fmt.t -> Fmt.t
817817
; branch_pro: Fmt.t
818818
; wrap_parens: Fmt.t -> Fmt.t
819+
; beginend_loc: Location.t option
819820
; box_expr: bool option
820821
; expr_pro: Fmt.t option
821822
; expr_eol: Fmt.t option
@@ -827,17 +828,21 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
827828
~parens_prev_bch ~xcond ~xbch ~expr_loc ~fmt_infix_ext_attrs
828829
~infix_ext_attrs ~fmt_cond ~cmts_before_kw ~cmts_after_kw =
829830
let imd = c.fmt_opts.indicate_multiline_delimiters.v in
830-
let beginend, infix_ext_attrs_beginend, branch_expr =
831+
let beginend_loc, infix_ext_attrs_beginend, branch_expr =
831832
let ast = xbch.Ast.ast in
832833
match ast with
833834
| { pexp_desc= Pexp_beginend (nested_exp, infix_ext_attrs)
834835
; pexp_attributes= []
836+
; pexp_loc
835837
; _ } ->
836-
(true, Some infix_ext_attrs, sub_exp ~ctx:(Exp ast) nested_exp)
837-
| _ -> (false, None, xbch)
838+
( Some pexp_loc
839+
, Some infix_ext_attrs
840+
, sub_exp ~ctx:(Exp ast) nested_exp )
841+
| _ -> (None, None, xbch)
838842
in
843+
let has_beginend = Option.is_some beginend_loc in
839844
let wrap_parens ~wrap_breaks k =
840-
if beginend then
845+
if has_beginend then
841846
let infix_ext_attrs_beginend =
842847
Option.value_exn infix_ext_attrs_beginend
843848
in
@@ -848,15 +853,15 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
848853
in
849854
let get_parens_breaks ~opn_hint_indent ~cls_hint:(ch_sp, ch_sl) =
850855
let brk hint = fits_breaks "" ~hint "" in
851-
let oh_other = ((if beginend then 1 else 0), opn_hint_indent) in
852-
if beginend then
856+
if has_beginend then
853857
let _, offset = ch_sl in
854-
wrap (brk oh_other) (break 1000 offset)
858+
wrap (brk (1, opn_hint_indent)) (break 1000 offset)
855859
else
856860
match imd with
857861
| `Space -> wrap (brk (1, opn_hint_indent)) (brk ch_sp)
858-
| `No -> wrap (brk oh_other) noop
859-
| `Closing_on_separate_line -> wrap (brk oh_other) (brk ch_sl)
862+
| `No -> wrap (brk (0, opn_hint_indent)) noop
863+
| `Closing_on_separate_line ->
864+
wrap (brk (0, opn_hint_indent)) (brk ch_sl)
860865
in
861866
let cond () =
862867
match xcond with
@@ -875,7 +880,7 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
875880
in
876881
let branch_pro ?(indent = 2) () =
877882
if Option.is_some cmts_after_kw then break 1000 indent
878-
else if beginend || parens_bch then str " "
883+
else if has_beginend || parens_bch then str " "
879884
else break 1 indent
880885
in
881886
match c.fmt_opts.if_then_else.v with
@@ -889,6 +894,7 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
889894
~wrap_breaks:
890895
(get_parens_breaks ~opn_hint_indent:0
891896
~cls_hint:((1, 0), (1000, -2)) )
897+
; beginend_loc
892898
; box_expr= Some false
893899
; expr_pro= None
894900
; expr_eol= None
@@ -901,13 +907,15 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
901907
; box_keyword_and_expr= Fn.id
902908
; branch_pro= branch_pro ()
903909
; wrap_parens= wrap_parens ~wrap_breaks:(wrap (break 1000 2) noop)
904-
; box_expr= Some beginend
910+
; beginend_loc
911+
; box_expr= Some has_beginend
905912
; expr_pro= None
906913
; expr_eol= Some (break 1 2)
907914
; branch_expr
908915
; break_end_branch=
909-
fmt_if (parens_bch || beginend || not last) (break 1000 0)
910-
; space_between_branches= fmt_if (beginend || parens_bch) (str " ") }
916+
fmt_if (parens_bch || has_beginend || not last) (break 1000 0)
917+
; space_between_branches= fmt_if (has_beginend || parens_bch) (str " ")
918+
}
911919
| `Fit_or_vertical ->
912920
{ box_branch=
913921
hovbox
@@ -922,6 +930,7 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
922930
~wrap_breaks:
923931
(get_parens_breaks ~opn_hint_indent:2
924932
~cls_hint:((1, 0), (1000, 0)) )
933+
; beginend_loc
925934
; box_expr= Some false
926935
; expr_pro=
927936
Some
@@ -933,7 +942,8 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
933942
; break_end_branch= noop
934943
; space_between_branches=
935944
( match imd with
936-
| `Closing_on_separate_line when beginend || parens_bch -> str " "
945+
| `Closing_on_separate_line when has_beginend || parens_bch ->
946+
str " "
937947
| _ -> space_break ) }
938948
| `Vertical ->
939949
{ box_branch= Fn.id
@@ -945,6 +955,7 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
945955
~wrap_breaks:
946956
(get_parens_breaks ~opn_hint_indent:2
947957
~cls_hint:((1, 0), (1000, 0)) )
958+
; beginend_loc
948959
; box_expr= None
949960
; expr_pro= Some (break_unless_newline 1000 2)
950961
; expr_eol= None
@@ -979,6 +990,7 @@ let get_if_then_else (c : Conf.t) ~pro ~first ~last ~parens_bch
979990
~wrap_breaks:
980991
(get_parens_breaks ~opn_hint_indent:0
981992
~cls_hint:((1, 0), (1000, -2)) )
993+
; beginend_loc
982994
; box_expr= None
983995
; expr_pro= None
984996
; expr_eol= None

‎lib/Params.mli‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,8 @@ type if_then_else =
207207
; box_keyword_and_expr: Fmt.t -> Fmt.t
208208
; branch_pro: Fmt.t
209209
; wrap_parens: Fmt.t -> Fmt.t
210+
; beginend_loc: Location.t option
211+
(** Location of the [beign..end] node, if any, for placing comments. *)
210212
; box_expr: bool option
211213
; expr_pro: Fmt.t option
212214
; expr_eol: Fmt.t option

‎test/passing/refs.ahrefs/ite-compact.ml.ref‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,3 +175,10 @@ let compare s1 s2 =
175175

176176
let _ = if x then 42 (* dummy *) else y
177177
let _ = if x then 42 (* dummy *) else if y then z else w
178+
179+
let () =
180+
if true then (* a *)
181+
begin
182+
()
183+
end
184+
else ()

‎test/passing/refs.ahrefs/ite-compact_closing.ml.ref‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,3 +192,10 @@ let compare s1 s2 =
192192

193193
let _ = if x then 42 (* dummy *) else y
194194
let _ = if x then 42 (* dummy *) else if y then z else w
195+
196+
let () =
197+
if true then (* a *)
198+
begin
199+
()
200+
end
201+
else ()

‎test/passing/refs.ahrefs/ite-fit_or_vertical.ml.ref‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,3 +210,11 @@ let compare s1 s2 =
210210

211211
let _ = if x then 42 (* dummy *) else y
212212
let _ = if x then 42 (* dummy *) else if y then z else w
213+
214+
let () =
215+
if true then (* a *)
216+
begin
217+
()
218+
end
219+
else
220+
()

‎test/passing/refs.ahrefs/ite-fit_or_vertical_closing.ml.ref‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,3 +220,10 @@ let compare s1 s2 =
220220

221221
let _ = if x then 42 (* dummy *) else y
222222
let _ = if x then 42 (* dummy *) else if y then z else w
223+
224+
let () =
225+
if true then (* a *)
226+
begin
227+
()
228+
end else
229+
()

‎test/passing/refs.ahrefs/ite-fit_or_vertical_no_indicate.ml.ref‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,3 +210,11 @@ let compare s1 s2 =
210210

211211
let _ = if x then 42 (* dummy *) else y
212212
let _ = if x then 42 (* dummy *) else if y then z else w
213+
214+
let () =
215+
if true then (* a *)
216+
begin
217+
()
218+
end
219+
else
220+
()

‎test/passing/refs.ahrefs/ite-kr.ml.ref‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,3 +249,10 @@ let _ =
249249
z
250250
else
251251
w
252+
253+
let () =
254+
if true then (* a *)
255+
begin
256+
()
257+
end else
258+
()

0 commit comments

Comments
 (0)