search

janestreet / ppx_sexp_conv

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

overloaded constructor names cause failure #10

Closed agarwal closed 4 years ago

agarwal commented 8 years ago

Consider the file a.ml:

open Sexplib.Std
type t = None [@@deriving sexp]
type u = {a : string} [@@deriving sexp]

Compiling gives this error:

$ ocamlfind ocamlc -package sexplib,ppx_sexp_conv -c a.ml
File "a.ml", line 4, characters 10-11:
Error: This variant pattern is expected to have type t
       The constructor Some does not belong to type t

The issue is we have used None as a constructor in a new type definition. The generated code for u includes the normal option type constructors None and Some, but unfortunately type inference ends up requiring these to be of type t. I'm not sure what the best solution would be. Maybe some explicit type annotations, or fully qualified names for all type constructors (though I can't find where None and Some are defined. Pervasives.None doesn't work, so I guess the compiler has special support for these. Sexplib could define an alias to the option type and that could be used).

On a related note, would it be possible to use fully qualified names for other functions like int_of_sexp in the generated code, thereby removing the requirement of having to do open Sexplib.Std. This would simplify things in a few cases.

ghost commented 8 years ago

For the option problem it should be possible to generate *predef*.None. This is not documented but compiler accepts that.

On a related note, would it be possible to use fully qualified names for other functions like int_of_sexp in the generated code, thereby removing the requirement of having to do open Sexplib.Std. This would simplify things in a few cases.

int is not a keyword in OCaml, and the user is free to redefine it. Even if we assume that no one will ever redefine the int type, it's not uncommon to at least redefine int_of_sexp or sexp_of_int. IIRC Core_kernel.Std does it

agarwal commented 8 years ago

int is not a keyword in OCaml, and the user is free to redefine it. Even if we assume that no one will ever redefine the int type, it's not uncommon to at least redefine int_of_sexp or sexp_of_int. IIRC Core_kernel.Std does it

I see, so end users actually have more flexibility the way it is. Okay, thanks for the explanation.

agarwal commented 7 years ago

Any update on this? I'm still getting this error in ppx_sexp_conv v0.9.0.

ghost commented 7 years ago

Sorry, not yet no.

BTW, whenever I want a constructor named None, I just use some other name to avoid the conflict, given how pervasive the option type is. How important is it in your case to use None?

agarwal commented 7 years ago

I just use some other name to avoid the conflict

I would too, but I'm using this for protoc files provided by a 3rd party. My solution right now is to replace NONE in their files with NOTHING. That's not too bad, so this isn't a major issue. Only slight negative is that names in our OCaml code don't correspond exactly to the standard as dictated by the protoc files.

aalekseyev commented 4 years ago

I believe this issue was fixed some time ago. The original example works fine today.