Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

type nonrec is ignored, causing an infinite loop #116

Open
emillon opened this issue Nov 7, 2016 · 6 comments
Open

type nonrec is ignored, causing an infinite loop #116

emillon opened this issue Nov 7, 2016 · 6 comments
Labels

Comments

@emillon
Copy link
Contributor

emillon commented Nov 7, 2016

Hi,

I'm trying to use is with type nonrec but it seems that this annotation is ignored.

Example:

type t = int
[@@deriving ord]

module M = struct
  type nonrec t = t
  [@@deriving ord]
end

let () = Printf.printf "%d\n" (M.compare 1 2)

This triggers an infinite loop instead of calling the top-level compare function.

Thanks for this great project!

@emillon emillon changed the title type nonrec is ignored causing an infinite loop type nonrec is ignored, causing an infinite loop Nov 7, 2016
@whitequark
Copy link
Collaborator

Please note that I do not have much time to maintain this library, and will not fix this bug. I can offer you assistance in writing a PR though.

@emillon
Copy link
Contributor Author

emillon commented Dec 6, 2016

Sure, I'd be interested. Any pointers would be appreciated.

@whitequark
Copy link
Collaborator

@emillon Sure. First, try compiling your code using ocamlc -dsource. This will output the preprocessed source as currently used. I think you will quickly spot why implementing type nonrec is problematic; this messes rather badly with the hygiene mechanism. (Also try using [@@deriving] with a let..and phrase to see the source of additional complexity.)

@whitequark
Copy link
Collaborator

I think rewriting the output of ppx_deriving such that it works with type nonrec will already go a long way towards fixing this.

emillon added a commit to emillon/ppx_deriving that referenced this issue Dec 21, 2016
This feature has two different implementations in the AST:

- in 4.02.3, it is an annotation on a type,
- in >= 4.03.0 it is straight in the constructor.

Note that this can break existing programs; however relying on
`ppx_deriving` to do the right thing seems dangerous.
Maybe an escape hatch could be added so that the recursive version is
derived nonetheless.

It is an intermediate solution to ocaml-ppx#116. Solving it is tricky because it
would be necessary to expose this to the plugins, breaking the API. It
is probably easier to do so once 4.02.3 support is dropped, since the
per-type semantics are difficult to handle.
whitequark pushed a commit that referenced this issue Dec 21, 2016
This feature has two different implementations in the AST:

- in 4.02.3, it is an annotation on a type,
- in >= 4.03.0 it is straight in the constructor.

Note that this can break existing programs; however relying on
`ppx_deriving` to do the right thing seems dangerous.
Maybe an escape hatch could be added so that the recursive version is
derived nonetheless.

It is an intermediate solution to #116. Solving it is tricky because it
would be necessary to expose this to the plugins, breaking the API. It
is probably easier to do so once 4.02.3 support is dropped, since the
per-type semantics are difficult to handle.
gasche added a commit to gasche/opam-repository that referenced this issue Nov 25, 2017
4.1.5 is a maintenance release to include support for OCaml 4.05 and
OCaml 4.06 in the ppx_deriving 4.1 branch, which does not contain the
driverization work that has been causing issues for some users.

  * Add support for OCaml 4.05.0 and 4.06.0 (ocaml-ppx/ppx_deriving#154, ocaml-ppx/ppx_deriving#155, ocaml-ppx/ppx_deriving#156, ocaml-ppx/ppx_deriving#159).
  * Consider { with_path = false } when printing record fields (ocaml-ppx/ppx_deriving#157).
  * Fix comparison order of fields in records (ocaml-ppx/ppx_deriving#136).
  * Silence an `unused rec flag` warning in generated code (ocaml-ppx/ppx_deriving#137).
  * Monomorphize comparison function for builtin types (ocaml-ppx/ppx_deriving#115)
  * Raise an error when `type nonrec` is encountered (ocaml-ppx/ppx_deriving#116).
  * Display an error message when dynamic package loading fails.
  * Add a `with_path` option to `@@deriving` to skip the module path
    in generated code (ocaml-ppx/ppx_deriving#120).
@XVilka
Copy link
Contributor

XVilka commented Nov 1, 2019

This issue is mentioned in Facebook's Infer Contributors Guide.

@gasche
Copy link
Contributor

gasche commented Nov 7, 2019

Recent versions of ppx_deriving (4.1.5 (released in November 2017) and later) show an error when nonrec is used, so at least there is no silent production of looping code anymore.

I had a look last week and my impression is that it is not an easy problem to solve. At first it looks like just not adding the rec keyword would make the resulting code sensible for a nonrec type (the name of the generated function would refer to the generated function of the previous type of the same name, which is what we want), but in fact the structure of the returned code is decided by the plugins, not by the API module (which mostly provide helpers to make plugin life easier), so (1) fixing this issue would require a change in each of the plugins and (2) we have to do some API design/changes if we want to push more logic into the API module. Then I ran out of time for the day, and I stopped looking at it.

Remark: some plugins have evolved in a way that uses the recursivity of the generator for convenience; for example show generates a show and pp function, the definition of show depends on pp, but there is no explicit sequencing of declarations as they are in a big mutually-recursive block. To stop being recursive by default, we have to break those mutual blocks in the plugins.

kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this issue Oct 26, 2020
CHANGES:

5.0 (26/10/2020)
----------------

* Migrate to ppxlib ocaml-ppx/ppx_deriving#206, ocaml-ppx/ppx_deriving#210
  (Anton Kochkov, Gabriel Scherer, Thierry Martinez)

4.5
---

* Add support for OCaml 4.11.
  - `Ppx_deriving.string_of_{constant,expression}_opt` to destruct
    `Pconst_string` in a version-independent way
  ocaml-ppx/ppx_deriving#220, ocaml-ppx/ppx_deriving#222
  (Kate Deplaix, Thierry Martinez, review by Gabriel Scherer)

* Stronger type equalities in `Ppx_deriving_runtime` (for instance,
  `Ppx_deriving_runtime.result` and `Result.result` are now compatible with
  all OCaml versions)
  ocaml-ppx/ppx_deriving#223, ocaml-ppx/ppx_deriving#225
  (Thierry Martinez, review by Gabriel Scherer)

* `Ppx_deriving_runtime.Option` compatibility module
  ocaml-ppx/ppx_deriving#222
  (Thierry Martinez, review by Gabriel Scherer)

4.4.1
-----

* Add support for OCaml 4.10
  ocaml-ppx/ppx_deriving#211
  (Kate Deplaix, review by Gabriel Scherer)

4.4
---

* Restore support for OCaml 4.02.3
  ocaml-ppx/ppx_deriving#188
  (ELLIOTTCABLE)
* workaround Location.input_filename being empty
  when using reason-language-server
  ocaml-ppx/ppx_deriving#196
  (Ryan Artecona)
* Add support for OCaml 4.08.0
  ocaml-ppx/ppx_deriving#193, ocaml-ppx/ppx_deriving#197, ocaml-ppx/ppx_deriving#200
  (Gabriel Scherer)

4.3
---

* use Format through Ppx_deriving_runtime to avoid deprecation warning
  for users of JaneStreet Base
  (Stephen Bastians and Gabriel Scherer, review by whitequark)
* silence a ambiguous-field warning (41) in generated code
  ocaml-ppx/ppx_deriving#163
  (Étienne Millon, review by Gabriel Scherer)
* use dune
  ocaml-ppx/ppx_deriving#170
  (Rudi Grinberg, Jérémie Dimino)
* silence an unused-value warning for show
  ocaml-ppx/ppx_deriving#179
  (Nathan Rebours)

4.2.1
-----

  * Add support for OCaml 4.06.0
    ocaml-ppx/ppx_deriving#154, ocaml-ppx/ppx_deriving#155, ocaml-ppx/ppx_deriving#156, ocaml-ppx/ppx_deriving#159
    (Gabriel Scherer, Fabian, Leonid Rozenberg)
  * Consider { with_path = false } when printing record fields
    ocaml-ppx/ppx_deriving#157
    (François Pottier)

4.2
---

  * Add support for OCaml 4.05.0.
  * Use the `ocaml-migrate-parsetree` library to support multiple
    versions of OCaml.
  * Fix comparison order of fields in records (ocaml-ppx/ppx_deriving#136).
  * Silence an `unused rec flag` warning in generated code (ocaml-ppx/ppx_deriving#137).
  * Monomorphize comparison function for builtin types (ocaml-ppx/ppx_deriving#115)
  * Raise an error when `type nonrec` is encountered (ocaml-ppx/ppx_deriving#116).
  * Display an error message when dynamic package loading fails.
  * Add a `with_path` option to `@@deriving` to skip the module path
    in generated code (ocaml-ppx/ppx_deriving#120).

The homepage for the project has now moved to:
<https://github.com/ocaml-ppx/ppx_deriving>

4.1
---

  * Fix type error with inheritied polymorphic variant type in
    [@@deriving map].
  * Fix incorrect handling of multi-argument constructors in
    [@@deriving show].
  * Add API hooks for ppx_type_conv.

4.0
---

  * Show, eq, ord, map, iter, fold: add support for `Result.result`.
  * Ppx_deriving.Arg: use Result.result instead of polymorphic variants.
  * Ppx_deriving.sanitize: parameterize over an opened module.
  * Add support for `[@@deriving]` in module type declarations.
  * Add support for loading findlib packages instead of just files in
    ppx_deriving_main.
  * Treat types explicitly qualified with Pervasives also as builtin.
  * Compatibility with statically linked ppx drivers.

3.1
---

  * Show, eq, ord: hygienically invoke functions from referenced modules
    (such as X.pp for X.t when deriving show) to coexist with modules
    shadowing ones from standard library.
  * Iter, map, fold: hygienically invoke List and Array functions.

3.0
---

  * Implement hygiene: Ppx_deriving.{create_quoter,quote,sanitize,with_quoter}.
  * Show, eq, ord: add support for `lazy_t`.
  * Add support for `[@nobuiltin]` attribute.
  * Add Ppx_deriving.hash_variant.
  * Remove allow_std_type_shadowing option.
  * Remove Ppx_deriving.extract_typename_of_type_group.

2.1
---

  * Fix breakage occurring with 4.02.2 w.r.t record labels
  * Fix prefixed attribute names (`[@deriving.foo.attr]` and `[@foo.attr]`).
  * Add allow_std_type_shadowing option for eq and show.

2.0
---

  * Add support for open types.

1.1
---

  * New plugin: create.
  * Show, eq, ord: handle `_`.
  * Show, eq, ord, map, iter, fold: handle inheriting from a parametric
    polymorphic variant type.
  * Make `Ppx_deriving.poly_{fun,arrow}_of_type_decl` construct functions
    in correct order. This also fixes all derivers with types with
    more than one parameter.
  * Add `Ppx_deriving.fold_{left,right}_type_decl`.

1.0
---

  * Make deriver names lowercase.
  * Remove Findlib+dynlink integration. All derivers must now be
    explicitly required.
  * Allow shortening [%derive.x:] to [%x:] when deriver x exists.
  * Make `Ppx_deriving.core_type` field optional to allow ignoring
    unsupported [%x:] shorthands.
  * Add support for [@@deriving foo { optional = true }] that does
    not error out if foo is missing, useful for optional dependencies.
  * Rename ~name and ~prefix of `Ppx_deriving.attr` and
    `Ppx_deriving.Arg.payload` to `~deriver`.
  * Renamed `Ppx_deriving.Arg.payload` to `get_attr`.
  * Add `Ppx_deriving.Arg.get_expr` and `get_flag`.

0.3
---

  * Show, Eq, Ord, Iter, Fold: handle ref.
  * Show: handle functions.
  * Show: include break hints in format strings.
  * Show: pull fprintf into local environment.
  * Show: add `[@polyprinter]` and `[@opaque]`.
  * Add `Ppx_deriving.Arg.expr`.

0.2
---

  * New plugins: Enum, Iter, Map, Fold.
  * All plugins: don't concatenate affix if type is named `t`.
  * Add `[%derive.Foo:]` shorthand.
  * Show, Eq, Ord: add support for list, array, option.
  * Show: include full module path in output, including for types with manifest.
  * A lot of changes in `Ppx_deriving interface`.

0.1
---

  * Initial release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants