No more GADTs #292
No more GADTs #292
Conversation
This patch replaces the GADTs in Paths/Paths_types with polymorphic variants. The resulting code is more verbose, but significantly easier to understand. There is one area that still needs some thought - the 'maps.ml' module. It currently allows the same overriding that was possible before, but the typing is now more permissive. In the vast majority of cases this will be fine, but it would be nice to close this particular hole. Signed-off-by: Jon Ludlam <jon@recoil.org>
|
Failed on 4.02.3 with "this ground coercion is not principal" |
|
I don't think it's a good idea to support 4.02.3 anyways since there were quite a few bugs with The backport for 4.02.3 was done because because of bucklescript (see #179). It seems they are now on track for 4.06 (see rescript-lang/rescript-compiler#3319). So I think we can drop 4.02.3 support now. |
|
Also, the ground coercion issue is discussed on MPR#7135 and fixed for 4.03. |
|
That MPR mentions a workaround is it applicable here ? |
|
It's only a warning, so we can just suppress it (which is what was changed in MPR#7135 that @trefis mentioned). The other problems with 4.02.3 are to do with not having String.equal or Char.equal, so it's not a big deal to make it work. |
- Disable warning 18 ("This ground coercion is not principal")
- Use polymorphic equality for strings and chars
- result -> Result.result
The first two should be reverted when we remove support for < 4.03
Signed-off-by: Jon Ludlam <jon@recoil.org>
|
Would it be okay to postpone the merging of this PR until the next release is cut?
Happy to see this happen after we have a BuckleScript release supporting 4.06 🙌 |
|
What's the reasoning behind delaying? I'm keen not to maintain this patch out of tree for long. |
|
To be clear, this PR doesn't conflict with the work that we're awaiting confirmation from @dbuenzli on (https://github.com/dbuenzli/odoc/tree/fix-html-cmds), nor #261 — it could very well be the case that merging it doesn't affect the release at all. But I'd also like to avoid the risk of it delaying the release, considering we are looking forward to it to adopt odoc a little more widely in the Reason/BuckleScript ecosystem. For more context, here's the last release request issue: #288. |
|
Not sure it applies in this particular case, but I think that generally when you find yourself delaying merging patches before a release then it is probably time to cut a release branch so that development can continue without disruption. |
This has been merged. See #289. |
|
I don't mind at all waiting for a bit if the release can be done in the not-too-distant future - I was expecting the review to take a bit of time :-) |
|
@Ostera This patch does not delay the release in any substantial way, provided we don't find any major snag in review of course, which I don't expect. |
The Reference coercions (e.g. Reference.module_of_t) were a little
too strict in what they accepted. This patch fixes that by ensuring that
`Paths_types.Reference.tag_{module,type,etc.}` are exactly the types
allowed by `Paths.Reference.Module.t` and others.
This doesn't seem to have been a problem in practice, but was spotted
while reviewing the no-more-gadts patch.
Signed-off-by: Jon Ludlam <jon@recoil.org>
There was a problem hiding this comment.
This looks largely ok to me.
@jonludlam can you briefly comment on the rationale for making this change?
@lpw25 and/or @trefis, does the new representation look reasonable to you, especially with respect to any planned work?
|
For reference, this patch is hugely inspired by work started by @lpw25 here: https://github.com/lpw25/doc-ock/tree/next - it's one of the first items in the 'model rewrite' project outlined here: https://github.com/ocaml/odoc/wiki/Outline-of-the-model-rewrite-project |
Signed-off-by: Jon Ludlam <jon@recoil.org>
Suggested by @aantron. Signed-off-by: Jon Ludlam <jon@recoil.org>
Yeah, these changes look right to me. They should be easier to work with and easier to evolve. I've regretted my decision to use GADTs with polymorphic variant indices for a while now -- the language just isn't able to handle them properly leading to things like the |
|
Thanks @jonludlam and @lpw25. This looks good to me. Let me know if/when you are done making changes, and I will merge. |
|
I think I'm done, but I wouldn't mind if you wanted to hold off a little until releasing? |
|
Thanks! |
Warning: this is a large patch.
This patch replaces the GADTs in Paths/Paths_types with polymorphic
variants. The resulting code is more verbose, but significantly easier
to understand. Additionally a few instances of
Obj.magicwere removedwhere OCaml wasn't quite smart enough to figure out that types
were actually OK.
Previously we had a GADT whose type index was a set of polymorphic
variants. This allowed us to define subsets of the constructors to use
in different contexts and also to declare functions that were polymorphic
on the index and could be guaranteed to return the same subset that
were passed in.
What we now have are sets of polymorphic variants, many of which
contain identically typed constructors and can be viewed as having
subset-superset relations. To replace the functions that were
polymorphic on the index we have to write multiple functions, though
these can use pattern matching to coerce to the smaller type and hence
the superset functions can simply call the subset functions.
This PR has been tested by running
odig odocover my installed opamswitch and diffing the results. I saw nothing other than hash differences
in the resulting docs.
There is one area that still needs some thought - the 'maps.ml' module.
It currently allows the same overriding that was possible before, but
the typing is now more permissive. In the vast majority of cases this
will be fine, but it would be nice to close this particular hole.
Signed-off-by: Jon Ludlam jon@recoil.org