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

Custom Exception Handlers at toplevel #8594

Merged
merged 16 commits into from Apr 14, 2019

Conversation

@AndrewLitteken
Copy link
Contributor

commented Apr 8, 2019

This is in response to issue #7156 where the OCaml Top-level level does not use any custom printers if they are registered.

There is a new function in Printexc.ml called to_string_opt that returns an option that is None where there are no printers registers, and Some string when there are printers that have been registered to resolve the Exception.

The addition involved adding to the print_out_exception function found in types/oprint.ml. For all Exceptions excluding Interrupts and Out of Memory errors, the result of the new function Printexc.to_string_opt is called against the exception. If None is returned, the previous method is used, if Some is returned then the string generated from the printers is used.

Closes #7156

Copy link
Member

left a comment

Thanks for your contribution! This goes in the right direction but it's not satisfying yet. I think that the original idea of the PR was to decompose the implementation of to_string into a more modular to_string_opt function and a "default printing behavior" which is called when none of the registered printers work. In particular, if the registered printers all return None on an exception, then to_string_opt should also return None. This is not what your current implementation does.

I think you should try to split the to_string implementation into to_string_opt and to_string_default (you could even expose this latter function to the user as well, as it may be useful).

I would also be interested in seeing tests for this feature/bug, as an expect-style testfile named testsuite/tests/tool-toplevel/uncaught_exceptions (for an example of how to write expect-style tests, see for example testsuite/tests/typing-misc/variant.ml. In particular, there should be a test for the case where there are some registered printers, but they all return None on the exception raised.

Changes Outdated
@@ -161,6 +161,9 @@ Working version

### Bug fixes:

- #7156: Create Mechanism so that OCaml Top Level uses custom printers

This comment has been minimized.

Copy link
@gasche

gasche Apr 8, 2019

Member

lowercase is more standard, and "exception" should be in the description: "make the toplevel use custom exception printers if they are available>

stdlib/printexc.ml Show resolved Hide resolved
@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Hah - @gasche has hit submit on his review fractionally before me! I would add only that I don't think the function should be to_string_opt - let's keep foo and foo_opt meaning that foo_opt returns None when foo would raise.

I wonder if, for example, what's wanted is something like fold_printers?

Changes Outdated Show resolved Hide resolved
@@ -69,6 +69,11 @@ let to_string x =
constructor ^ (fields x) in
conv !printers

let to_string_opt x =
match !printers with
[] -> None

This comment has been minimized.

Copy link
@dra27

dra27 Apr 8, 2019

Contributor

If this stands, please indent as in other match cases (so reduce indentation) and include a | on the first case.

val to_string_opt: exn -> string option
(** [Printexc.to_string_opt e] returns a string option containing None if
there are no registered printers and Some s for a representation if there
are registered printers. *)

This comment has been minimized.

Copy link
@dra27

dra27 Apr 8, 2019

Contributor

Again, if it stands only: this could be more terse, in keeping with other descriptions. The type tells us that the function returns string option so it doesn't need stating in the description. Something more like [Printexc.to_string_opt e] returns [None] if there are no registered printers and [Some (to_string e)] otherwise.

This comment has been minimized.

Copy link
@trefis

trefis Apr 8, 2019

Contributor

Nitpick, but I'd make the name more explicit, something like: to_pretty_string_opt.
(feel free to ignore this comment)

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Yes, I agree to_string_opt is not the best name; fold_printers is nice, or even just use_printers may be fine. Also, in the .mli file this function should go right after register_printer, I think, as it assumes the knowledge of the details of the registration mechanism.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

(The CI failure for 32-bit was of course #8585, which I've just merged)

@Armael

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

(This is great! I wanted to fix that bug a long time ago, but couldn't find how at that time.)

updating to trunk of ocaml
This changes Printexc.to_string into a match on Printexc.use_printers which
returns None if no printer exists or matches. Then a default using the
previous to_string is run.  Both of these new functions are surfaced to
the user.

The top level attempts to use use_printers, and if None is returned, uses
the previous exception printing.
@AndrewLitteken

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

I have made the restructuring changes that were suggested and renamed to_string_opt to use_printers.

I'm having some trouble creating expect tests for these since whenever an exception is raised computation ends. Do you have any suggestions to work around this?

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

The following source file works on my machine as testsuite/tests/tool-toplevel/uncaught_exceptions.ml:

(* TEST
   * expect
*)

raise Exit;;
[%%expect {|
Exception: Stdlib.Exit.
|}];;

exception Foo of string;;
[%%expect {|
exception Foo of string
|}];;

raise (Foo "bar");;
[%%expect {|
Exception: Foo "bar".
|}];;

The workflow I use to create this file is to start with empty expect blocks:

[%%expect {|
|}];;

after each toplevel phrase, and then use (from the directory tool-toplevel/) the command ../../../ocamltest/ocamltest.opt -promote uncaught_exceptions.ml to populate it with expected outputs.

typing/oprint.ml Outdated Show resolved Hide resolved
Changes Outdated
@@ -167,6 +167,8 @@ Working version

### Bug fixes:

- #7156: make top level use custom printers if they are available

This comment has been minimized.

Copy link
@nojb

nojb Apr 9, 2019

Contributor

Please also add the PR number.

stdlib/printexc.mli Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

The API changes look fine, thanks. Some stylistic suggestions, and some documentation changes.

Changes Outdated Show resolved Hide resolved
let rec conv = function
| hd :: tl ->
(match try hd x with _ -> None with
| Some s -> s
| Some s -> Some s
| None -> conv tl)

This comment has been minimized.

Copy link
@dra27

dra27 Apr 9, 2019

Contributor

Stylistically, this could more neatly be:

let result = (try hd x with _ -> None) in
if result <> None then result else conv tl

or

  (try
     hd x
   with _ -> conv tl)

This comment has been minimized.

Copy link
@gasche

gasche Apr 14, 2019

Member

(I'm not asking for a change to the PR because I think the current version is okay but) personally I found the earlier version more readable: it's easier for me to read a pattern-matching on an option type and detect the success and failure case than to understand what if result <> None is doing.

A third option would be to use the match .. with exception feature to avoid the intermediate try .. with.

match hd x with
| None | exception _ -> conv tl
| Some s -> Some s

if one is disturbed by the lack of sharing in the last case (I'm not), it's also possible to write it as

| (Some _) as result -> result

This comment has been minimized.

Copy link
@AndrewLitteken

AndrewLitteken Apr 14, 2019

Author Contributor

Since I had to fix documentation, I went ahead and went with the third option, I agree it is more readable

@@ -95,6 +99,10 @@ val register_printer: (exn -> string option) -> unit
@since 3.11.2
*)

val use_printers: exn -> string option
(** [Printexc.use_printers e] returns [None] if there are no registered
printers and [Some (to_string e)] otherwise.*)

This comment has been minimized.

Copy link
@dra27

dra27 Apr 9, 2019

Contributor

This needs @since 4.09

stdlib/printexc.mli Outdated Show resolved Hide resolved
conv !printers

let to_string_default x =
match x with

This comment has been minimized.

Copy link
@dra27

dra27 Apr 9, 2019

Contributor

A refactoring nit: but this could now be

let to_string_default = function

...

sprintf locfmt file line char (char+6) "Assertion failed"
| Undefined_recursive_module(file, line, char) ->
sprintf locfmt file line char (char+6) "Undefined recursive module"
| _ ->

This comment has been minimized.

Copy link
@dra27

dra27 Apr 9, 2019

Contributor

... with x here instead

merging trunk
@AndrewLitteken

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I believe all the style and formatting changes are present now. Are there anymore tests that should be added?

@gasche
gasche approved these changes Apr 10, 2019
Copy link
Member

left a comment

Thanks! This looks good to me -- see a minor change suggestion inline. I won't merge right ahead to let my esteemed reviewer colleagues comment if they wish to. (Feel free to ping me if nothing has happened by the end of the week.)

There remains a minor glitch, which is that Printexc.to_string will in some case print a dedicated message coming from to_string_default by catching a few built-in exceptions (for example Assert_failure), which neither the compilers nor the toplevel seem to handle correctly in my test (the toplevel has some builtin-in logic of its own, which shows slightly different messages for a subset of those built-in-exceptions). It would have been nice to unify this a bit more, but I'm not sure what the right choice is there, it is not directly related to the problem reported in the PR, I don't want to delay the nice work and ultimately I think it's not a very important problem.

testsuite/tests/tool-toplevel/uncaught_exceptions.ml Outdated Show resolved Hide resolved
@AndrewLitteken

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@gasche I think I updated the test as you suggested to be more readable. Just pinging you to merge the pull request.

Copy link
Contributor

left a comment

There’s a doc comment which has got out of sync with all the changes, but otherwise LGTM

@@ -95,6 +101,12 @@ val register_printer: (exn -> string option) -> unit
@since 3.11.2
*)

val use_printers: exn -> string option
(** [Printexc.use_printers e] returns [None] if there are no registered
printers and [Some (to_string e)] otherwise.

This comment has been minimized.

Copy link
@dra27

dra27 Apr 14, 2019

Contributor

This comment needs completely updating

This comment has been minimized.

Copy link
@AndrewLitteken

AndrewLitteken Apr 14, 2019

Author Contributor

I think this was referring to the [Some (to_string e)] it has been updated for says [Some s] where s is the resulting string

@gasche gasche merged commit d4ef2ee into ocaml:trunk Apr 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

I went ahead and merged. Thanks @AndrewLitteken and all reviewers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.