janestreet / ppx_sexp_conv

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

Fewer parens #27

Closed copy closed 3 years ago

copy commented 5 years ago

Sexps written by ppx_sexp_conv often contain a lot of unnecessary parentheses, in my experience especially around record fields. In particular for the output-only generator (deriving sexp_of) it would be nice if the user could configure omitting parens in certain places.

yminsky commented 5 years ago

There is already some support for this. For example, sexp_option, sexp_list, and various annotations specified in the README all highlight ways you can adjust the default generator.

A concrete proposal for what flexibility to add would be helpful to understand the issue.

copy commented 5 years ago

I'd like the writer to unpack children when the result is unambiguous (i.e., a generalisation of sexp_option), for example:

In record fields:

Before:

type record =
  { a : int option
  ; b : string list
  ; c : sub }
and sub = B of int [@@deriving sexp]
let () =
  print_endline
    (Sexp.to_string_hum (sexp_of_record { a = Some 5; b = ["foo"; "bar"]; c = B 9 }))
(* ((a (5)) (b (foo bar)) (c (B 9))) *)

After:

type record =
  { a : int option [@unboxed]
  ; b : string list [@unboxed]
  ; c : sub [@unboxed] }
and sub = B of int [@@deriving sexp]
let () =
  print_endline
    (Sexp.to_string_hum (sexp_of_record { a = Some 5; b = ["foo"; "bar"]; c = B 9 }))
(* ((a 5) (b foo bar) (c B 9)) *)

In a single-argument variant:

Before:

type poly = [ `List of int list ] [@@deriving sexp]
let () = print_endline (Sexp.to_string_hum (sexp_of_poly (`List [1; 2; 3; 4])))
(* (List (1 2 3 4)) *)

After:

type poly = [ `List of int list [@unpack] ] [@@deriving sexp]
let () = print_endline (Sexp.to_string_hum (sexp_of_poly (`List [1; 2; 3; 4])))
(* (List 1 2 3 4) *)

I'm not sure if the reader could also support this. It might depend on whether it can determine if the contained type expects an atom or a list in the single-element case.

Either way, for me only writer support would be useful already.

yminsky commented 5 years ago

I think we're very unlikely to add writer support for things for which we don't also have reader support.

Have you looked at things like sexp_option, and the existing annotations supported by ppx_sexp_conv? I know that they can do unboxing of the kind you suggest in some cases.

copy commented 5 years ago

I think we're very unlikely to add writer support for things for which we don't also have reader support.

I believe in that case only support for built-in types (which are know to always return lists) are unpackable.

Have you looked at things like sexp_option, and the existing annotations supported by ppx_sexp_conv? I know that they can do unboxing of the kind you suggest in some cases.

Only sexp_option does this on record fields. The sexp_ types and the sexp_drop_default and omit_nil attributes only deal with omitting the record field if it is empty, not unpacking it if it isn't empty.

yminsky commented 5 years ago

OK. That all makes sense to me. We have no immediate plans to fix this, but we are looking at cleaning up some of the ways we use things like sexp_list and sexp_option, and some rationalization and cleaning up of the annotations to make them applicable in more cases could go along with that.

So, no promises, but we'll keep it in mind.

aalekseyev commented 4 years ago

Not doing anything here now because there's not a concrete design available. In addition to that, I believe it's a useful and important property of a generic sexp converter that, by default, if a value occurs in a data structure, then its sexp also does occur. Unboxing representations violates that property, so I'm not even convinced this is a good direction to move in.