janestreet / ppx_sexp_conv

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

Chops off the last variant's documentation for a variant type #14

Closed infinity0 closed 4 years ago

infinity0 commented 7 years ago
open Sexplib.Std

(** Testing "deriving sexp", variants *)
type test_option =
  | First (** First variant *)
  | Second (** Second variant *)
[@@deriving sexp]

(** Testing "deriving sexp", records *)
type test_record = {
  first : unit; (** First field *)
  second : unit; (** Second field *)
}
[@@deriving sexp]

With both ppx-deriving and ppx-driver, ocamldoc gives me something like:

type test_option = 
|   First   (*  First variant   *)
|   Second
    Testing "deriving sexp", variants
val test_option_of_sexp : Sexplib.Sexp.t -> test_option
val sexp_of_test_option : test_option -> Sexplib.Sexp.t
type test_record = {
    first : unit;   (*  First field *)
    second : unit;  (*  Second field    *)
}
    Testing "deriving sexp", records
val test_record_of_sexp : Sexplib.Sexp.t -> test_record
val sexp_of_test_record : test_record -> Sexplib.Sexp.t

ocamldoc for me is at version 4.03.0

ghost commented 7 years ago

And without the [@@deriving sexp] it works as expected?

infinity0 commented 7 years ago

Yes, indeed.

ghost commented 7 years ago

Could you try to replace [@@deriving sexp] by a dummy attribute [@@foo] and remove the ppx rewriter?

infinity0 commented 7 years ago

With no attribute:

(** Test module *)

(** Testing "deriving sexp", variants *)
type test_option =
  | First (** First variant *)
  | Second (** Second variant *)

(** Testing "deriving sexp", records *)
type test_record = {
  first : unit; (** First field *)
  second : unit; (** Second field *)
}

I get an expected result:

type test_option = 
|   First   (*  First variant   *)
|   Second  (*  Second variant  *)
Testing "deriving sexp", variants
type test_record = {
    first : unit;   (*  First field *)
    second : unit;  (*  Second field    *)
}
Testing "deriving sexp", records

Interestingly with [@@foo] a different bug crops up:

(** Test module *)

(** Testing "deriving sexp", variants *)
type test_option =
  | First (** First variant *)
  | Second (** Second variant *)
[@@foo]

(** Testing "deriving sexp", records *)
type test_record = {
  first : unit; (** First field *)
  second : unit; (** Second field *)
}
[@@foo]

Result, now chops off the documentation for the overall record type.

type test_option = 
|   First   (*  First variant   *)
|   Second  (*  Second variant  *)
Testing "deriving sexp", variants
type test_record = {
    first : unit;   (*  First field *)
    second : unit;  (*  Second field    *)
}

Both of these were run with ocamldoc test.ml, no wrapping buildsystem was used.

infinity0 commented 7 years ago

Whoops, I mean ocamldoc -html -verbose test.ml

ghost commented 7 years ago

And for the original version, did you pass -ppx to ocamldoc? Basically we were talking about it with @trefis and we were wondering whether this was in fact an ocamldoc bug

infinity0 commented 7 years ago

OK I managed to recover the big command line that oasis ran for the original version. Here it is, with tweaks to run outside of oasis:

ocamldoc.opt \
 -I /usr/lib/ocaml/compiler-libs \
 -I $HOME/.opam/system/lib/result \
 -I $HOME/.opam/system/lib/ppx_deriving \
 -I $HOME/.opam/system/lib/ppx_core \
 -I $HOME/.opam/system/lib/ppx_optcomp \
 -I $HOME/.opam/system/lib/ppx_driver \
 -I $HOME/.opam/system/lib/ppx_type_conv \
 -I /usr/lib/ocaml \
 -I $HOME/.opam/system/lib/sexplib \
 -I $HOME/.opam/system/lib/ppx_sexp_conv \
 -ppx "$HOME/.opam/system/lib/ppx_deriving/./ppx_deriving \
       $HOME/.opam/system/lib/ppx_core/./ppx_core.cma \
       $HOME/.opam/system/lib/ppx_sexp_conv/./ppx_sexp_conv_expander.cma \
       $HOME/.opam/system/lib/ppx_optcomp/./ppx_optcomp.cma \
       $HOME/.opam/system/lib/ppx_driver/./ppx_driver.cma \
       $HOME/.opam/system/lib/ppx_type_conv/./ppx_type_conv.cma \
       $HOME/.opam/system/lib/ppx_type_conv/./ppx_type_conv_deriving.cma \
       $HOME/.opam/system/lib/ppx_sexp_conv/./ppx_sexp_conv.cma" \
 -html \
 -colorize-code \
 -short-functors \
 -charset utf-8 \
 -stars \
 test.ml

You have to also add open Sexplib.Std to my immediately-preceding examples. You will see the bug mentioned in the first post. Then if you remove the big -ppx flag, the first bug (chops off the documentation for the last variant) disappears but the second bug (chops off the documentation for the overall record type) now re-appears.

infinity0 commented 7 years ago

Actually, in both cases I just described, the second bug is present. But the second bug goes away if one uses ppx-driver instead of ppx-deriving. I haven't yet managed to reproduce the exact command line with ppx-driver outside of oasis though, it depends on a ppx-driver-ppx_sexp_conv.native that seems to be tied to a particular project.

infinity0 commented 7 years ago

Oh, this works:

ocamldoc.opt \
 -I /usr/lib/ocaml/compiler-libs \
 -I $HOME/.opam/system/lib/result \
 -I $HOME/.opam/system/lib/ppx_deriving \
 -I $HOME/.opam/system/lib/ppx_core \
 -I $HOME/.opam/system/lib/ppx_optcomp \
 -I $HOME/.opam/system/lib/ppx_driver \
 -I $HOME/.opam/system/lib/ppx_type_conv \
 -I /usr/lib/ocaml \
 -I $HOME/.opam/system/lib/sexplib \
 -I $HOME/.opam/system/lib/ppx_sexp_conv \
 -html \
 -colorize-code \
 -short-functors \
 -charset utf-8 \
 -stars \
 -pp "./ppx-driver-ppx_sexp_conv.native \
 -dump-ast" \
 test.ml

if you copy ppx-driver-ppx_sexp_conv.native to the same directory as test.ml. I guess you guys probably know how to create one and do that.

In summary,

Command line Bugs present
First big one that I posted, with ppx-deriving "chops off doc for last variant", "chops off parent record doc"
First big one that I posted, minus -ppx "chops off parent record doc"
Second big one that I posted, with ppx-driver "chops off doc for last variant"
ghost commented 7 years ago

I don't know how ocamldoc works exactly, my guess is that it's using the location in the AST and somehow this get messed up by the use of a pre-processor. The second line in the table clearly indicates a bug in ocamldoc itself, so I think this should get fixed before doing anything else here, as the fix might fix the other issues.

aalekseyev commented 4 years ago

Closing it because it's seems not sexp-conv-specific. (and I don't know if the issue still exists)