Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow exception-patterns under or-patterns #1568

Merged
merged 4 commits into from Jul 16, 2018

Conversation

Projects
None yet
5 participants
@trefis
Copy link
Contributor

commented Jan 15, 2018

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 #1567 , so the first 3 commits can be disregarded.

@trefis trefis force-pushed the trefis:or-exception-2018 branch from 349efb6 to a9f0cdf Jan 15, 2018

@@ -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

This comment has been minimized.

Copy link
@kit-ty-kate

kit-ty-kate Jan 15, 2018

Contributor

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

This comment has been minimized.

Copy link
@trefis

trefis Jan 15, 2018

Author Contributor

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

Thanks!

@trefis trefis force-pushed the trefis:or-exception-2018 branch from a9f0cdf to aa3dc64 Jan 15, 2018

hhugo added a commit to ocsigen/js_of_ocaml that referenced this pull request Jan 16, 2018

Problematic bytecode files.
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 trefis force-pushed the trefis:or-exception-2018 branch from aa3dc64 to 2195817 Jan 22, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

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 trefis force-pushed the trefis:or-exception-2018 branch from 41e078a to a1992b3 Feb 19, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2018

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 trefis requested a review from gasche Feb 19, 2018

@trefis trefis force-pushed the trefis:or-exception-2018 branch 2 times, most recently from 757ca8d to 2834938 Jun 13, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2018

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 #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

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

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
Copy link
Contributor

left a comment

Excuse the nit, which is #1148-related

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

This comment has been minimized.

Copy link
@dra27

dra27 Jun 30, 2018

Contributor

This line is over 80 chars!

This comment has been minimized.

Copy link
@trefis

trefis Jul 2, 2018

Author Contributor

Not anymore! :)

This comment has been minimized.

Copy link
@dra27

dra27 Jul 3, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

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

@dra27 dra27 dismissed their stale review Jul 3, 2018

check-typo changes dealt with

@lpw25

lpw25 approved these changes Jul 13, 2018

Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

Thanks for the review!

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

@trefis trefis force-pushed the trefis:or-exception-2018 branch from 7d5455c to 593e0e1 Jul 16, 2018

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@trefis trefis deleted the trefis:or-exception-2018 branch Jul 16, 2018

@bobzhang bobzhang referenced this pull request Nov 13, 2018

Open

to-do list for upgrade #3000

31 of 37 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.