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

Improved API for toplevel printers #7770

Closed
vicuna opened this issue Apr 11, 2018 · 21 comments
Closed

Improved API for toplevel printers #7770

vicuna opened this issue Apr 11, 2018 · 21 comments

Comments

@vicuna
Copy link

vicuna commented Apr 11, 2018

Original bug ID: 7770
Reporter: @Drup
Status: acknowledged (set by @xavierleroy on 2018-05-23T18:13:32Z)
Resolution: open
Priority: normal
Severity: feature
Category: toplevel
Monitored by: @nojb @Drup @diml @hcarty @dra27 @dbuenzli

Bug description

The current API for toplevel printers is not particularly convenient. It either require manual work from the user or ocamlfind predicate tricks.
One potential solution would be to annotate type declarations:

type foo = ...
[@@printer pp_foo]

When the toplevel tries to print an object of type foo, we lookup the definition, find the annotation, and that's it.

This ticket aims to discus the design of this before I start implementing it. There are various potential choices. pp_foo will be almost always declared after the current module, so we can't really typecheck at declaration time.
Either:

  1. We assume pp_foo is local to the current module and we check its type once we are done with the rest of the module.
  2. We don't typecheck anything statically and instead check dynamically, just like #install_printer. This solution also allows printers that are not local to the current scope (as long as they are in scope for printing, it's fine).
  3. We wait for modular implicit, which provide a similar solution to the problem.

Opinions ?

@vicuna
Copy link
Author

vicuna commented Apr 11, 2018

Comment author: @dbuenzli

Nice to look at this Garbiel ! Can be related to #7589 point 5.

I guess these annotations will go in cmis ? Didn't think it through but what about annotating the pretty printer with the type instead ?

If two annotations declare the same type the last one loaded takes over. This also has the advantage of being able to override or provide alternate pretty printers by simply loading a cmi.

@vicuna
Copy link
Author

vicuna commented Apr 11, 2018

Comment author: @dbuenzli

In fact I guess you don't even need to specify the type. You'd just have an [@@toploop] annotation and any function that sports this annotation when a cmi is loaded automatically given to #install_printer. Not sure if that's feasible though.

EDIT That's actually what Jérémie suggested here:

ocaml/dune#114 (comment)

@vicuna
Copy link
Author

vicuna commented Apr 11, 2018

Comment author: @Drup

I didn't know jeremy got the same idea. Yes, the annotations would go in the cmis.

I'm not very convinced by the idea of annotating the printers since module types are loaded lazily in OCaml, in particular with module aliases.

Basically, if you have a Foo_printers module, or even Foo.Printers where Printers is a module alias, unless the cmi is loaded by other means (like calling Foo_printers.pp explicitly), the printer annotation will never be seen.
Annotating on the type avoids the issue.
Having your printers depend on which cmi the typechecker decided to load seem very unfriendly to me.

As far as order of printer is concerned, I would go like this:

  • Starts from the type proposed by -short-alias
  • For each type alias, try to find a printer at the type definition, if unsuccessful, resolve one layer of alias, and try again.

@vicuna
Copy link
Author

vicuna commented Apr 11, 2018

Comment author: @alainfrisch

An alternative approach would be to use some naming convention (deriving-like). For a value of a type [Foo.Bar.my_type], the toplevel would look e.g. for a printer
called [print_my_type] of type [Format.formatter -> Foo.Bar.my_type -> unit] in module [Foo.Bar].

This is a bit ad hoc but for the toplevel this might be ok.

@vicuna
Copy link
Author

vicuna commented Apr 15, 2018

Comment author: @Drup

firsch: That's strictly less powerful than 1, and not much easier to implement. It's also far less flexible/retro-compatible. It doesn't even really help all that much for a future migration to a modular implicit based approach.

@vicuna
Copy link
Author

vicuna commented Apr 16, 2018

Comment author: @diml

The reason I suggested annotating the printer rather than the type is that if you annotate the type, you end up typing the printer identifier twice. It seems to me that toplevel printers are generally defined in the same module as the types themselves, so the typer should see them.

@vicuna
Copy link
Author

vicuna commented Apr 16, 2018

Comment author: @Drup

dim: This means you have one reasonable case (printer is local) and the other cases which might or might not work depending on the whims of the typechecker. I think it makes more sense to either support that one good case (proposition 1) or all the cases (proposition 2).

@vicuna
Copy link
Author

vicuna commented May 17, 2018

Comment author: @diml

Personally, I prefer proposition 1 as we get errors immediately. When things are not checked, they tend to quickly get out of sync.

@vicuna
Copy link
Author

vicuna commented Jan 16, 2019

Comment author: @diml

Drup, unless you are planning to look at this in the near future, I'm going to give a shot at implementing this feature.

@vicuna
Copy link
Author

vicuna commented Jan 16, 2019

Comment author: @Drup

My time is very limited lately, so go ahead! I'll be happy to review and/or help when needed.

@vicuna
Copy link
Author

vicuna commented Jan 30, 2019

Comment author: @diml

I had a first look at this, and thought a bit more about how we could adapt it so that we could get maps and sets printed in the toplevel.

I get the feeling that this is going to turn into a less good implementation of implicits. So I'm leaning towards simply abandoning this feature and waiting for implicits instead. In the meantime, the hack like the one I did in utop should be enough.

@vicuna
Copy link
Author

vicuna commented Feb 4, 2019

Comment author: @diml

Just to clarify, it doesn't seem worth to me to invest too much time trying to solve this problem in a general way given than implicits will do exactly that in a more principled way.

However, we could import the same patch as I wrote for utop:

ocaml-community/utop@fa3880d

It doesn't solve the problem in a general way, but is an improvement over the current situation until we get the proper solution.

@vicuna
Copy link
Author

vicuna commented Feb 5, 2019

Comment author: @dbuenzli

Having this only in utop is kind of useless since there are many places where it's not used (e.g. on the web). This means we still need to keep our lib_top_init.ml files around for these places (see for example 1 where this came up).

It may not solve the problem in a general way but then we don't even have an ETA on implicits. So if we can ensure this design will not collide with a future better one I'd rather have this in now even if that may solve only 95% of the cases. It seems the patch is rather light.

If you have time to upstream it @dim that would be very much appreciated.

@vicuna
Copy link
Author

vicuna commented Feb 6, 2019

Comment author: @diml

Yh, I agree. TBH the patch itself is trivial, it's more everything around that always takes time: doc, tests, PR description, etc...

I have a few more urgent things to look at, but I'll come back to this once my stack clears up a bit.

@github-actions
Copy link

github-actions bot commented May 7, 2020

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 7, 2020
@Drup
Copy link
Contributor

Drup commented May 7, 2020

While we do have a "fix" in utop, I agree with @dbuenzli that @jeremiedimino's solution should be upstreamed. If you already have a patch around, I'll be happy to help get it merged.

@avsm avsm removed the Stale label May 7, 2020
@ghost
Copy link

ghost commented May 7, 2020

I don't have a patch at hand, but the code that does that in utop is small and self-contained and should be easy to extract:

https://github.com/ocaml-community/utop/blob/471cb311159773bbb280d0f1a4b47c54456c67a9/src/lib/uTop_main.ml#L642

Essentially, it "monitors" new cmi files by setting Persistent_signature.load, scan them for the [@@toplevel_printer] attribute and calls Topdirs.dir_install_printer on all the collected printers.

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 12, 2021
@gasche
Copy link
Member

gasche commented May 12, 2021

Here is my understanding of the PR:

  • The first proposal, which is to attach annotations to types to be able to resolve printers for them, is judged too ad-hoc (compared to runtime type representations or modular implicits) and abandoned.
  • There is another proposal which is to integrate a feature of utop where printers can be marked with an attribute instead of having to run #install_printer ... directives in toplevel-specific code. This was proposed by @diml in February 2019, and it sounds reasonable, @diml and @Drup discussed it but so far no one has put the time into turning the utop code into a PR.

Has the situation changed? Is there interest in submitting this proposal as an OCaml feature?

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 16, 2022
@nojb nojb removed the Stale label May 16, 2022
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

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

Successfully merging a pull request may close this issue.

5 participants