Skip to content

Commit

Permalink
fix: wrong renderer used due to memoization
Browse files Browse the repository at this point in the history
When a document is printed multiple times, memoization is reused, as intended.
However, these printing could be done with different renderers.
Prior this commit, the memoization also remembers the renderer that was used.
Therefore, subsequent printing could use a stale, memoized renderer.
This commit fixes the issue by threading the renderer through the layout
function, so that it becomes "stateless" and can be memoized with no
stale state saving.

Fixes #2
  • Loading branch information
sorawee committed Mar 13, 2024
1 parent 4a6c9c3 commit 79b6802
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 10 deletions.
4 changes: 3 additions & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
## 0.5 (2024-02-16)
## 0.5 (2024-03-13)

* Improve performance in `two_columns` via the zipper data structure
and caching.
* Fix a bug caused from a wrong renderer is used due to memoization.
Thanks to @zbyrn who reported the issue (#2)!

## 0.4 (2024-02-14)

Expand Down
16 changes: 8 additions & 8 deletions lib/printer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ module Core (C : Signature.CostFactory) = struct
global_id := id + 1;
id

type measure = { last: int; cost: C.t; layout: unit -> unit }
type measure = { last: int; cost: C.t; layout: Signature.renderer -> unit }

let (<==) (m1 : measure) (m2 : measure): bool =
m1.last <= m2.last && C.le m1.cost m2.cost
Expand Down Expand Up @@ -236,9 +236,9 @@ module Core (C : Signature.CostFactory) = struct
let (++) (m1 : measure) (m2 : measure): measure =
{ last = m2.last;
cost = C.combine m1.cost m2.cost;
layout = fun () ->
m1.layout ();
m2.layout () }
layout = fun renderer ->
m1.layout renderer;
m2.layout renderer }

let process_concat
(process_left : measure -> measure_set)
Expand Down Expand Up @@ -395,11 +395,11 @@ module Core (C : Signature.CostFactory) = struct
| Text (s, len_s) ->
MeasureSet [{ last = c + len_s;
cost = C.text c len_s;
layout = fun () -> render_tree renderer s }]
layout = fun renderer -> render_tree renderer s }]
| Newline _ ->
MeasureSet [{ last = i;
cost = C.newline i;
layout = fun () ->
layout = fun renderer ->
renderer "\n";
renderer (String.make i ' ') }]
| Concat (d1, d2) ->
Expand All @@ -421,7 +421,7 @@ module Core (C : Signature.CostFactory) = struct
| Blank i ->
MeasureSet [{ last = c + i;
cost = C.text 0 0;
layout = fun () -> renderer (String.make i ' ') }]
layout = fun renderer -> renderer (String.make i ' ') }]
| Evaled ms -> ms
| Fail -> failwith "fails to render"
in
Expand All @@ -439,7 +439,7 @@ module Core (C : Signature.CostFactory) = struct
(* so the memoization tables should be cleared. *)
(* However, in OCaml, there is no need to do the same, *)
(* since a doc is tied to a cost factory. *)
m.layout ();
m.layout renderer;
{ is_tainted ; cost = m.cost }
end

Expand Down
21 changes: 20 additions & 1 deletion test/main.ml
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,22 @@ let test_two_columns_performance () =
|> ignore;
true)

let test_different_renderer () =
let w = 20 in
let cf = Printer.default_cost_factory ~page_width:w () in
let module P = Printer.Make (val cf) in
let open P in

let d = text "while (true) {" ^^
nest 4
(nl ^^ text "f();" ^^ nl ^^ text "if (done())" ^^
(let exit_d = text "exit();" in
(space ^^ exit_d) <|> nest 4 (nl ^^ exit_d))) ^^
nl ^^ text "}"
in
Alcotest.(check string) "same string" vert_layout (pretty_format d);
Alcotest.(check string) "same string" vert_layout (pretty_format d)

let () =
Alcotest.run "pretty expressive"
[ "example doc",
Expand All @@ -391,4 +407,7 @@ let () =
"regression phantom space", `Quick, test_two_columns_regression_phantom;
"cost factory - overflow", `Quick, test_two_columns_factory_overflow;
"cost factory - bias", `Quick, test_two_columns_factory_bias;
"performance", `Slow, test_two_columns_performance ] ]
"performance", `Slow, test_two_columns_performance ];

"regression test",
[ "different renderer (issue #2)", `Quick, test_different_renderer ] ]

0 comments on commit 79b6802

Please sign in to comment.