ppxlib icon indicating copy to clipboard operation
ppxlib copied to clipboard

`esequence` with more than two expressions in `[@@deriving_inline]`

Open dvulakh opened this issue 3 years ago • 1 comments

Calling esequence on a list with more than two expressions in a [@@deriving_inline] gives the following error:

----line 1 of ppx_contract_test.ml col 1
File "./ppx_contract_test.ml", line 1, characters 0-0:

(yes, that is the entire error)

A small breaking example:

Ppx definition:

open! Base
open Ppxlib

let gen_bug ~loc ~path:_ _mod =
  let (module Builder) = Ast_builder.make loc in
  let open Builder in
  let to_sequence = List.init 3 ~f:(Fn.const [%expr ()]) in
  [%str let bug = [%e esequence to_sequence]]
;;

let str_module_type_decl = Deriving.Generator.make_noarg gen_bug
let () = Deriving.add "bug" ~str_module_type_decl |> Deriving.ignore

Breaking test:

module type S = sig end [@@deriving_inline bug]

let bug =
  ();
  ();
  ()
;;

let _ = bug

[@@@end]

Changing the 3 to a 2 causes the error to disappear. I suspect the problem is that esequence treats the sequence operator as left-associative, where it should instead be right-associative (there is no semantic difference, but now the AST doesn't round-trip). In particular, the following definition of the ppx compiles:

open! Base
open Ppxlib

let gen_bug ~loc ~path:_ _mod =
  let (module Builder) = Ast_builder.make loc in
  let open Builder in
  let esequence = function
    | [] -> [%expr ()]
    | l ->
      let l = List.rev l in
      List.fold (List.tl_exn l) ~f:(Fn.flip pexp_sequence) ~init:(List.hd_exn l)
  in
  let to_sequence = List.init 3 ~f:(Fn.const [%expr ()]) in
  [%str let bug = [%e esequence to_sequence]]
;;

let str_module_type_decl = Deriving.Generator.make_noarg gen_bug
let () = Deriving.add "bug" ~str_module_type_decl |> Deriving.ignore

dvulakh avatar Aug 02 '22 18:08 dvulakh

This has tripped us up in a couple of ppxes at janestreet. Writing esequence using fold_right instead of fold_left appears to fix it.

ceastlund avatar Aug 11 '22 22:08 ceastlund

Closed by #366.

ceastlund avatar Aug 24 '22 19:08 ceastlund