ocaml-ppx / ppx_tools

Tools for authors of ppx rewriters
MIT License
134 stars 39 forks source link

genlifter: fix code generation for inline records #68

Closed let-def closed 5 years ago

let-def commented 5 years ago

The main problem with inline records is that labels are prefixed with an absolute path, e.g.

  type t = A of { a : string; b : int }

generates code which looks like:

class virtual ['res] lifter =
  object (this)
    method lift_Test_t : Test.t -> 'res=
      (function
       | Test.A { Test.a = a; Test.b = b } ->
           this#constr "Test.t"
             ("A",
               (this#record "Test.t.A"
                  [("a", (this#string a)); ("b", (this#int b))])) : Test.t ->
                                                                    'res)
  end

which is not valid OCaml code. For inline records, labels should just be identifiers (disambiguation is based on the name of the constructor, e.g. Test.A in this example).

This patch removes the prefix at two levels:

The bug was found by @daacaceressa who helped testing this solution. Open question: should we backport the patch to older versions of OCaml? Because of this issue, inline records have never been supported by genlifter.

alainfrisch commented 5 years ago

Thanks, and sorry for the slow reaction. LGTM, only a minor inline remark.

let-def commented 5 years ago

@alainfrisch: Thanks for the review. I updated the diff.

jhwoodyatt commented 5 years ago

I believe this patch is required for supporting OCaml 4.08 (Issue #69), which adds an inline record for the Pexp_letop variant constructor.

XVilka commented 5 years ago

There are some conflicts that require rebase.

XVilka commented 5 years ago

I checked the PR - it does seem not required anymore? Because it seems it is already integrated in the master. So can be closed, I guess.