pattern-matching refactoring: clarify usage of the Cannot_flatten exception#9608
pattern-matching refactoring: clarify usage of the Cannot_flatten exception#9608gasche merged 6 commits intoocaml:trunkfrom
Conversation
8a6cee4 to
959795a
Compare
Octachron
left a comment
There was a problem hiding this comment.
First some nitpickings:
The two commits (9447ef5 and 16669cc) defining flatten_pattern_simple could maybe be merged together.
(Also for ulterior reference, the commit order presented by Github is the wrong one, the one in the history is more logical.)
We could add some documentation to Matching.for_multiple_match to document its precondition on its cases arguments.
Another point for later PRs is that it might be interesting to make flatten_pattern returns an option in order to make Cannot_flatten an internal exception of the Matching module.
Otherwise, the PR does make more obvious where Cannot_flatten is raised, by exploiting a precondition for Matching.for_multiple_match.
The test failure is a mere (+) 1 identifier drift that seems unrelated.
|
I'm not convinced by the idea of merging the two commits. The first one adds a new function that is completely equivalent to the non-simple one (and it is very easy to review), the second turns an exception into a fatal error, and requires a careful call-graph analysis to be reviewed. I think that merging the two may blur things a bit. I will fix the testsuite failure. (The identifier drift is not a change in an existing test, but in a test that was added in this commit, I think this corresponds to minor code-production difference between the time were the test was written and the current trunk.) |
a1ad7e8 to
8822951
Compare
Note: this is due to mk_alpha_env raising Cannot_flatten during splitting/precompilation.
Unfortunately since the function is exposed and used in translcore we need to keep the generic one, and introduce a flatten_simple_pattern.
This makes it clearer where the exception comes from.
8822951 to
f8bf8c3
Compare
This is the next bit of the big pattern-matching refactoring change (#9321) after #9599 has been merged. This patchset clarifies the usage of the
Cannot_flattenexception indo_for_multiple_match.(cc @trefis @Octachron)
For now this is only a "dynamic check" clarification (we are thinking hard about the control flow and turning an exception into a fatal error in some cases where it would be a programming error if it happened), not a static enforcement of the corresponding discipline. I thought a bit about how to do the latter, but it is not easy¹. Suggestions are welcome, but I think that any change of this nature should go in a separate PR.
¹: We would like to "learn" some static fact from whether an exception is raised or not, but preferably without rewriting the whole call stack to use explicit results instead. Another option would be to pass a failure continuation instead of raising an exception, but it is unclear that this would be better. Yet another option would be to remove the
optioncharacter ofarg_idwith two different entry points depending on whether we know we need to flatten or not; with some clever factorization to avoid having to duplicate the implementation.