ocaml-ppx / ppx_deriving

Type-driven code generation for OCaml
MIT License
466 stars 89 forks source link

Version 4.2 fails to compile unused attributes #148

Open pdonadeo opened 7 years ago

pdonadeo commented 7 years ago

I have this type definition:

type raw_region_group = {
  name : string;
  iitc_draw_inline : (raw_iitc_draw option [@default None]);
  iitc_draw_filename : (string option [@default None]);
  json_tiles_inline : (string list option [@default None]);
  json_tiles_filename : (string option [@default None]);
  text_tiles_filename : (string option [@default None]);
  accounts : IntelBot.account list;
} [@@deriving sexp, yojson]

It works with version 4.1 but fails in 4.2 with this error:

File "src/configuration.ml", line 11, characters 45-52:
Error: Attribute `default' was not used.
Hint: `default' is available for label declarations but is used here in the
context of a core type.
Did you put it at the wrong level?

default is actually used by yojson but not by sexp. Is this behaviour the expected one? This actually prevents to mix two generators.

whitequark commented 7 years ago

ppx_deriving was designed with namespacing in mind; use [@yojson.default None] everywhere to work around issues like this.

pdonadeo commented 7 years ago

@whitequark thanks for your suggestion but something is going wrong:

File "src/configuration.ml", line 11, characters 45-59:
Error: Attribute `yojson.default' was not used

Any ideas? Maybe the culprit is not ppx_deriving but the specific filter, yojson?

@mjambon FYI

In this moment I worked around downgrading ppx_deriving to 4.1.

whitequark commented 7 years ago

@pdonadeo Try [@deriving.yojson.default].

pdonadeo commented 7 years ago

@whitequark nope, same error as before :sweat_smile:

File "src/configuration.ml", line 11, characters 45-68:
Error: Attribute `deriving.yojson.default' was not used
whitequark commented 7 years ago

Hmm, this might be a ppx_type_conv issue... I'll look at it

pdonadeo commented 7 years ago

Thanks @whitequark. It's not urgent for me, I can happily live with version 4.1 for a while. Do you need a minimal code that stimulate this issue?

whitequark commented 7 years ago

That would be helpful

pdonadeo commented 7 years ago

New OPAM switch:

$ opam switch --alias-of=4.04.1 4.04.1-ISSUE-148
$ opam install core ppx_deriving ppx_deriving_yojson

main.ml:

open Core

type person = {
  first_name : string;
  last_name : string;
  age : (int option [@deriving.yojson.default None]);
} [@@deriving sexp, yojson]

let _ =
  Printf.printf "Hello!\n%!"

Compile with:

ocamlbuild -use-ocamlfind \
    -cflag -thread -lflag -thread \
    -package core -package ppx_deriving_yojson -package ppx_sexp_conv \
    main.native

The error is:

File "main.ml", line 7, characters 22-45:
Error: Attribute `deriving.yojson.default' was not used
Command exited with code 2.
Compilation unsuccessful after building 1 target (0 cached) in 00:00:00.

Downgrading compiles again:

$ opam pin add ppx_deriving 4.1
$ ocamlfind ocamlc -thread -i -package ppx_sexp_conv -package ppx_deriving_yojson -package core main.ml > main.inferred.mli
$ cat main.inferred.mli
type person = { first_name : string; last_name : string; age : int option; }
val person_of_sexp : Sexplib.Sexp.t -> person
val sexp_of_person : person -> Sexplib.Sexp.t
val person_to_yojson : person -> Yojson.Safe.json
val person_of_yojson :
  Yojson.Safe.json -> person Ppx_deriving_yojson_runtime.error_or
emillon commented 7 years ago

I confirm that this happens also with [@compare] and [@equal].

It seems that it's also triggering errors when other extensions like ppx_blob are used in the same file. ("Extension `blob' was not translated").

xclerc commented 7 years ago

The error is actually raised by ppx_core that is used by ppx_sexp_conv. Indeed, ppx_core does some sanity checks and reports unused attributes, not knowing that another non-ppx_core-based deriver will use them later on.

With @diml, we discussed the possibility to change these errors into warnings plugged into the AST (i.e. [@@ocaml.ppwarning "some message"]), and have a patch ready. However, it does "silence" the issue but does not really solve it.

copy commented 7 years ago

Temporary workaroud until ppx_core is fixed: Add -no-check to the command line of ppx_sexp_conv or ppx_jane.

xclerc commented 7 years ago

The latest versions of ppx_driver (0.9.2) disables the checks by default. They can be enabled through a new command-line switch, namely -do-check.

tomjridge commented 7 years ago

I also have this problem. As a non-ppx expert, how do I add -no-check to my Makefile-based build?

copy commented 7 years ago

I also have this problem. As a non-ppx expert, how do I add -no-check to my Makefile-based build?

Find the -pp or -ppx flags and add -no-check after each preprocessor. It should look something like this:

ocamlfind ocamlopt … -pp 'ppx-jane -no-check'
gasche commented 7 years ago

Do I correctly understand that marking ppx_deriving 4.2.x as conflicting with ppx_driver < 0.9.2 would make people's life easier by ruling this bug out?

emillon commented 7 years ago

Probably, but on the other hand it seems impossible to install ppx_driver.0.9.2 on 4.04.2 or 4.05.0, so this would restrict a lot the potential users of ppx_deriving.4.2.

gasche commented 7 years ago

@xclerc would you consider releasing 4.04.2 or 4.05.0 versions of ppx_driver with the disabled check?

xclerc commented 7 years ago

ppx_driver only constraints available: [ ocaml-version >= "4.03.0" ]; the problem comes from ppx_ast that, in its v0.9.2 version, uses a function that does not exist (or rather has a different name) before 4.06.

(I still have to check whether it is the only problem.)