The Pattern-Matching Bug: propagate mutability of argument positions#13138
The Pattern-Matching Bug: propagate mutability of argument positions#13138gasche merged 1 commit intoocaml:trunkfrom
Conversation
3d08d30 to
670d2a3
Compare
670d2a3 to
daaf46b
Compare
| { | ||
| arg; | ||
| binding_kind = StrictOpt; | ||
| mut = compose_mut mut Mutable; |
There was a problem hiding this comment.
I am not sure how I feel having constant multiplications by 0 or 1 in the source code, but why not.
There was a problem hiding this comment.
With this code organization I find it much easier to check how mutability information evolves during compilation. (Also: if I wanted to track both the "transitive mutability" and the "immediate mutability" of the argument position, this would be easy to do by changing mut here to be a pair, without changing much of the rest of the code. This would let us get rid of mut_of_binding_kind. I decided not to do it because there is bigger fish to fry, but it suggests that compose_mut is a nice abstraction to have.)
ncik-roberts
left a comment
There was a problem hiding this comment.
The code is correct, and it makes sense to me why this will be needed for your ensuing PRs.
c56c967 to
50f6c2c
Compare
|
Thanks! I took the review comments into account. |
|
The diff looks to have picked up some unrelated changes in |
50f6c2c to
32ba25f
Compare
|
Note: this needs a maintainer approval if we want to merge and rebase the follow-up PRs. |
lthls
left a comment
There was a problem hiding this comment.
Approving on behalf of @ncik-roberts (after a quick look through the code)
This is the next PR in the stack to fix #7241. To fix the bug, we need to pessimize the totality information coming from the type-checker when we switch on arguments/positions that may have been mutated. As suggested by the testcase match-side-effects/partiality.ml:deep, positions affected are not just those immediately below a mutable field, but also those that are transitively mutable -- there is a mutable field somewhere between the root of the value and the current position.
The present PR does not change the compilation behavior, it just adds a
mutfield to argument records to track whether the argument is in transitively-mutable position. (Currently this information is not tracked at all, unlike the information "immediately mutable" that can be reconstructed from the binding kind of the argument expression.)(cc @ncik-roberts, @Octachron, @lthls )