Extensible types in pattern matching.#1459
Extensible types in pattern matching.#1459maranget merged 7 commits intoocaml:trunkfrom maranget:pr7661-squashed
Conversation
gasche
left a comment
There was a problem hiding this comment.
The logic to distinguish extension constructors is duplicated in two places: in the MayCompat functor argument, and in the ctx_matcher function. Would it be possible to factor the logic out of an auxiliary function used in both places? (For example it could be a constructor_description -> constructor_description -> [Compatible | Incompatible | Unknown] function :-)
Could we improve this test by looking at the types of the parameters? For types as well we know that two types are equal, may be equal or must be different. If the extension constructors have an argument position where the two types must be different, we know they are distinct.
typing/parmatch.mli
Outdated
| val compats : pattern list -> pattern list -> bool | ||
|
|
||
| (* Exported compatibility, | ||
| "may_compat p q" returns true when p and q never admit a common instance; |
There was a problem hiding this comment.
Well done, read return false
typing/env.mli
Outdated
| | Env_type of summary * Ident.t * type_declaration | ||
| | Env_extension of summary * Ident.t * extension_constructor | ||
| | Env_module of summary * Ident.t * module_declaration | ||
|
|
There was a problem hiding this comment.
You probably didn't intend to commit that.
typing/parmatch.mli
Outdated
| "may_compat p q" returns true when p and q never admit a common instance; | ||
| returns true when they may have a common instance. *) | ||
| val may_compat : pattern -> pattern -> bool | ||
| val may_compats : pattern list -> pattern list -> bool |
There was a problem hiding this comment.
Considering that may_compat and may_compats are not used (and indeed shouldn't be used) in parmatch, instead of defining them in parmatch and exporting them, could we export the functor and define the functions in matching.ml?
There was a problem hiding this comment.
I'd rather leave this to a later refactoring, such as the one you have started. The functor belongs to some utility module. I remember that have created such a utility module (for printing).
There was a problem hiding this comment.
The functor doesn't seem to belong to an external utility module any more than may_compat itself. And doing the change now instead of later reduces the probability of people using the wrong function by mistake.
Your choice. :)
typing/parmatch.ml
Outdated
| | [], [] -> true | ||
| | p::ps, q::qs -> compat p q && compats ps qs | ||
| | _,_ -> assert false (* By typing *) | ||
| end |
There was a problem hiding this comment.
The indentation looks weird.
There was a problem hiding this comment.
It is weird, thanks.
bytecomp/matching.ml
Outdated
| (* NB: matcher_constr applies to default matrices. | ||
|
|
||
| In that context matching by constructors of extemsible | ||
| types degardes to arity checking, due to potential rebinding. |
|
@gashe The logics is very simple, and the code is not so easy to factorize. I see no reason to change As to you suggestion of considering types, I would not delay bug fix for this. |
|
My suggestion was not meant as an immediate implementation requirement, but I think that (if you find it reasonable) it suggests that this logic may evolve in the future and it's important to have it one place only. Do I correctly understand, in your reply, that it is okay if the compatibility test becomes more permissive (for example reasoning on types), but |
|
Well I am not sure I understood your suggestion correctly. Let us reconsider all this. You suggest having a function somewhere (In Types where we already have My answer is that I do not know without trying, whether such a solution is feasible, without significant alterations of typically |
|
It's not particularly important, but I would suggest that an extension constructor compatibility test should probably live in |
|
The factorization looks nice.
Sounds fine to me. |
|
Yes, the factorization is nice -- I didn't mean to seriously insist about the three-value return idea. Thanks! (I haven't had the time yet to do a careful review of the whole patch.) |
|
@gasche, @trefis, @damiendoligez I believe this patch is ready for being merged, would you do a formal review, please? Tu veux bien être mon ami ? |
|
Hi, As I said off-line, I currently do not understand all of the bits in |
trefis
left a comment
There was a problem hiding this comment.
Apart from the Changes nitpick this looks correct to me.
Changes
Outdated
| ### Bug fixes | ||
| - MPR#7661: fix faulty compilation of patterns of extension types | ||
| (Luc Maranget, review by Thomas Refis and Gabriel Scherer, report by | ||
| Abdelraouf Ouadjaout and Thibault Suzanne) |
There was a problem hiding this comment.
IIRC we want to keep things ordered by MPR/GPR number in the changelog, so this is misplaced.
There was a problem hiding this comment.
Yes, and the entry should also reference the GPR: MPR#7661, GPR#1459: ...
…Types.may_equal_constr
…l to matching.ml. Various consequences.
|
@maranget I changed the way you rebased your PR -- next time we can do it together so that I show you my workflow. It would be nice to squash all the commits into one, as they don't mean much independently of each other (and the commit messages, such as "Oups", do not need to be preserved for eternity). Would you like to do it yourself or should I take care of it? |
|
@maranget I changed the way you rebased your PR -- next time we can do it together so that I show you my workflow.
It would be nice to squash all the commits into one, as they don't mean much independently of each other (and the commit messages, such as "Oups", do not need to be preserved for eternity). Would you like to do it yourself or should I take care of it?
Why not keep "Oups" ?
If clicking on the "squash and merge" button does the job, I can do it.
…--Luc
|
|
This was not the right way to do it (the commit message sucks and the global squash is too large), but I guess I should have complained earlier. When you use "squash and merge", (1) you lose any ability to have more than one commit, and (2) you have to remove the useless junk in the commit message (and hopefully write something readable) because by default it concatenates all the commit messages of all squash commits. |
|
Too late... |
* Handle extensible types * various typos * Oups. * Astract constructor equality that considers potential rebinding into Types.may_equal_constr * Moved rebinbing aware pattern compatibility functions from parmatch.ml to matching.ml. Various consequences. * Change Changes * change the Changes change
|
compiling jbuilder with trunk gives me the following error: |
This reverts commit 7f5f82d.
This reverts commit 7f5f82d.
This reverts commit 7f5f82d.
This reverts commit 7f5f82d.
(cherry picked from commit 7f5f82d)
- local blogs get their own dedicated page that lists their posts at https://ocaml.org/blog/[SOURCE_ID], as well as their own RSS feed at https://ocaml.org/blog/[SOURCE_ID]/feed.xml. - For more clarity regarding the input format: we explicitly distinguish between the local blogs hosted on ocaml.org (they now live in the folder data/planet-local-blogs), and the external sources and individual posts (living in data/planet) when parsing.
This pull request fixes MPR7661. The problem originated from incomplete handling of "extension" types.
Fixes mostly consist in
compatis the usual syntactic compatibility that considers all constructors to be distinctmay_compatconsiders that constructors of identical arity from an extension type are identical (due to exception/constructor rebinding)ctx_matcherandconstr_matcherin matching.ml.Thanks to those fixes that apply to static data, compilation or or-patterns (
split_or) can be simplified a bit and some functions that looked for constructor from extension types in patterns can be deleted.