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

Manual: update the exception section in the tutorial #1831

Merged
merged 7 commits into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@Octachron
Copy link
Contributor

commented Jun 11, 2018

This PR proposes to update the exception section in the tutorial to mention exception cases and local exceptions. The relevant sections in the language extensions chapter are also moved to the language reference chapter. Moreover, the third commit adds a small example of try ... with construction that catches two exceptions.

\subsubsection*{Exception patterns} \label{s:exception-match}
(Introduced in OCaml 4.02)

A new form of exception patterns, @ 'exception' pattern @ is allowed,

This comment has been minimized.

Copy link
@hcarty

hcarty Jun 11, 2018

Contributor

s/patterns/pattern/ maybe?

expression, but nothing prevents exception values created with this
constructor from escaping this scope. Two executions of the definition
above result in two incompatible exception constructors (as for any
exception definition).

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

I'm not clear on what this last sentence means.

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 11, 2018

Author Contributor

An example might help:

let f () = let exception A in A
;; assert ( f () <> f ())

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

A good example might assist in context, yes.

as opposed to regular ``value cases''. Exception cases are applied
when the evaluation of the matched expression raises an exception.
The exception value is then matched against all the exception cases
and re-raised if none of them accept the exception (as for a

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

"as with" might be better English than "as for".

The exception value is then matched against all the exception cases
and re-raised if none of them accept the exception (as for a
"try"..."with" block). Since the bodies of all exception and value
cases is outside the scope of the exception handler, they are all

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

The verb must agree with the subject, which is plural, so "Since the bodies of all exception and value cases are outside the scope of the exception handler" is better.

call.

It is an error if all cases are exception cases in a given pattern
matching.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

Might be good to have a newline at the end of the file?

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

Also, I don't really understand this sentence now that I am re-reading it.

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 11, 2018

Author Contributor

I have fixed the missing newline. In term of text, this sentence is trying to say that

let f x = match x with
 | exception E_1 -> ...
...
| exception E_n -> ...

is an error.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

How about:

It is an error if all cases in a given pattern match are exceptions. For example:

let f x = match x with
 | exception E_1 -> ...
 | exception E_2 -> ...
 | exception E_3 -> ...

is illegal.

A single sentence explaining the rationale for why it is illegal might also help, both by giving context and making it more obvious which case is being discussed. That might be better than having the example, which feels a bit redundant.

This comment has been minimized.

Copy link
@gasche

gasche Jun 11, 2018

Member

Or maybe we could turn this around and say that we expect at least one value pattern. (This wording would remain correct if we add effect p patterns for effect handlers.)

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 11, 2018

Author Contributor

Perhaps:

Beware that a pattern match must contain at least one value case.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

Or:

A pattern match must contain at least one value case. It is an error if all cases are exceptions, because then there would be no code to handle the return of an actual value.

(Is my understanding on that correct?)

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 12, 2018

Author Contributor

It is probably better to be explicit here, indeed.

\end{caml_example}
Note that this construction is only useful if the exception is raised
between "match"\ldots"with". Moreover, exceptions can only
appear at the toplevel of such a pattern matching.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

Perhaps "of such a pattern match." would be more idiomatic?

Note that this construction is only useful if the exception is raised
between "match"\ldots"with". Moreover, exceptions can only
appear at the toplevel of such a pattern matching.
For instance, exceptions cannot be combined with a or-pattern:

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 11, 2018

Member

Since "or" starts with a vowel, it must be "an or-pattern".

@pmetzger

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

I've left some comments on minor changes in wording (verb agreement, article agreement, etc.)

let assoc_opt x l =
match List.assoc x l with
| exception Not_found -> None
| x -> Some x;;

This comment has been minimized.

Copy link
@yallop

yallop Jun 12, 2018

Member

This example would be more enlightening if the value branch involved some evaluation:

  | x -> f x

As it is, the example could be more concisely written using try:

let assoc_opt x l =
  try Some (List.assoc x l) with
   Not_found -> None

However, if the value branch involves evaluation then it's harder to write the function using try, since moving the call to f between try and with changes the exception-handling behaviour.

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 12, 2018

Author Contributor

Good idea, adding f x in the non-exceptional branch is a nice touch.

between "match"\ldots"with". Moreover, exceptions can only
appear at the toplevel of such a pattern match.
For instance, exceptions cannot be combined with an or-pattern:
"exception A | exception B ->"\ldots .

This comment has been minimized.

Copy link
@yallop

yallop Jun 12, 2018

Member

Indeed, exception clauses can't be combined with an or-pattern. I think this is just a limitation of the current implementation, which could perhaps be indicated by writing "... cannot currently be combined ..."

However, the exception patterns themselves can be combined with an or-pattern: exception (A|B) ->

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 12, 2018

Author Contributor

The term clause is not used currently inside the tutorial, thus I think that exception *cases* would be clearer to the reader. But I agree that is better to avoid giving the impression that exception (A|B) is not valid.


A pattern match must contain at least one value case. It is an error if
all cases are exceptions, because there would be no code to handle
the return of a value.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Jun 12, 2018

Member

This is missing a trailing carriage return.

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 14, 2018

Author Contributor

Fixed.

Octachron added some commits Jun 13, 2018

@pmetzger

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

I don't know about other people's opinions, but this now looks quite good to me.

@gasche

gasche approved these changes Jun 15, 2018

Copy link
Member

left a comment

Approved on the basis of @pmetzger's review (I didn't review myself).

@Octachron Octachron merged commit 9e67f4e into ocaml:trunk Jun 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.