Skip to content

Commit 6d892ea

Browse files
committed
fix a few failures when printing a modified AST
1 parent fa53910 commit 6d892ea

File tree

6 files changed

+206
-7
lines changed

6 files changed

+206
-7
lines changed

CHANGES.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ profile. This started with version 0.26.0.
2222
- `Ast_mapper.default_mapper` now iterates on the location of `in` in `let+ .. in ..`
2323
(#2658, @v-gb)
2424

25+
- ocamlformat is now more robust when used as a library to print modified ASTs
26+
(#2659, @v-gb)
27+
2528
## 0.27.0
2629

2730
### Highlight

lib/Cmts.ml

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -365,13 +365,16 @@ let relocate_ext_cmts (t : t) src (pre, pld) ~whole_loc =
365365
| PStr [{pstr_desc= Pstr_eval _; pstr_loc; _}] ->
366366
let kwd_loc =
367367
match Source.loc_of_first_token_at src whole_loc LBRACKETPERCENT with
368-
| Some loc -> loc
368+
| Some _ as o -> o
369369
| None -> (
370370
match Source.loc_of_first_token_at src whole_loc PERCENT with
371-
| Some loc -> loc
372-
| None -> impossible "expect token starting extension" )
371+
| Some _ as o -> o
372+
| None ->
373+
if whole_loc.loc_ghost then None
374+
else impossible "expect token starting extension" )
373375
in
374-
relocate_cmts_before t ~src:pstr_loc ~sep:kwd_loc ~dst:whole_loc
376+
Option.iter kwd_loc ~f:(fun kwd_loc ->
377+
relocate_cmts_before t ~src:pstr_loc ~sep:kwd_loc ~dst:whole_loc )
375378
| _ -> ()
376379

377380
let relocate_wrongfully_attached_cmts t src exp =

lib/Fmt_ast.ml

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,7 @@ let fmt_constant c ?epi {pconst_desc; pconst_loc= loc} =
287287
then str_as 1000
288288
else str )
289289
(Format_.sprintf "{%s|%s|%s}" delim s delim)
290-
| Pconst_string (_, loc', None) -> (
290+
| Pconst_string (orig_s, loc', None) -> (
291291
let delim = ["@,"; "@;"] in
292292
let contains_pp_commands s =
293293
let is_substring substring = String.is_substring s ~substring in
@@ -343,7 +343,10 @@ let fmt_constant c ?epi {pconst_desc; pconst_loc= loc} =
343343
| `Never -> `Preserve
344344
| `Auto -> `Normalize
345345
in
346-
let s = Source.string_literal c.source preserve_or_normalize loc in
346+
let s =
347+
if loc.loc_ghost then String.escaped orig_s
348+
else Source.string_literal c.source preserve_or_normalize loc
349+
in
347350
Cmts.fmt c loc'
348351
@@
349352
match c.conf.fmt_opts.break_string_literals.v with

lib/Source.ml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,8 @@ let empty_line_after t (loc : Location.t) =
138138

139139
let extension_using_sugar ~(name : string Location.loc)
140140
~(payload : Location.t) =
141-
Source_code_position.ascending name.loc.loc_start payload.loc_start > 0
141+
name.loc.loc_ghost
142+
|| Source_code_position.ascending name.loc.loc_start payload.loc_start > 0
142143

143144
let type_constraint_is_first typ loc =
144145
Location.compare_start typ.ptyp_loc loc < 0

test/unit/test_fmt_ast.ml

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
open Ocamlformat_stdlib
2+
open Ocamlformat_lib
3+
4+
let updated_ast_tests =
5+
[ ( "updated AST"
6+
, `Quick
7+
, fun () ->
8+
(* We try to ensure that modified ASTs can be printed by ocamlformat,
9+
which can fail due to assumption about certain constructions
10+
having corresponding bits of syntax in the Source.t. *)
11+
let source1 = "" in
12+
let source2 =
13+
(* Shift all the locations down, in case the parser consults
14+
location information somehow. *)
15+
String.make 1000 '\n'
16+
^ {outer|
17+
let _ =
18+
(* exercise every expression construct *)
19+
x;
20+
1_2;
21+
12l;
22+
'a';
23+
'\n';
24+
"a\013";
25+
{|a|};
26+
12e1;
27+
(let rec x = 1 and y = 2 in ());
28+
(let x = 1 and y = 2 in ());
29+
(fun x y : a -> function 1 -> 1);
30+
f a ~b ?c;
31+
(match () with () -> () | () -> ());
32+
(try () with () -> () | () -> ());
33+
((), ());
34+
(Some (); None);
35+
(`Some (); `None);
36+
({ a = 1; b : float = 2 }, { r with a });
37+
a.x;
38+
a.x <- 1;
39+
[|1;2|];
40+
[1;2];
41+
(if a then b else if c then d else e);
42+
(a; b);
43+
(while a; do b; done);
44+
(for a = b to c do d done);
45+
(a : b);
46+
(a : b :> c);
47+
a#b;
48+
x <- 2;
49+
{< x = 1 >};
50+
(let module M = struct end in ());
51+
(let exception E in ());
52+
assert ();
53+
lazy 1;
54+
object val x = 1 end;
55+
(module M);
56+
(module M : S);
57+
(let open M in 1);
58+
M.(1);
59+
(let+ x = 1 and+ y = 2 in ());
60+
[%extension 1];
61+
(function _ -> .);
62+
_;
63+
begin () end;
64+
(a :: b);
65+
a.!(b);
66+
a.!(b) <- c;
67+
!a;
68+
a + b;
69+
|outer}
70+
in
71+
let conf = Ocamlformat_lib.Conf.default in
72+
let ast ~input_name ~source =
73+
Ocamlformat_lib.Parse_with_comments.parse
74+
(Ocamlformat_lib.Parse_with_comments.parse_ast conf)
75+
Structure conf ~input_name ~source
76+
in
77+
let ast1 = ast ~input_name:"source1" ~source:source1 in
78+
let ast2 =
79+
let ast = ast ~input_name:"source2" ~source:source2 in
80+
let ghostify =
81+
{ Ocamlformat_parser_extended.Ast_mapper.default_mapper with
82+
location= (fun _ loc -> {loc with loc_ghost= true}) }
83+
in
84+
{ast with ast= ghostify.structure ghostify ast.ast}
85+
in
86+
let ast_replaced = {ast1 with ast= ast2.ast} in
87+
let with_buffer_formatter ~buffer_size k =
88+
let buffer = Buffer.create buffer_size in
89+
let fs = Format_.formatter_of_buffer buffer in
90+
Fmt.eval fs k ;
91+
Format_.pp_print_flush fs () ;
92+
if Buffer.length buffer > 0 then Format_.pp_print_newline fs () ;
93+
Buffer.contents buffer
94+
in
95+
let print (ast : _ Parse_with_comments.with_comments) =
96+
let open Fmt in
97+
let debug = conf.opr_opts.debug.v in
98+
with_buffer_formatter ~buffer_size:1000
99+
( set_margin conf.fmt_opts.margin.v
100+
$ set_max_indent conf.fmt_opts.max_indent.v
101+
$ Fmt_ast.fmt_ast Structure ~debug ast.source
102+
(Ocamlformat_lib.Cmts.init Structure ~debug ast.source
103+
ast.ast ast.comments )
104+
conf ast.ast )
105+
in
106+
let printed_ast_replaced = String.strip (print ast_replaced) in
107+
let expected =
108+
String.strip
109+
{outer|
110+
let _ =
111+
x;
112+
1_2;
113+
12l;
114+
'a';
115+
'\n';
116+
"a\r";
117+
{|a|};
118+
12e1;
119+
(let rec x = 1 and y = 2 in
120+
());
121+
(let x = 1 and y = 2 in
122+
());
123+
(fun x y : a -> function 1 -> 1);
124+
f a ~b ?c;
125+
(match () with () -> () | () -> ());
126+
(try () with () -> () | () -> ());
127+
((), ());
128+
Some ();
129+
None;
130+
`Some ();
131+
`None;
132+
({ a = 1; b : float = 2 }, { r with a });
133+
a.x;
134+
a.x <- 1;
135+
[| 1; 2 |];
136+
[ 1; 2 ];
137+
if a then b else if c then d else e;
138+
a;
139+
b;
140+
while a do
141+
b
142+
done;
143+
for a = b to c do
144+
d
145+
done;
146+
(a : b);
147+
(a : b :> c);
148+
a#b;
149+
x <- 2;
150+
{<x = 1>};
151+
(let module M = struct end in
152+
());
153+
(let exception E in
154+
());
155+
assert ();
156+
lazy 1;
157+
object
158+
val x = 1
159+
end;
160+
(module M);
161+
(module M : S);
162+
(let open M in
163+
1);
164+
M.(1);
165+
(let+ x = 1 and+ y = 2 in
166+
());
167+
[%extension 1];
168+
(function _ -> .);
169+
_;
170+
();
171+
a :: b;
172+
a.!(b);
173+
a.!(b) <- c;
174+
!a;
175+
a + b
176+
|outer}
177+
in
178+
(* Ideally we'd improve two things about this test:
179+
180+
- check the new string parses, to the same AST as the original one
181+
- use ppx_expect, so we have a nicer workflow and more readable
182+
errors *)
183+
if String.( <> ) expected printed_ast_replaced then (
184+
print_endline "got:" ;
185+
print_endline printed_ast_replaced ;
186+
failwith "different result" ) ) ]
187+
188+
let tests = updated_ast_tests

test/unit/test_unit.ml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ let tests =
118118
; ("Ast", Test_ast.tests)
119119
; ("Literal_lexer", Test_literal_lexer.tests)
120120
; ("Fmt", Test_fmt.tests)
121+
; ("Fmt_ast", Test_fmt_ast.tests)
121122
; ("Translation_unit", Test_translation_unit.tests) ]
122123

123124
let () = Alcotest.run "ocamlformat" tests ~compact:true

0 commit comments

Comments
 (0)