search

ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.19k stars 1.06k forks source link

The Pattern-Matching Bug: testsuite improvements #13121

Closed gasche closed 2 weeks ago

gasche commented 3 weeks ago

I am still working on #7241; the next installment of bugfixing PRs is taking a bit more effort than I expected, and here is a PR that only improves the testsuite for this bug in order to make reviewer's life easier in the short future. (cc @Octachron, @trefis, @lthls, @ncik-roberts)

The first commit refines an existing test in a "simple" and a "complex" version. Currently there is only the "complex" version in the testsuite, but both versions behave differently in some cases, and while trying to reason about the compiler behavior I found the behavior surprising. (If someone remembers the full details, we considered two approaches to fix the "context" part of the #7241 bug, and eventually I decided on one; the testsuite was written when I was still working on the first/earlier approach, and so it lacks distinctions that did not matter with that different implementation.)

The second commit adds a more complex test for the "totality bug" part of the fix of #7241, fix which has not been merged yet. This is about testing what happens on argument positions that are immutable themselves (they result from projecting an immutable field) but they are "transitively mutable", they are transitively under a mutable position. This is related to a discussion that we had with @ncik-roberts in https://github.com/ocaml/ocaml/pull/12555#discussion_r1552374951 . Adding a new testcase for this behavior proved very useful because it let me realize that my current "fix for the totality issue" was incomplete in this situation. (I went back and forth on how to handle transitivity, and my working branches include a simplistic approach that I thought was complete and in fact is not.)

The third commit adds coverage for cases that are currently handled by the check_partial logic within runtime/matching.ml, which is a pre-existing, incomplete criterion to downgrade certain pattern-matching problems from Total to Partial. (This heuristic fires when one clause has both mutable fields and either a lazy pattern or a guard.) The final fix for the Pattern-Matching Bug subsumes this incomplete criterion (in a way that also lets us un-pessimize certain programs I think), so that logic will be removed in a further PR. The new testcases ensure that we would detect a regression coming from this, by testing the various settings where it applies.

ncik-roberts commented 2 weeks ago

(I expect you mean #7241 and not #7421.) I'm taking a look at this.

gasche commented 2 weeks ago

Note: this PR and also #13129 would need a maintainer approval so that I can merge them and send the next, more interesting PR in the stack. It would be nice to do this sooner rather than later to benefit from the potential availability of @ncik-roberts.

gasche commented 2 weeks ago

(I have updated the PR after handling the review comments.)

Octachron commented 2 weeks ago

I will approve once @ncik-roberts approves the review changes.