Skip to content

Protect exception by injecting them in result type #11

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

Merged
merged 5 commits into from
Mar 16, 2022

Conversation

n-osborne
Copy link
Contributor

@n-osborne n-osborne commented Mar 10, 2022

A function returning a value of type t and that can raise an exception will return, in its specification a (t, exn) result

Util module provides a protect combinator and a protected type constructor to avoid some boilerplate.
Some refactoring has been done (repeat and print_triple_vertical are now in module Util)

@jmid
Copy link
Collaborator

jmid commented Mar 10, 2022

Thanks a bunch for this!
It is nice to see how the "client code" is simplified by using the built-in result type!

So is the idea that

  • Util contains reusable definitions helpful to outsiders and
  • Common contains reusable definition used internally?
    Note: CI is failing because it cannot find Common. Did you forget to add the file?

Looking at #9 from @OlivierNicole yesterday I noticed that he also uses the result type. He goes a step further though making res a result type.
For testing a pure interface (without exceptions thrown) that may perhaps be overkill to do in general.

I'm wondering why you went with an "internal" 'a protected type?
I'm guessing it may be because you wanted the right show-function for exceptions?

A different aspect is that a client would have to understand a new type 'a protected to use this.
To keep the barrier to entry as low as possible, I wonder whether we could achieve the same thing without having to introduce a new type, e.g., by:

  let show_exn = ...
  let show_result = ...
  let protect (f : 'a -> 'b) (a : 'a) : ('a, 'exn) result = ...

Simultaneously in #5 @OlivierNicole is trying to let users install lin or STM independently - and with minimal opam-file dependencies. If we can get away with "hand-written" show functions, we could eliminate a ppx_deriving dependency for those.
I'm unsure how Common will work in that situation since it will be common to both packages.
Actually I don't know how sharing Util works out in #9. @OlivierNicole?

@OlivierNicole
Copy link
Contributor

Simultaneously in #5 @OlivierNicole is trying to let users install lin or STM independently - and with minimal opam-file dependencies. If we can get away with "hand-written" show functions, we could eliminate a ppx_deriving dependency for those.
I'm unsure how Common will work in that situation since it will be common to both packages.
Actually I don't know how sharing Util works out in #9. @OlivierNicole?

As far as I know, any executables and libraries can be attributed to a package by the dune files (via public_name or package directives) and then dune sorts out the dependencies. Common modules between packages doesn't seem to be a problem—although I don't know how it is handled, installation-wise.

@n-osborne
Copy link
Contributor Author

Thanks for the review.

  • Util contains reusable definitions helpful to outsiders and

  • Common contains reusable definition used internally?

It's something like that. Some code was copy-pasted, so I put it in a Common module.
But I don't have any strong opinion about this organization.

  • Note: CI is failing because it cannot find Common. Did you forget to add the file?

Indeed, I forget to add the common.ml file... It's done now.

I'm wondering why you went with an "internal" 'a protected type?
I'm guessing it may be because you wanted the right show-function for exceptions?

I used a 'a protected type so that if we change its definition (I was unsure whether to use ('a, exn) result or ('a, string) result for example) there is minimal change to do in "clients".
Also it is less to type for the user and the show is already derived.
Note: I'm not sure about the name protected.

A different aspect is that a client would have to understand a new type 'a protected to use this.

But I'm happy to use an explicit result type if you feel it would be easier for the user.

@jmid
Copy link
Collaborator

jmid commented Mar 10, 2022

I used a 'a protected type so that if we change its definition (I was unsure whether to use ('a, exn) result or ('a, string) result for example) there is minimal change to do in "clients".

OK, makes sense.
I suspect that ('a,'b) result won't require many changes though.
It has worked for @OlivierNicole in #13 and I can also see Monolith uses result for a similar situation.

Also it is less to type for the user and the show is already derived.

That's where I was hoping we could make a difference:
If we just include suitable definitions for pp_exn and show_exn in Util, Common, or Lin, say:

  let pp_exn = fun fmt e -> Format.fprintf fmt "%s" (Printexc.to_string e)
  let show_exn e = Format.asprintf "%a" e pp_exn

a client using both protect and ppx_deriving.show will just pick them up from the environment.
Putting them in Common which is opened by both Lin and STM should then make them
visible to any user doing an open on one of those, right?

But I'm happy to use an explicit result type if you feel it would be easier for the user.

I think it would, following the principle of least surprise: OCaml already has a standard result type that users are likely to know.

@jmid
Copy link
Collaborator

jmid commented Mar 16, 2022

This has shaped up great - thanks! (and sorry for the packaging delay!)

@jmid jmid merged commit a754111 into ocaml-multicore:main Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants