matching: use polymorphic variants to classify general/half_simple/simple patterns#9361
matching: use polymorphic variants to classify general/half_simple/simple patterns#9361trefis merged 1 commit intoocaml:trunkfrom
Conversation
|
I discussed this offline with @Octachron last week, and he does not seem particularly excited by the use of polymorphic variants. Having just read over both versions of
That said, I will let @Octachron present his grievances, and defend the merits of his approach. |
|
My main grievance is that the polymorphic variant types are not used that much in this PR in fine. My view of this PR is that it refines the pattern type family in order to both expose the subtyping relationship between the family members and precise their "decomposition" (for lack of better or). The first part can be done without polymorphic variants (as seen in my toy implementation), so the real advantages of polymorphic variants comes from the second part. However, most of the PR improvement comes from exposing the first part: the subtyping relationship makes it possible to have a generic The precise decomposition of polymorphic variant is used exactly thrice in the PR. Two of those are an effect of making I ended up looking to the overview pattern matching PR to see if it is only a short growing pain. However, even there, it is not that clear: the pattern of refine/erase is still present. In other words, I am paradoxically stating that the lack of ambition in this PR makes it harder to review. And I am not fully convinced that this back-and-forth is only a growing pain, even if polymorphic variants does seem to be a natural fit. Overall, I think that the balance still tip in favor of this PR, but by a far thinner margin than I expected when I started the review. |
|
I don't like @Octachron's proposal for two reasons:
Compare for example: val explode : Half_simple.pattern -> Ident.t list -> Simple.clause list -> Simple.clause list
(* polymorphic variant version -- slightly reworded from the PR *)
let rec explode p aliases rem =
let explode_p p aliases rem = explode (General.view p) aliases rem in
match p.pat_desc with
| `Or (p1, p2, _) -> explode_p p1 aliases (explode_p p2 aliases rem)
| `Alias (p, id, _) -> explode_p p (id :: aliases) rem
| `Var (id, str) ->
let env = mk_alpha_env arg (x :: aliases) vars in
((omega, patl), mk_action ~vars:(List.map snd env)) :: rem
| #simple_view as view ->
let p = { p with pat_desc = view } in
let env = mk_alpha_env arg aliases vars in
((alpha env p, patl), mk_action ~vars:(List.map snd env)) :: rem
(* phantom type versions *)
let rec explode p aliases rem =
match p.pat_desc with
| Tpat_or (p1, p2, _) -> explode p1 aliases (explode p2 aliases rem)
| Tpat_alias (p, id, _) -> explode p (id :: aliases) rem
| Tpat_var (x, _) ->
let env = mk_alpha_env arg (x :: aliases) vars in
((omega, patl), mk_action ~vars:(List.map snd env)) :: rem
| _ ->
let env = mk_alpha_env arg aliases vars in
((alpha_pat env p, patl), mk_action ~vars:(List.map snd env)) :: remIn the polymorphic version, we know statically that the There are more such code traversals in matching.ml and parmatch.ml, that can benefit from the static safety of the polymorphic-variant version. Most of these functions should not be pushed within the abstraction boundary. One could define a generic "non-simple decomposition" higher-order function that would sit inside the abstraction, to be used by those traversals outside, but then again this would result in more complex code.
Sure, there is a frontier between the static-discipline-aware code (that knows which patterns are (half)simple in its inputs and outputs) and the unaware code. Whenever we call an unaware helper function (for example |
Octachron
left a comment
There was a problem hiding this comment.
It seems indeed more natural to let the type-level computations to the type checker rather than to encode them as admitted axioms in the module interface.
My remaining remarks are completely minor.
Like I said, the subtyping and the removal of try_no_or are certainly nice, thus the PR seems good to go for me.
49dfa67 to
33546df
Compare
…mple patterns Also: these types are now instances of Typedtree.pattern_data.
33546df to
bc0206b
Compare
|
@trefis I rebased this PR on top of trunk (most of the work was coming from changes to previous PRs in the series). It's now up to you to decide when to merge (assuming CI is green), and to shoot the next round. |
|
Thanks! I'll probably send the next PR early next week. |
Another one-commit-PR extracted from #9321 .
Note: we have tried an alternative to this, which was to make the "intermediary types" (i.e. the ones representing half-simplified and simplified patterns) abstract.
But it didn't turn out as nice as this.