Add a Repr module to the standard library

[kit-ty-kate]

First proposed in #13753 (comment)

As previously extensively discussed in #9928, #9080 and in many tickets or forum posts, both the physical equality operator == and the polymorphic comparison functions compare and = are known footguns for OCaml developers of all levels.

Similarly to #9080 / #13753, this PR hopes to be a first step towards a world where OCaml does not have this footgun anymore by encouraging people to use functions from this new module (which is more explicit than using it directly from Stdlib and thus less prone to footgunning), then deprecating ==, compare, … when they are less used.

Note for reviewers: i'll add documentation to the functions once/if the interface is agreed

[nojb]

I am in two minds about these submodules.

The main use of them is with numeric types (Int, Int64, Char, maybe String, etc). If anything, monomorphic versions of these operators should be added to those modules. I don't think the operators <, >, <=, >= are very useful in contexts where one is explicitly using a polymorphic compare (since the ordering is implementation-defined). As for = and <>, they are useful, but once you have put them inside a submodule, I don't see myself using a local open to save a few character with respect to Compare.Poly.equal.

In other words, perhaps it would be simpler not to include the Syntax submodules at all (which would also mean that we do not have to discuss what to name the submodule).

[gasche]

In other words, perhaps it would be simpler not to include the Syntax submodules at all (which would also mean that we do not have to discuss what to name the submodule).

Yes, that would also be my preference to reduce decision fatigue.

[kit-ty-kate]

fair enough, that works for me. Done.

[nojb]

LGTM

Docstrings need to be added.

And a second official approval is needed to move forward with this PR.

[kit-ty-kate]

Docstrings need to be added.

what do you think makes the most sense for this module? Should we copy the docstrings from stdlib.mli or should we simply say something like Alias of {!Stdlib.compare}, or even Alias of {!Compare.Poly.compare} in stdlib.mli if we want to move the full description to the new module?

[gasche]

Actually it may be nice to do things the other way around, that is, move the stdlib docstrings here and point to this new module in the Stdlib documentation. This paves the way for eventually deprecating Stdlib.compare if we decide to.

[gasche]

I am not sure about the proposed API: Compare.Poly.compare is a mouthful, maybe there is a way to organize this module that makes the name nicer?

For example, I thought of the following "flat" interface (I'm not saying I strongly prefer it, but I think that throwing some ideas around may help decide how we feel about the API):

val phys_equal : 'a -> 'a -> bool
val equal : 'a -> 'a -> bool
val compare : 'a -> 'a -> int
val min, max: 'a -> 'a -> 'a

We loose the information that min, max depend on structural ("poly") comparison, but then it is unclear to me that they could rely on anything else.

Compare.compare still reads a bit weird, and maybe this interface is not as good in terms of showing warning signs about which functions rely on ad-hoc polymorphism.

[redianthus]

Compare.Poly.compare is a mouthful, maybe there is a way to organize this module that makes the name nicer?

We could simply have Poly.compare ?

[nojb]

I thought of the following "flat" interface

In that case, better call the module "Poly".

[yallop]

I think it'd be good to have a name than emphasizes the fact that the behaviour is defined on representations, i.e. that the functions are not parametrically polymorphic, despite their types.

[dbuenzli]

Generic.compare ?

[nojb]

I think it'd be good to have a name than emphasizes the fact that the behaviour is defined on representations, i.e. that the functions are not parametrically polymorphic, despite their types.

Repr?

[dbuenzli]

A twist on @nojb's proposal could be:

Compare.repr : 'a -> 'a -> int
Compare.repr_equal : 'a -> 'a -> bool
Compare.phys_equal : 'a -> 'a -> bool
Compare.repr_min : 'a -> 'a -> 'a
Compare.repr_max : 'a -> 'a -> 'a

[kit-ty-kate]

I find the Compare.repr* alternative proposal quite elegant (I'm also ok with the Repr proposal). The only "negative" comment i would have is that for beginners it might feel a bit obscure, but much like Obj maybe this is actually something positive and might invite people to read the documentation instead of reading/using it by default without thinking much about it.

I'm happy to update the PR with the proposal that makes consensus.

[gasche]

I think that there is a tension between having ominous-looking names that make people think twice about using polymorphic comparison, and having pleasant-looking names that encourage people to use them and gradually organize a transition out of Stdlib.compare.

In this context I personally find that Generic.compare is better than Compare.repr: it is clear that this comparison is doing something weird, but the name itself reads well -- someone can easily guess what the function does, even without looking at the type information. On the other hand, I have ideas for future extensions of a Compare module (for example a helper for lexicographic ordering with a thunked unit -> int second parameter; or maybe a type t = Lt | Eq | Gt to eventually graduate out of magical numbers), while I suppose Generic would be restricted to weird runtime support -- I guess we could move input_value and output_value there as well, for example.

[kit-ty-kate]

I've pushed two commits with the updated docstrings to help visualize the two alternative proposals.
I have some additional thoughts:

• Compare.repr*:
  • Pros: introduces a Compare module which i might see people wanting to add more features to it in the future (e.g. Compare.S, operators, maybe things about implicits why not)
  • Cons: longer names. Maybe a bit awkward to use repr without its module name as opposed to compare
• Repr.compare:
  • Pros: naming is concise, straightforward and enjoyable to use
  • Cons: i'm having trouble describing the module in more general terms that doesn't sound like "Obj but meant to be used by non-advanced users" or "Comparison function" which describes the current state of the module but feels detached from its name. I also can't think of other things to add to that module in the future that would fit its name.

[dbuenzli]

• Cons: i'm having trouble describing the module in more general terms that doesn't sound like "Obj but meant to be used by non-advanced users"

Well it could be for operations on the representation of values "that you are allowed to use". For example Obj.reachable_words could be moved there.

So a synopsis could be:

(** Operations on the representation of values. *)
module Repr : sig 
…
end

[yallop]

I also can't think of other things to add to that module in the future that would fit its name.

One often-requested example: Repr.print. Similar functions that are already in the standard library (e.g. Marshal.to_string, Obj.size) would also fit well, if they weren't already situated elsewhere.

[OlivierNicole]

It seems the principle and latest module layout are accepted by everyone; the current name looks nice to me and doesn’t seem to bother anyone too much. Should we go ahead and merge?

[nojb]

Should we go ahead and merge?

The protocol says two approval by core devs are needed before merging changes to the standard library. We are not there yet :)

[kit-ty-kate]

Is Repr the consensus for everyone? Anyone with strong opinions for or against?

So far we had the following proposals which mostly differ on what they invite in terms of future development:

• Repr could invite to move things slightly more high level from Obj and Marshal
• Compare could invite the addition of type t = Lt | Eq | Gt and other comparison utilities
• Generic: could invite to move the Stdlib.*_value functions and similar things
• Poly seems to have been rejected by Add a Repr module to the standard library #13755 (comment)

What is the most compelling? Can anyone think of more things that these modules might be suited for?

[dbuenzli]

• Repr could invite to move things slightly more high level from Obj and Marshal
• Compare could invite the addition of type t = Lt | Eq | Gt and other comparison utilities

Personally I don't see these two as mutually exclusive.

[OlivierNicole]

I agree they are not mutually exclusive, and the current functions belong more to Repr than to Compare to me.

I don’t like Generic because it is less descriptive than Repr and sounds too fancy and alluring for functions that should, precisely, only be used when one knows what they’re doing.

[kit-ty-kate]

not mutually exclusive

Fair enough. I've changed the PR title and implemented the suggestions. How does the PR sound in its current state?

I'll rebase in a minute to resolve the conflicts in the changelog and it should be good to go once approved.

[OlivierNicole]

This is still missing a second approval. @nojb Since you have reviewed a previous version of the changes, would you be available to review the new one?

[nojb]

This is still missing a second approval. @nojb Since you have reviewed a previous version of the changes, would you be available to review the new one?

Thanks for the ping. Semantically, I am in agreement with the PR, but since most of the discussion has revolved around naming choices, which I care very little about, I think someone who is more interested in this aspect of things should make the final call.

[nojb]

Maybe @gasche or @yallop could be coaxed to do the second review :)

[yallop]

I've approved, but have two quick final comments:

• the name should be centred in the copyright heading, to match other files
• I'd prefer "defined on" rather than "based on"

[kit-ty-kate]

• the name should be centred in the copyright heading, to match other files
• I'd prefer "defined on" rather than "based on"

done

[dinosaure]

Sorry to come after the merge, but repr is currently being used one by one mirage library (used in projects like irmin or tezos): https://github.com/mirage/repr. I don't really have an opinion on the name, but it seems that this module name will impact this library?

[dbuenzli]

I don't think so. It's just that people who want to access the standard library Repr modules in sources that compile against this library will have to mention it as Stdlib.Repr.

[Octachron]

Indeed, the standard library is essentially opened with a special "weak" open that doesn't shadow existing persisting modules in the environment.