Allow exception-patterns under or-patterns #1568

trefis · 2018-01-15T16:44:16Z

Revival of #305 .

A few things of note compared to the previous version:

Parmatch and Matching always assert false when they see a Tpat_exception
Parmatch.pressure_variants is called on the value and exception patterns independently.
This wasn't done in the previous version and was the cause of PR#7083.
Parmatch.check_ambiguous_bindings is called independently on the value and exception patterns. As for the other operations in parmatch, it wouldn't make sense to mix both kind of patterns in a single matrix.
In Rec_check I believe I handle cases containing only exception-patterns in exactly the same way as before. However, I wonder if perhaps this special casing could not simply be dropped (considering that Tpat_exception is not a destructuring pattern). I didn't try to understand how the check works though (nor do I plan to right now) so I'm going to leave things as is unless @yallop says it can be simplified.
in Translcore I make sure to remember (i.e. save in try_ids) all the names given to the exception of an exception pattern so that reraise works properly. I also added a test to make sure reraise works as intended when raising from a branch when we either match or catch an exception.

Apart from that everything should be as in #305, in particular : mixing value and exception patterns under a guard is still disallowed.

Off-topic:

I noticed that Parmatch.check_ambiguous_bindings is delayed when the matching contains polymorphic variants (and I didn't change it here) but I don't think that is necessary.
~~This PR is based on top of Register all idents relevant for reraise. #1567 , so the first 3 commits can be disregarded.~~

kit-ty-kate · 2018-01-15T16:56:58Z

bytecomp/matching.ml

@@ -2824,6 +2825,7 @@ let find_in_pat pred =
        find_rec p || find_rec q
    | Tpat_constant _ | Tpat_var _
    | Tpat_any | Tpat_variant (_,None,_) -> false
+  | Tpat_exception _ -> assert false


Is there an indentation issue or did it mean something else ?

Looks like indentation/copy-paste failure on my part; it's now fixed.

Thanks!

Generated with ocaml/ocaml#1568 $ cat test3.ml exception A of int let y = match Random.int 2 with | 0 as i | exception A (2 as i) -> i | i -> i+1 | exception A i -> i+2 $ cat test4.ml exception A of int let y = match Random.int 2 with | 0 as i | exception A (2 as i) -> i | i -> i+1 jbuilder build compiler/js_of_ocaml.bc && _build/default/compiler/js_of_ocaml.bc --debug main test3.cmo assert false jbuilder build compiler/js_of_ocaml.bc && _build/default/compiler/js_of_ocaml.bc --debug main test4.cmo stackoverflow

trefis · 2018-01-22T16:01:16Z

Rebased, as #1567 is now on top of #1573, the first 5 commits can be ignored.
The tests should now pass on both linux and windows!

trefis · 2018-02-19T11:55:22Z

I just rebased this PR on trunk now that the #1573 and #1567 have been merged. This should therefore be much easier to review.

Also, it should be noted that js_of_ocaml has been updated to handle exceptions under or-patterns!

trefis · 2018-06-13T11:28:22Z

I rebased this once more!

I also pinged @gasche by email to get some review, who (for obscure reasons) said it wasn't reasonable (sic) for him to review this.
So let me repeat some of my earlier points and ping some people explicitly:

testsuite/tests/match-exception-warnings/placement.ml tests all the syntactically valid patterns involving a Ppat_exception.
In particular one can see that exception patterns are now allowed under: or-patterns, local opens, type constraints. And that's the only change, every other appearance is still rejected.
Rec_check has been updated.
I suspect that the changes are correct but could be simplified. I would very much appreciate a review from @yallop on this.
Parmatch will error out (assert false or fatal_error) if it ever sees a Tpat_exception: there is no real need for review here (but the callers must be checked).
The compilation (most of the changes happening in Translcore) is as it was in Support "exception" under or-patterns (PR#6422) #305 .
I still believe it is fine, but a fresh review from @chambart or @maranget can't hurt.
The changes in Typecore are the ones that need review the most: they aren't that big, but that's where the issue was missed last time. The changes to type_pat are fairly trivial, but a complete review type_cases is required.
@garrigue , @lpw25: would one of you mind giving it a look?

yallop · 2018-06-22T13:55:37Z

In Rec_check I believe I handle cases containing only exception-patterns in exactly the same way as before. However, I wonder if perhaps this special casing could not simply be dropped (considering that Tpat_exception is not a destructuring pattern). I didn't try to understand how the check works though (nor do I plan to right now) so I'm going to leave things as is unless @yallop says it can be simplified.
[...]
I suspect that the changes are correct but could be simplified.

Yes -- it looks to me as though is_exception_pattern could be dropped altogether, leaving the code in case as it was previously.

dra27

Excuse the nit, which is #1148-related

dra27 · 2018-06-30T17:03:20Z

testsuite/tests/backtrace/backtrace_or_exception.ml

+   flags = "-g"
+   ocamlrunparam += ",b=1"
+   * bytecode
+     reference = "${test_source_directory}/backtrace_or_exception.byte.reference"


This line is over 80 chars!

Not anymore! :)

I thank you - now that the an off-by-one error in the Travis CI script is fixed, it'll even get scanned next time 🙂

trefis · 2018-07-02T14:49:52Z

@yallop: thanks! I pushed a commit doing the simplification.

check-typo changes dealt with

lpw25

I've reviewed the type-checking stuff and it looks fine. Since the other parts have already been reviewed, I think this is ready to merge.

trefis · 2018-07-13T16:10:53Z

Thanks for the review!

I'll rebase and merge this on Monday, unless someone objects.

trefis force-pushed the or-exception-2018 branch from 349efb6 to a9f0cdf Compare January 15, 2018 16:49

kit-ty-kate reviewed Jan 15, 2018

View reviewed changes

trefis force-pushed the or-exception-2018 branch from a9f0cdf to aa3dc64 Compare January 15, 2018 17:14

hhugo mentioned this pull request Jan 22, 2018

Compiler: support for OCaml GPR#1568 ocsigen/js_of_ocaml#642

Merged

trefis force-pushed the or-exception-2018 branch from aa3dc64 to 2195817 Compare January 22, 2018 16:01

trefis mentioned this pull request Feb 16, 2018

Register all idents relevant for reraise. #1567

Merged

trefis force-pushed the or-exception-2018 branch from 41e078a to a1992b3 Compare February 19, 2018 11:51

trefis requested a review from gasche February 19, 2018 11:55

trefis force-pushed the or-exception-2018 branch 2 times, most recently from 757ca8d to 2834938 Compare June 13, 2018 11:03

dra27 previously requested changes Jun 30, 2018

View reviewed changes

lpw25 approved these changes Jul 13, 2018

View reviewed changes

trefis added 4 commits July 16, 2018 10:41

match with exception: add some (expect) tests

4cb30bb

allow exceptions under or-patterns

369ea5f

testsuite: update

37cde6b

Changes

593e0e1

trefis force-pushed the or-exception-2018 branch from 7d5455c to 593e0e1 Compare July 16, 2018 09:48

trefis merged commit 83a65c0 into ocaml:trunk Jul 16, 2018

trefis deleted the or-exception-2018 branch July 16, 2018 10:40

bobzhang mentioned this pull request Nov 13, 2018

to-do list for upgrade rescript-lang/rescript-compiler#3000

Closed

37 tasks

Octachron mentioned this pull request Dec 7, 2018

manual: document exception A | exception B #2187

Merged

vicuna mentioned this pull request Mar 14, 2019

Support "exception" under or-patterns #6422

Closed

aantron mentioned this pull request Mar 25, 2020

Support exception patterns not at the top level of cases (4.08+) aantron/bisect_ppx#297

Closed

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

Add hint how to contribute a job listing on the jobs page (ocaml#1568)

3045026

lpw25 mentioned this pull request Mar 18, 2024

Add effect syntax #12309

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow exception-patterns under or-patterns #1568

Allow exception-patterns under or-patterns #1568

trefis commented Jan 15, 2018 •

edited

Loading

kit-ty-kate Jan 15, 2018

trefis Jan 15, 2018

trefis commented Jan 22, 2018

trefis commented Feb 19, 2018

trefis commented Jun 13, 2018 •

edited

Loading

yallop commented Jun 22, 2018

dra27 left a comment

dra27 Jun 30, 2018

trefis Jul 2, 2018

dra27 Jul 3, 2018

trefis commented Jul 2, 2018

lpw25 left a comment

trefis commented Jul 13, 2018

Allow exception-patterns under or-patterns #1568

Allow exception-patterns under or-patterns #1568

Conversation

trefis commented Jan 15, 2018 • edited Loading

kit-ty-kate Jan 15, 2018

Choose a reason for hiding this comment

trefis Jan 15, 2018

Choose a reason for hiding this comment

trefis commented Jan 22, 2018

trefis commented Feb 19, 2018

trefis commented Jun 13, 2018 • edited Loading

yallop commented Jun 22, 2018

dra27 left a comment

Choose a reason for hiding this comment

dra27 Jun 30, 2018

Choose a reason for hiding this comment

trefis Jul 2, 2018

Choose a reason for hiding this comment

dra27 Jul 3, 2018

Choose a reason for hiding this comment

trefis commented Jul 2, 2018

lpw25 left a comment

Choose a reason for hiding this comment

trefis commented Jul 13, 2018

trefis commented Jan 15, 2018 •

edited

Loading

trefis commented Jun 13, 2018 •

edited

Loading