janestreet / ppx_sexp_conv

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

Doesn't work for exceptions #11

Closed infinity0 closed 4 years ago

infinity0 commented 8 years ago

If I write this:

module type Test  = sig
  type v
  exception Found_cycle of v list [@@deriving sexp]
  type found_chain = { tip: v; } [@@deriving sexp]
  exception Found_chain of found_chain [@@deriving sexp]
  exception Found_chain2 of int [@@deriving sexp]
end

merlin tells me the type signature is this:

sig
  type v
  exception Found_cycle of v list
  type found_chain = { tip : v; }
  val found_chain_of_sexp : Sexplib.Sexp.t -> found_chain
  val sexp_of_found_chain : found_chain -> Sexplib.Sexp.t
  exception Found_chain of found_chain
  exception Found_chain2 of int
end

Also I can change sexp to sexpomgwtfbbq in the exception declaration without getting a compilation error; but the error does occur if I make the same change for the type declaration.

I'm using

ppx_sexp_conv     113.33.03  Generation of S-expression conversion functions from type definitions

and

system  C system  System compiler (4.02.3)
infinity0 commented 8 years ago

Same result when running with

4.03.0  C 4.03.0  Official 4.03.0 release
ghost commented 8 years ago

Could you try to use ppx_sexp_conv through ppx_driver to see if it's a problem related to the ppx_deriving glue code? You can just install the ppx_jane package in opam and do ppx-jane file.ml

infinity0 commented 8 years ago

With 113.33.03 of ppx_driver and ppx_jane:

I'm having a trouble getting ppx_driver to work - I added XOCamlbuildPluginTags: package(ppx_driver.ocamlbuild) to _oasis and created _tags with these contents:

<**/*>: predicate(custom_ppx)
<src/*.{ml,mli}>: ppx-driver(ppx_sexp_conv)

but when I regen oasis files and run make I get these warnings:

W: No replace section found in file _tags
Finished, 1 target (0 cached) in 00:00:00.
File "_tags", line 2, characters 18-43:
Warning: tag "ppx-driver" does not expect a parameter, but is used with parameter "ppx_sexp_conv"
File "_tags", line 2, characters 18-43:
Warning: the tag "ppx-driver(ppx_sexp_conv)" is not used in any flag or dependency declaration, so it will have no effect; it may be a typo. Otherwise you can use `mark_tag_used` in your myocamlbuild.ml to disable this warning.

then Unbound module Sexplib as a build error, I guess caused by the above.

When I run ppx-jane test.ml with the above test module I get:

module type Test  =
  sig
    type v
    exception Found_cycle of v list
    include sig [@@@ocaml.warning "-32"] end
    type found_chain = {
      tip: v;}
    include
      sig
        [@@@ocaml.warning "-32"]
        val found_chain_of_sexp : Sexplib.Sexp.t -> found_chain
        val sexp_of_found_chain : found_chain -> Sexplib.Sexp.t
      end
    exception Found_chain of found_chain
    include sig [@@@ocaml.warning "-32"] end
    exception Found_chain2 of int
    include sig [@@@ocaml.warning "-32"] end
  end
ghost commented 8 years ago

Sorry i didn't realize this was a signature. [@@deriving sexp] in signatures has no effect. We just accept it for symmetry with the implementation.

For the ppx_driver problem you need to add something to myocamlbuild.ml, it should ve described in the doc

On Tue, 19 Jul 2016, 07:34 Ximin Luo, notifications@github.com wrote:

With 113.33.03 of ppx_driver and ppx_jane:

I'm having a trouble getting ppx_driver to work - I added XOCamlbuildPluginTags: package(ppx_driver.ocamlbuild) to _oasis and created _tags with these contents:

</>: predicate(custom_ppx) <src/.{ml,mli}>: ppx-driver(ppx_sexp_conv)

but when I regen oasis files and run make I get these warnings:

W: No replace section found in file _tags Finished, 1 target (0 cached) in 00:00:00. File "_tags", line 2, characters 18-43: Warning: tag "ppx-driver" does not expect a parameter, but is used with parameter "ppx_sexp_conv" File "_tags", line 2, characters 18-43: Warning: the tag "ppx-driver(ppx_sexp_conv)" is not used in any flag or dependency declaration, so it will have no effect; it may be a typo. Otherwise you can use mark_tag_used in your myocamlbuild.ml to disable this warning.

then Unbound module Sexplib as a build error, I guess caused by the above.

When I run ppx-jane test.ml with the above test module I get:

module type Test = sig type v exception Found_cycle of v list include sig [@@@ocaml.warning "-32"] end type found_chain = { tip: v;} include sig [@@@ocaml.warning "-32"] val found_chain_of_sexp : Sexplib.Sexp.t -> found_chain val sexp_of_found_chain : found_chain -> Sexplib.Sexp.t end exception Found_chain of found_chain include sig [@@@ocaml.warning "-32"] end exception Found_chain2 of int include sig [@@@ocaml.warning "-32"] end end

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/janestreet/ppx_sexp_conv/issues/11#issuecomment-233544350, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMZAPFVvdXlFB33lRUdiWZelJvIdrobks5qXHAMgaJpZM4JO-xY .

infinity0 commented 8 years ago

OK, I've changed module type Test = sig to module Test = struct and ppx-jane does indeed output things like:

    exception Found_cycle of v list
    let () =
      Sexplib.Exn_magic.register1 (fun v0  -> Found_cycle v0)
        "test.ml.Test.Found_cycle" (sexp_of_list sexp_of_v)

So I guess this part works. But the original point of this exercise was to be able to pretty-print exceptions. In the realworldocaml book, it states that we can achieve this with with sexp. Now ppx is The New Way, so I thought I'd be able to use [@@deriving sexp] to achieve the same, but this appears not to work. Bypassing oasis/ocamlbuild altogether:

open Sexplib.Std
type num = N of int [@@deriving sexp]
exception NumError of num [@@deriving sexp]
let () = raise (NumError (N 3))

output of ppx-jane:

open Sexplib.Std
type num =
  | N of int
let _ = fun (_ : num)  -> ()

let num_of_sexp: Sexplib.Sexp.t -> num =
  let _tp_loc = "test.ml.num" in
  function
  | Sexplib.Sexp.List ((Sexplib.Sexp.Atom ("n"|"N" as _tag))::sexp_args) as
      _sexp ->
      (match sexp_args with
       | v0::[] -> let v0 = int_of_sexp v0 in N v0
       | _ -> Sexplib.Conv_error.stag_incorrect_n_args _tp_loc _tag _sexp)
  | Sexplib.Sexp.Atom ("n"|"N") as sexp ->
      Sexplib.Conv_error.stag_takes_args _tp_loc sexp
  | Sexplib.Sexp.List ((Sexplib.Sexp.List _)::_) as sexp ->
      Sexplib.Conv_error.nested_list_invalid_sum _tp_loc sexp
  | Sexplib.Sexp.List [] as sexp ->
      Sexplib.Conv_error.empty_list_invalid_sum _tp_loc sexp
  | sexp -> Sexplib.Conv_error.unexpected_stag _tp_loc sexp
let _ = num_of_sexp
let sexp_of_num: num -> Sexplib.Sexp.t =
  function
  | N v0 ->
      let v0 = sexp_of_int v0 in
      Sexplib.Sexp.List [Sexplib.Sexp.Atom "N"; v0]
let _ = sexp_of_num
exception NumError of num
let () =
  Sexplib.Exn_magic.register1 (fun v0  -> NumError v0) "test.ml.NumError"
    sexp_of_num

let () = raise (NumError (N 3))
$ ocamlfind ocamlc -package sexplib -package ppx_sexp_conv /usr/lib/ocaml/bigarray.cma /home/infinity0/.opam/system/lib/sexplib/sexplib.cma test.ml && ./a.out 
findlib: [WARNING] Interface topdirs.cmi occurs in several directories: /usr/lib/ocaml/compiler-libs, /usr/lib/ocaml
Fatal error: exception Test.NumError(_)
2

but I'm expecting to see at least Test.NumError(3) or Test.NumError((N 3)) or something like that.

Separately, and as described earlier, if in the above mini example I

and this still feels very very wrong to me.

(I also can't get ppx_driver to work even adding a minimal myocamlbuild.ml, but never mind.)

ghost commented 8 years ago

I can reproduce the problem. However it does work correctly when using ppx-driver:

$ ocamlfind ocamlc -predicates custom_ppx -pp ppx-jane -linkpkg \
  -package sexplib -package ppx_sexp_conv test.ml && ./a.out
[...]
Fatal error: exception (test.ml.NumError (N 3))

I checked the glue code between ppx_type_conv and ppx_deriving and it seems that it's indeed not exporting exception generators. Then I checked ppx_deriving itself and it seems that it doesn't support exceptions at all, so there is no chance this can work.

Could you report this to ppx_deriving? Once ppx_deriving supports exceptions I'll update the glue code.

infinity0 commented 8 years ago

Understood, I will do that. Thanks for the help!

infinity0 commented 8 years ago

FWIW, I managed to get this working via ppx-driver in infinity0/ocaml-bjsim@3a7e505e. I had to do some weird things to _tags and couldn't figure out how to make it remain compatible with UTop_main.interact. But it might help others in the future, as a "working example".

aalekseyev commented 4 years ago

I don't really understand the issue, and we're using deriving sexp on exceptions in Jane Street, so presumably the issue is solved in the modern world. I'm closing the issue. Feel free to re-open if it's not, actually, solved.