Remove preprocessing of backwards-compatibility code

[Leonidas-from-XIV]

This PR attempts to completely remove preprocessing code via the use of shadowing. It works on 4.14, let's see if it works on older versions as well.

[kit-ty-kate]

What are you trying to achieve with this? I disagree with this at the very least Unix.realpath as the compatibility code is not thread safe and that would break opam-repo-ci (see #4961)

[Leonidas-from-XIV]

The idea is to get rid of preprocessing altogether (which would make it easier for e.g. Merlin).

I don't see how this is a regression, since both before and after this PR Unix.realpath is used on 4.13 and the fallback before.

[kit-ty-kate]

Ah nevermind sorry, I hadn't noticed the order of inclusion in

include OpamCompatPolyfill.Unix
include Unix

Looking at the code I'm wondering if we really need this new OpamCompatPolyfill module. Couldn't it be done using e.g. something like this:

module Unix = struct
  let realpath s =
    [...]
  
  include Stdlib.Unix [@@warning "-33"]
end

[Leonidas-from-XIV]

@kit-ty-kate You're right, I've been thinking too complicated, simplified it now. The only place where I need an additional Polyfill module is Either, because I need to open two modules of which one (4.08-4.12) or both (4.12+) declare Either without mentioning Either otherwise it will fail on older versions.

[Leonidas-from-XIV]

@kit-ty-kate Through your simplifications the OpamCompat.Foo modules cannot be used in place of the Stdlib.Foo modules anymore. Not sure if that's an important aspect, but my idea was to preserve that behavior if possible.

[kit-ty-kate]

We don't really need for OpamCompat.Either.t to be equal to Stdlib.Either.t so we can simplify that further. I've done that in 8f9105d

The current state of the PR is rather nice and it simplifies things a bit. However something that's a bit annoying is that it doesn't really cover for typos.
For example if at some point we made a mistake and named Unix.reelpath instead of Unix.realpath or something like that and the change is propagated to the definition and uses (easy to make that kind of mistake with copy/paste), we don't really get alerted that we are not using the real Unix.realpath in 4.13, as opposed to the old generator which would've forced us to detect that issue in CI as that line would become module Unix = Unix

@kit-ty-kate Through your simplifications the OpamCompat.Foo modules cannot be used in place of the Stdlib.Foo modules anymore. Not sure if that's an important aspect, but my idea was to preserve that behavior if possible.

To me that's not important, it's actually better to not have them so we can detect which compatibility functions we actually have/use easier.

[kit-ty-kate]

One thing that's also missing now is a way to know from a glance, which functions to remove when we'll eventually raise the minimum required version of OCaml in a few years.

Maybe a simple comment like this would be enough?

[Leonidas-from-XIV]

For example if at some point we made a mistake and named Unix.reelpath instead of Unix.realpath or something like that and the change is propagated to the definition and uses

True, but it is only a problem in the OpamCompat module and I guess there you generally need to be a bit careful. An option to fix this is to continue exposing the whole module and then restrict it with the signature of the module that is being emulated (something akin to : sig module type of Unix end, but for older versions you'd have to add val realpath again you don't win too much by this).

[hannesm]

First of all, I do appreciate this PR.

Minor nit: I'd put the version comment (as well) in the implementation, not only the interface (since that's where merlin pushes you).

Another minor comment: I dislike warning numbers, and since (actually since when?) OCaml supports as well [@@@warning "-unused-value-declaration"] -- which IMHO makes it much more readable for a future reader.

[kit-ty-kate]

Another minor comment: I dislike warning numbers, and since (actually since when?) OCaml supports as well [@@@warning "-unused-value-declaration"] -- which IMHO makes it much more readable for a future reader.

This is only applicable since OCaml 4.13 so for the three cases we have here it would probably work but in the general case it wouldn't.

[Leonidas-from-XIV]

Minor nit: I'd put the version comment (as well) in the implementation, not only the interface (since that's where merlin pushes you).

I would've assumed Merlin can pull these comments from the interface as well? Maybe if we marked them as doc-comments. But I think it is no harm and possibly useful to add them in the implementation as well, added that.

This is only applicable since OCaml 4.13 so for the three cases we have here it would probably work but in the general case it wouldn't.

A shame because I really liked the idea (and share Hannes' dislike for the arbitrary numbers). Well at least now I know this is possible going forward in 4.13.

[kit-ty-kate]

Thanks