I would leave the uses in the compiler unchanged, I would rather avoid a bootstrap for such a small addition.

I had understood that the fact that the signature of Pervasives changed was enough to require the bootstrap. If that is not the case, I am happy to leave the copy in Misc even if it is a bit redundant.

No, it should not be necessary to bootstrap if you do not use the added functionality in the compiler proper.

I would like to argue for not leaving the copy in Misc. Having redundant copies of functions in the tree isn't going to further maintainability of the code, a goal I think we should all be striving for.

I have a backup of the branch, so it's easily done. I probably prefer to have just the implementation in Pervasives (that already has to be copied to the threads version), but I am fine to keep changes to a minimum (it could be done in two stages, cleaning up Misc once something bigger requires a bootstrap anyway)

Why you are at it, when you remove Misc.foo into Pervasives.foo, I think you could just write foo as Pervasives is opened by default.

But I haven't seen the Real Bikeshedding start yet: are we sure that we don't want to label the argument(s)?

@gasche I had written Pervasives.try_finalize because I had noticed Pervasives explicitly mentioned in a few parts of the code and thought that that was some kind of preferred style.

I did not name the arguments because I was not really happy with their names and because in Pervasives there is no other use of named arguments. I'd rather not label them but I'll leave that open to votes, like the Misc cleanup.

But I haven't seen the Real Bikeshedding start yet: are we sure that we don't want to label the argument(s)?

Not only that... we might prefer a version where work/cleanup accept
a non-unit parameter...

@xclerc I'd like that discussion though. Is it more ergonomic/used as try_finalize f x finally y or as try_finally f finally? Almost all the use I've seen have been of the second type.

I like the convention of naming the cleanup function so you could reorder, and cleanup is a nice, clear name.

Could we also discuss the opportunity of storing the backtrace raised by work, and using Printexc.reraise instead of raise? This version of try_finally makes debugging really painful.

Why do you need to bootstrap if you change Misc?

@mseri I guess might depend on your computations, but if you
consider the more-or-less canonical example of reading data
from a file, try_finally will lead to something akin to:

let first_line filename =
  let chan = open_in filename in
  try_finally
    (fun () -> input_line chan)
    (fun () -> ignore (close_in chan))

while try_finalize would avoid the closures:

let first_line filename  =
  let chan = open_in filename in
  try_finalize input_line chan close_in chan

Also, I cannot remember ever wanting the cleanup function
to take anything else than the input of the work function (possibly
just ignoring it). It has the arguably nice property that you do not
have to let-bind the value:

let first_line filename  =
  try_finalize input_line chan close_in

(To be clear, it is not the terseness that I find appealing, but the
fact that you can avoid namespace pollution.)

To follow @yakobowski you could look at #374 for a discussion on which implementation for Misc.try_finally that use reraise. You could also adopt the PR if you want.

@yakobowski I completely agree on the use of reraise.
@xclerc indeed the third option would probably be the most common.
Thanks @bobot, I was completely unaware of that PR, I am going to read the discussion there. I wonder if that implementation is too general for the common use, on the other hand the arguments are optional so it should not make too much difference.

If we settle on a form of try_finally for Pervasives, I am happy to update that PR next. I suggest to postpone the cleanup of Misc to that stage then

I have read the discussion on #374. I am going to use that implementation as it is allowing for greater flexibility without too much additional burden, but I think ~always should be non optional. As far as I can see: calling it with nothing makes not sense in general, calling it with just ~exceptional but no ~always is equivalent to calling it with ~always. Am I missing something?

I am still undecided in the balance between simplicity and flexibility. I think if we use this other version then making the three functions get different parameters gets too verbose. In that case I'd stick with unit.

Seems like this would help for #640 … 💯

As far as I can see: calling it with just ~exceptional:f but no ~always is equivalent to calling it with ~always:f.

It is not the case, ~exceptional is run if and only if work or always raises. So if always is not given exceptional is run only if work raises.

Oh, yes, you are right! I'll update the PR

I had to locally re-define raw_backtrace, get_raw_backtrace, raise_with_backtrace because I could not refer to Printexc. It seemed better than trying to change the build to make it work.

I have updated the implementation by dropping the ~exceptionally argument, as discussed here. I think that this version behaves much more closely to what most users would expect, and adding the exceptional handling is possibly even easier to understand by dealing with it explicitly (as @dbuenzli was suggesting):

try try_finally work ~always with 
| e -> exceptionally (); raise e

or

try try_finally work ~always with 
| e ->
  let bt = Printexc.get_raw_backtrace in
  exceptionally ();
  Printexc.raise_with_backtrace e bt

I have kept a link to Printexc.raise_with_backtrace in the docstrings but I made it a bit less prominent. Let me know if I should remove it completely but I think it is a reasonable middle ground.

Should I keep an example in the docstring, maybe something like the one in @xclerc comment?

let first_line filename =
  let chan = open_in filename in
  try_finally
    (fun () -> input_line chan)
    (fun () -> close_in chan)

It may be worth adding an example on the line of the second snippet above (without try_finally) to the Printexc.raise_with_backtrace documentation, to show an example of use (and why it can be useful there).

Concerning @xclerc comments about the signature, I am not convinced that going to the full generality of try_finalize with something like

val try_finally : work:('a -> 'b) -> 'a -> always:('c -> unit) -> 'c -> 'b

is worth. It reduces the number of closures needed to possibly none but complicates unnecessarily the function call. Regarding the other suggestion of passing the same input

val try_finally : always:('a -> unit) -> ('a -> 'b) -> 'a -> 'b

I think it would cover nicely a common use case, but I think the assumption on always is unnecessarily restrictive. I have seen many examples in which you need to pass different values to the cleanup function, that would practically require to add closure that ignores the input and deals with the rest.

In this respect I find the current version

val try_finally : always:(unit -> unit) -> (unit -> 'a) -> 'a

explicit enough and easy to understand, and I think that the fact that it usually requires to add two closures is a minor pain and does not affect the readability of the code.

Regarding the signature discussion I have no strong feelings about it. But here are a few cents.

I initially did like the style of the ocaml unix book and in general when wrapping functions I'd go application style with wrap : ('a -> 'b) -> 'a -> 'b which I use in a number of my libraries.

However this is only nice when your function has a single argument. When it has multiple ones then you have to e.g. wrap (f x y) z which is not that great. Even worse it can lead to sublte error: wrap (f x) y z is entirely valid and does not do what you want (wraps only the partial application). My impression is that having the less natural (unit -> 'a) signature you are less likely to perform that kind of error and thus would go with @mseri's proposal.

Thanks for the comments. I have updated the docstring with @dbuenzli suggestion and the Changes file.

Feel free to ignore this bike shedding comment: when the function is called try_finally, should the cleanup part not be called finally rather than always assuming it refers to Java's try-catch-finally syntax? Alternatively it could be called try_always.

After a peak following my request for participation on the discuss forum, the poll has been quiet for a while again. At least we have doubled the votes.

I have updated the PR with the winning signature. A screenshot of the comment box a minute ago is attached to this comment.

Appveyor failed because it took too long to compile and run the tests: "Build execution time has reached the maximum allowed time for your plan (60 minutes)."

FWIW now here's the one in guestfs: https://github.com/libguestfs/libguestfs/blob/2547df8a0de46bb1447396e07ee0989bc3f8f31e/common/mlstdutils/std_utils.mli#L328

Now what we really want is a return statement: https://github.com/libguestfs/libguestfs/blob/2547df8a0de46bb1447396e07ee0989bc3f8f31e/common/mlstdutils/std_utils.mli#L340

Is there any reason not to merge this?

I realize now that @alainfrisch, who has been driving most of the standard-library issue, is not cc-ed in this PR, so maybe that's one reason why he hasn't given an opinion yet. (Or maybe people are in holidays in August or less active.)

I would be inclined to merge once the CI passes. @gasche do you agree? (BTW, Alain is on holiday, so I don't expect him to chime in soon).

@nojb: I don't have an opinion.

Personally I find it too difficult to make decisions on standard library changes because the design space is very large and so subjective. When I don't see objective reasons to prefer one choice to another, I prefer to not make a choice at all. I'm glad that @alainfrisch (and you, apparently) volunteered to take on the burden of making those decisions, and I'll let you do just that.

(I think that the Option/Result modules that went in today as part of this Standard Library Festival Week are just fine, and I'm glad that you merged them. Thanks!)

Merged; unfortunately I didn't notice that the history was quite a mess; it would have been much better to squash before.

Sorry, I was waiting for all the comments before squashing them