Make pattern matching compilation more robust to ill-typed columns#1538
Make pattern matching compilation more robust to ill-typed columns#1538trefis merged 4 commits intoocaml:trunkfrom
Conversation
(reviewed by Thomas Refis)
|
PS: more tests could be added to this GPR, indeed there is a quite extensive list in GPR#1518, but I was too lazy to select the subset of tests from that list which exercised "bugs" in matching.ml while also avoiding assertion failures in parmatch.ml. I vote for adding more tests only after parmatch.ml is fixed. |
9bc223b to
7b0532c
Compare
gasche
left a comment
There was a problem hiding this comment.
I reviewed it again and I believe the PR is correct -- and corresponds to what we discussed. Good to hear that jbuilder is fixed.
| | Tpat_any | ||
| | Tpat_var _ -> | ||
| record_matching_line num_fields [] @ rem | ||
| | Tpat_record ([], _) when num_fields = 0 -> rem |
There was a problem hiding this comment.
Interesting, how did that line about the empty record end up here?
There was a problem hiding this comment.
It seemed better to be robust rather than assert false (especially considering other GPRs/discussions currently happening).
As we've discovered, assert falses can go unnoticed for a while even when the underlying invariant cannot be safely assumed anymore.
There was a problem hiding this comment.
On the other hand, the module code generally assumes that record are non-empty everywhere (I thought about this when writing my part of the patch, notice the ubiquitous (_, lbl, _) :: _ patterns). I think it would have been better, for consistency, to have all the code share this same assumption, rather than have some parts be clever and some not. But I also think that the new line you added is correct, and will not require further work by people willing to add empty records, so it's fine to leave it in.
|
@gasche looking at the 4.06 branch, it seems that #1459, while listed under the "4.06 maintenance branch" section of Changes, was never cherry-picked. Should I just cherry-pick and then cherry-pick the current GPR? PS: for cherry-picking, I planned to squash all the commits of the PR together, for simplicity, does that sound OK to you? |
Make pattern matching compilation more robust to ill-typed columns (cherry picked from commit 0f342e5)
|
I pushed the two cherry-picked (and conflict-solved) commits in #1539, but I'd like to make a pause before merging in 4.06, give others the opportunity to object. |
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
The first commit is extracted from #1518 and the test is the one produced in #1493 .
This GPR, unlike #1518, focuses only on the compilation of pattern matchings and fixes the regression introduced by GPR#1459 (which broke the build of jbuilder).
There is a consensus (among @maranget , @gasche and I) that we should merge it and then cherry-pick it on the 4.06 branch (which I'll do once someone formally approves).
The alternative being to revert #1459 from the 4.06 branch, but that means reintroducing a known compilation bug.