janestreet / ppx_sexp_conv

Generation of S-expression conversion functions from type definitions
MIT License
88 stars 17 forks source link

variant type with inline record is different from "normal" records #25

Closed eriklovlie closed 3 years ago

eriklovlie commented 6 years ago

This might be merely a documentation issue, however: the README states:

Records are represented as lists of lists, where each inner list is a key-value pair.

This doesn't seem to be the case for variant constructors with inline records:

Uncaught exception:

  (Sexplib.Conv.Of_sexp_error
   (Failure
    "hello.ml.t_of_sexp: record conversion: only pairs expected, their first element must be an atom")
   ((a 42) (b foo)))

The program is:

open! Core

type t =
  | A of { a: int; b: string }
  | B
[@@deriving sexp]

let () =
  let yo = Sexp.of_string "(A ((a 42) (b foo)))" |> t_of_sexp in
  match yo with
  | A { a; b } -> sprintf "a is %d, b is %s" a b |> print_endline
  | B -> ()

However, if I modify the string in Sexp.of_string to this it works:

  let yo = Sexp.of_string "(A (a 42) (b foo))" |> t_of_sexp in

So it seems the fact that the record is "inline" is significant in the serialization form. Should there be a note in the documentation on this?

eriklovlie commented 6 years ago
$ opam show ppx_jane
             package: ppx_jane
             version: v0.11.0
          repository: default
        upstream-url: https://ocaml.janestreet.com/ocaml-core/v0.11/files/ppx_jane-v0.11.0.tar.gz
       upstream-kind: http
   upstream-checksum: 11da0871ae3841fb6710ec6471ce6b92
            homepage: https://github.com/janestreet/ppx_jane
         bug-reports: https://github.com/janestreet/ppx_jane/issues
            dev-repo: git+https://github.com/janestreet/ppx_jane.git
              author: Jane Street Group, LLC <opensource@janestreet.com>
             license: Apache-2.0
             depends: ppx_assert (>= v0.11 & < v0.12) & ppx_base (>= v0.11 & < v0.12) & ppx_bench (>= v0.11 & < v0.12) & ppx_bin_prot (>= v0.11 & < v0.12) & ppx_custom_printf (>= v0.11 & < v0.12) & ppx_expect (>= v0.11 & < v0.12) & ppx_fail (>= v0.11 & < v0.12) & ppx_fields_conv (>= v0.11 & < v0.12) & ppx_here (>= v0.11 & < v0.12) & ppx_inline_test (>= v0.11 & < v0.12) & ppx_let (>= v0.11 & < v0.12) & ppx_optcomp (>= v0.11 & < v0.12) & ppx_optional (>= v0.11 & < v0.12) & ppx_pipebang (>= v0.11 & < v0.12) & ppx_sexp_message (>= v0.11 & < v0.12) & ppx_sexp_value (>= v0.11 & < v0.12) & ppx_typerep_conv (>= v0.11 & < v0.12) & ppx_variants_conv (>= v0.11 & < v0.12) & jbuilder >= 1.0+beta18.1 & ocaml-migrate-parsetree >= 1.0 & ppxlib >= 0.1.0
  installed-versions: v0.10.0 [4.05.0], v0.11.0 [4.06.1]
  available-versions: 113.24.00, 113.24.01, 113.33.00, 113.33.03, v0.9.0, v0.10.0, v0.11.0
         description: Standard Jane Street ppx rewriters

This package installs a ppx-jane executable, which is a ppx driver
including all standard Jane Street ppx rewriters.
ghost commented 6 years ago

It's indeed a documentation issue. We add a discussion about this when we added support for inline records and decided to avoid the extra parentheses.

aalekseyev commented 3 years ago

Added some doc in 407ef6c.