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

PR#7060: [toplevel] Print exceptions in installed custom printers #1035

Merged
merged 5 commits into from Feb 10, 2017

Conversation

Projects
None yet
3 participants
@tadeuzagallo
Contributor

tadeuzagallo commented Feb 10, 2017

This pull request improves the message printed when exceptions are raised from within installed custom printers (<printer %s raised an exception>) by adding the caught exception to it.

Link to the Mantis PR

Here's what the example from the ocamllabs issue looks like now:

# type t = A | B;;
type t = A | B
# let print_t out = function A -> Format.fprintf out "A";;
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
B
val print_t : Format.formatter -> t -> unit = <fun>
# #install_printer print_t;;
# B;;
- : t =
<printer print_t raised an exception: File "//toplevel//", line 1, characters 18-23: Pattern matching failed>
@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

This seems more engineered than it needs to be - with the effect at the moment that it's lost the name of the printer. Does Printexc.to_string not work in this context (so why can't _exn just be passed to exn_printer)?

Contributor

dra27 commented Feb 10, 2017

This seems more engineered than it needs to be - with the effect at the moment that it's lost the name of the printer. Does Printexc.to_string not work in this context (so why can't _exn just be passed to exn_printer)?

Show outdated Hide outdated Changes
@@ -162,6 +162,9 @@ Next version (4.05.0):
- PR#5163: ocamlobjinfo, dump globals defined by bytecode executables
(Stéphane Glondu)
- PR#7060: toplevel, print exceptions in installed custom printers

This comment has been minimized.

@dra27

dra27 Feb 10, 2017

Contributor

Please include GPR number as well;

PR#7060, GPR#1035: print name of the exception raised in custom printers.

It should also be in section ### Toplevel and debugger: (which will be a new section for this release)

@dra27

dra27 Feb 10, 2017

Contributor

Please include GPR number as well;

PR#7060, GPR#1035: print name of the exception raised in custom printers.

It should also be in section ### Toplevel and debugger: (which will be a new section for this release)

@dra27 dra27 changed the title from PR#7473: [toplevel] Print exceptions in installed custom printers to PR#7060: [toplevel] Print exceptions in installed custom printers Feb 10, 2017

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

Link to Mantis PR

Contributor

dra27 commented Feb 10, 2017

Link to Mantis PR

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Feb 10, 2017

Contributor

Thanks for the prompt review. Printexc.to_string indeed works, I feel a bit silly that I couldn't find it. I'll push an update in a couple minutes.

Contributor

tadeuzagallo commented Feb 10, 2017

Thanks for the prompt review. Printexc.to_string indeed works, I feel a bit silly that I couldn't find it. I'll push an update in a couple minutes.

tadeuzagallo added some commits Feb 10, 2017

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

Even with OCaml's relatively small standard library, it can be hard to see the wood for the trees sometimes!

However, now I can't make up my mind whether I preferred it saying Match_failure ("//toplevel//", 1, 18) before or the output with printers which Printexc.to_string gives! Quick opinion, @gasche?

Contributor

dra27 commented Feb 10, 2017

Even with OCaml's relatively small standard library, it can be hard to see the wood for the trees sometimes!

However, now I can't make up my mind whether I preferred it saying Match_failure ("//toplevel//", 1, 18) before or the output with printers which Printexc.to_string gives! Quick opinion, @gasche?

Show outdated Hide outdated toplevel/genprintval.ml
@@ -151,15 +151,15 @@ module Make(O : OBJ)(EVP : EVALPATH with type valu = O.t) = struct
(fun x -> Oval_int64 (O.obj x : int64)) ))
] : (Path.t * printer) list)
let exn_printer ppf path =
fprintf ppf "<printer %a raised an exception>" Printtyp.path path
let exn_printer ppf path _exn =

This comment has been minimized.

@gasche

gasche Feb 10, 2017

Member

Sorry for the nitpick, but I find the use of a variable of the form _foo confusing here (and in the rest of the file), given that those variable names are often used for ignored variables, and this variable is very much not ignored. Could you use exn instead?

@gasche

gasche Feb 10, 2017

Member

Sorry for the nitpick, but I find the use of a variable of the form _foo confusing here (and in the rest of the file), given that those variable names are often used for ignored variables, and this variable is very much not ignored. Could you use exn instead?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

@dra27 I am not sure exactly which output you have in mind, would you show an example? (Generally I would be tempted to not be super-picky: if we don't know which one we prefer, and nobody is going to be strongly negatively affected by the change, then maybe either is fine.)

I wonder what is the behavior when the value on which the exception is raised occurs in depth. Is the rest of the value printed correctly, or does the exception abort all printing? Could we have the testcase (a new one or modify the proposed one) to exhibit this behavior?

Member

gasche commented Feb 10, 2017

@dra27 I am not sure exactly which output you have in mind, would you show an example? (Generally I would be tempted to not be super-picky: if we don't know which one we prefer, and nobody is going to be strongly negatively affected by the change, then maybe either is fine.)

I wonder what is the behavior when the value on which the exception is raised occurs in depth. Is the rest of the value printed correctly, or does the exception abort all printing? Could we have the testcase (a new one or modify the proposed one) to exhibit this behavior?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@gasche The message before was the value itself (Match_failure ("//toplevel//", 1, 18)) but of course Printexc.to_string uses printers so it now becomes File "//toplevel//", line 1, characters 18-23: Pattern matching failed.

I think it's probably the case that using printers is better?

Contributor

dra27 commented Feb 10, 2017

@gasche The message before was the value itself (Match_failure ("//toplevel//", 1, 18)) but of course Printexc.to_string uses printers so it now becomes File "//toplevel//", line 1, characters 18-23: Pattern matching failed.

I think it's probably the case that using printers is better?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@gasche The behaviour is to propagate the < ... > string. e.g.

type t = A | B;;
let print_t f = function A -> Format.fprintf f "A";;
#install_printer print_t;;
type u = X of t | Y;;
X B;;
- : u = X <printer print_t raised an exception: ...>

Given that this only alters the string (rather than changing the propagation of the exception), is it really worth a test case too?

Contributor

dra27 commented Feb 10, 2017

@gasche The behaviour is to propagate the < ... > string. e.g.

type t = A | B;;
let print_t f = function A -> Format.fprintf f "A";;
#install_printer print_t;;
type u = X of t | Y;;
X B;;
- : u = X <printer print_t raised an exception: ...>

Given that this only alters the string (rather than changing the propagation of the exception), is it really worth a test case too?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

I would just use this as a testcase instead of the proposed one, because it is strictly more informative. (Test cases are also used to let people that review patches quickly get a sense of what the changes does, without testing it by themselves.)

Re. using the printers: I agree using the printers is better. It seems to be the simplest approach to implement anyway.

Member

gasche commented Feb 10, 2017

I would just use this as a testcase instead of the proposed one, because it is strictly more informative. (Test cases are also used to let people that review patches quickly get a sense of what the changes does, without testing it by themselves.)

Re. using the printers: I agree using the printers is better. It seems to be the simplest approach to implement anyway.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@gasche OK, while we were chatting Tadeu has pushed the test!

@tadeuzagallo This looks great, thanks - as soon as the CI returns I'll merge.

Contributor

dra27 commented Feb 10, 2017

@gasche OK, while we were chatting Tadeu has pushed the test!

@tadeuzagallo This looks great, thanks - as soon as the CI returns I'll merge.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 10, 2017

Member

Indeed, thanks for the nice patch -- and @dra27 for the review work. @dra27, as you merge, will you also close the Mantis PR? (I regularly forget to.)

Member

gasche commented Feb 10, 2017

Indeed, thanks for the nice patch -- and @dra27 for the review work. @dra27, as you merge, will you also close the Mantis PR? (I regularly forget to.)

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Feb 10, 2017

Contributor

The only issue I'm seeing is that there's a weird linebreak in the output now:

# C B;;
- : u =
C
 <printer print_t raised an exception: File "//toplevel//", line 1, characters 18-23: Pattern matching failed>

I couldn't figure out why though...

Contributor

tadeuzagallo commented Feb 10, 2017

The only issue I'm seeing is that there's a weird linebreak in the output now:

# C B;;
- : u =
C
 <printer print_t raised an exception: File "//toplevel//", line 1, characters 18-23: Pattern matching failed>

I couldn't figure out why though...

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 10, 2017

Contributor

@tadeuzagallo That's not an issue - that's the formatter doing its job and line-wrapping!

@gasche Yes, I will (and yes, it is easy to forget, I agree!)

Contributor

dra27 commented Feb 10, 2017

@tadeuzagallo That's not an issue - that's the formatter doing its job and line-wrapping!

@gasche Yes, I will (and yes, it is easy to forget, I agree!)

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Feb 10, 2017

Contributor

@dra27 That makes sense, thanks for the review and feedback!

Contributor

tadeuzagallo commented Feb 10, 2017

@dra27 That makes sense, thanks for the review and feedback!

@dra27 dra27 merged commit 46676cd into ocaml:trunk Feb 10, 2017

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.

Show comment
Hide comment
@gasche

gasche Feb 11, 2017

Member

The testsuite fails on some of our Continuous Integration machines, but I cannot reproduce the failure at home. For one machine, the content of pr7060.ml.result is as follows:


# type t = A | B
# type u = C of t
# Characters 18-54:
  let print_t out = function A -> Format.fprintf out "A";;
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
B
val print_t : Format.formatter -> t -> unit = <fun>
# Cannot find type Topdirs.printer_type_new.
# - : t = B
# - : u = C B
# 

I suppose that the issue comes from Cannot find type Topdirs.printer_type_new..

This result comes from a trunk-arm-32 machine, but trunk-linux-32, trunk-mingw-64, trunk-cygwin-64, trunk-macos-10.9, and a few others are also broken. (On the one I looked at, the .result file was the same as above.)

I suspect the issue is not caused by the patch, but the thing we are testing may not be reliable/portable enough. @dra27, do you have access to the CI infrastructure to test this? (Maybe we could skip the test on the affected configurations, or consider disabling it for now to clear the CI again.)

Member

gasche commented Feb 11, 2017

The testsuite fails on some of our Continuous Integration machines, but I cannot reproduce the failure at home. For one machine, the content of pr7060.ml.result is as follows:


# type t = A | B
# type u = C of t
# Characters 18-54:
  let print_t out = function A -> Format.fprintf out "A";;
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 8: this pattern-matching is not exhaustive.
Here is an example of a case that is not matched:
B
val print_t : Format.formatter -> t -> unit = <fun>
# Cannot find type Topdirs.printer_type_new.
# - : t = B
# - : u = C B
# 

I suppose that the issue comes from Cannot find type Topdirs.printer_type_new..

This result comes from a trunk-arm-32 machine, but trunk-linux-32, trunk-mingw-64, trunk-cygwin-64, trunk-macos-10.9, and a few others are also broken. (On the one I looked at, the .result file was the same as above.)

I suspect the issue is not caused by the patch, but the thing we are testing may not be reliable/portable enough. @dra27, do you have access to the CI infrastructure to test this? (Maybe we could skip the test on the affected configurations, or consider disabling it for now to clear the CI again.)

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 11, 2017

Contributor

I do have access - I'll try to have a look later.

Contributor

dra27 commented Feb 11, 2017

I do have access - I'll try to have a look later.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 12, 2017

Contributor

Hmm - I'm also seeing that with mingw64.

Contributor

dra27 commented Feb 12, 2017

Hmm - I'm also seeing that with mingw64.

@tadeuzagallo

This comment has been minimized.

Show comment
Hide comment
@tadeuzagallo

tadeuzagallo Feb 13, 2017

Contributor

Is there something I can do here to help?

Contributor

tadeuzagallo commented Feb 13, 2017

Is there something I can do here to help?

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 13, 2017

Contributor

The fix is straightforward - I'm just trying to work out why the CI didn't pick up the problem!

Contributor

dra27 commented Feb 13, 2017

The fix is straightforward - I'm just trying to work out why the CI didn't pick up the problem!

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

PR#7060: [toplevel] Print exceptions in installed custom printers (#1035
)

Exceptions raised by toplevel printers now include details of the exception itself.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment