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

Top-level custom printing for GADTs: interface change in 4.02.2 #6908

Closed
vicuna opened this issue Jun 18, 2015 · 7 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Jun 18, 2015

Original bug ID: 6908
Reporter: @yallop
Status: closed (set by @damiendoligez on 2015-07-20T13:15:02Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.2
Target version: 4.02.3+dev
Fixed in version: 4.02.3+dev
Category: ~DO NOT USE (was: OCaml general)
Related to: #5958
Monitored by: @gasche @diml @hcarty

Bug description

PR5958 changed the behaviour of #install_printer to make it possible to install printers for parameterised types. Unfortunately the change has broken some existing code for printing GADTs.

For example, if I have the following GADT definition

type (_, _) eql = Refl : ('a, 'a) eql

then the following printer works with #install_printer in 4.02.1, but is rejected in 4.02.2:

let format_eql : 'a 'b. Format.formatter -> ('a, 'b) eql -> unit =
fun fmt _ -> Format.pp_print_string fmt "reflexivity"

and the following printer works with #install_printer in 4.02.2 but is rejected in 4.02.1:

let format_eql : 'a 'b.
(Format.formatter -> 'a -> unit) ->
(Format.formatter -> 'b -> unit) ->
Format.formatter -> ('a, 'b) eql -> unit =
fun _ _ fmt _ -> Format.pp_print_string fmt "reflexivity";;

This is a problem in practice. For example, top-level printing no longer works in ctypes, which installs a number of printers of the first form:

https://github.com/ocamllabs/ocaml-ctypes/blob/cfb72eeb/src/ctypes-top/ctypes_printers.mli#L10

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

Comment author: @gasche

I see no better solution than making the directive code more clever to detect types that (1) have type parameters but (2) do not expect printers for those parameters, and use the old behavior on those.

Maybe we could generalize this to accept that only a subset of the parameters expect printers (synthesizing printers does not make sense for phantom or even contravariant parameters). But a first goal should be to have a safe patch that solve ctypes immediate problem.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

Comment author: hnrgrgr

Attached is a simple patch that should restore the compatibility.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

Comment author: @gasche

Would it be possible to have tests for the expected behavior(s) in the testsuite, for example a new file tests/tool-toplevel/install_printer.ml?

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 19, 2015

Comment author: @yallop

Thanks for the quick response! The patch works for me.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jun 22, 2015

Comment author: @yallop

Another data point: the interface change is also incompatible with the code-printing function in MetaOCaml, which has the following type:

val print_code : Format.formatter -> 'a code -> unit

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 8, 2015

Comment author: @damiendoligez

I have a question about the patch: it looks like the comment is not true any more? If this is the case, it should be removed.

@vicuna

This comment has been minimized.

Copy link
Author

commented Jul 20, 2015

Comment author: @damiendoligez

I have removed the comment and applied the patch to 4.02 (rev. 16221).

@vicuna vicuna closed this Jul 20, 2015

@vicuna vicuna added this to the 4.02.3 milestone Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.