Comment author: @dra27

The scenario you're describing is really the point of Warning 52 - that all a Failure exception tells you is that a failure occurred, and that you shouldn't be using the string in code to try to identify it further.

The Failure exception, as stated in the manual (http://caml.inria.fr/pub/docs/manual-ocaml/core.html#hevea_manual28), is intended to indicate that a function call is undefined for the given arguments. That statement is true regardless of where in the implementation of a given function that failure actually occurred.

You are correct that if there is a need to identify the precise kind of failure in a function, then the Failure exception is not appropriate for that implementation - but Warning 52 helps steer design in that direction, surely?

Comment author: @xavierleroy

we've already been through several of these issues in #7392. What is the new point you'd like to make and have discussed?

Comment author: @damiendoligez

The title of this PR is misleading because, as you point out, the real solution is not to change the documentation but the interface of most of the stdlib functions.

Comment author: bartjacobs

The real solution is to do both.

Yes, the issue described by this PR is being discussed in the thread for 0007392. But strictly speaking it's a distinct issue. Updating the stdlib docs to say "Raises Failure _ iff ..." would technically solve 007392 (although I would argue against that solution, because as a client, even though the docs now allow me to write code that is clearly correct with respect to the docs, I will not have great confidence that the code is correct with respect to the stdlib implementation, given how hard it is in general to ensure that Failure _ exceptions are not raised for other reasons) but it would not solve this issue. Also, updating the stdlib to include option-returning variants would solve 007392 to my probable satisfaction, but it would not solve this PR unless the warning 52 docs are updated as well.

But yes, there is really one general issue, which is "The exceptions story is somewhat broken."

Comment author: bartjacobs

dra: consider the hypothetical possibility that int_of_string internally uses some function that under certain conditions reads a string from a file and tries to turn it into an int using a recursive call of int_of_string. Now suppose that file is malformed due to an administrator error, but the hypothetical function does not try to catch this. Then we get a Failure "int_of_string" propagated from the outer int_of_string call, even though the argument to the outer int_of_string call might be perfectly well-formed.

This is an example of a scenario where even catching Failure "int_of_string" is dangerous. The case holds a fortiori for Failure _ (and for functions more complex than int_of_string). The place where your reasoning fails is that what is a domain error to one function, can be an internal error to a caller of that function.

You could eliminate this kind of scenario by requiring that callers always catch Failure _ and turn it into some other kind of exception (Assert_failure?) if they don't want to handle it more specifically. But, first of all, I don't think people are currently aware of (or agree on) this obligation, and secondly, even if people were aware of it and agreed on it, it's dangerous to assume that that obligation will always be fulfilled because it's hard not to forget about it.

The solution is to report domain errors (that clients might want to detect) as a special return value (like None) rather than an exception. Using exceptions to denote conditions that refer specifically to the arguments of the raising function is dangerous, because that means propagation is unsound, and exceptions are optimised for propagation rather than catching. (Note that updating the docs of Failure to say that the meaning is that some nested call got invalid inputs makes propagation of Failure sound, and properly alerts clients to the fact that they shouldn't deduce from catching a Failure that the call from which they caught the exception was the one that had the bad input.)

Comment author: @dra27

All great, and hypothetically sound - but given the absolute impossibility of changing the semantics (and certainly signature) of int_of_string, etc., what do you want to change?

Comment author: bartjacobs

I'm not sure I know a perfect solution, but the best approach I can currently think of is to A) introduce new variants of int_of_string and similar functions, called int_of_string_opt, that return an option, and B) fix the warning 52 docs to say that the _opt variants should be used instead of catching Failure.

Comment author: @alainfrisch

#885 is a proposal to add option-returning variants for stdlib functions (including *_of_string ones).

Comment author: @xavierleroy

This discussion is going into too many directions, so let me try to summarize.

1- Most of us would agree that the standard library should use fewer exceptions and more "option" or "error" result types. Rather than haphazardly patching the existing stdlib, I'm more interested in clean-slate designs like Batteries or Jane Street's libraries. Your mileage can vary, but at any rate this discussion is distinct from this PR.

2- I already wrote it but I'll write it again: when the documentation says that "function FOO raises Failure "message" if CONDITION", it really means that Failure "message" will be raised if and only if CONDITION holds, and that no Failure exception whatsoever will be raised if CONDITION doesn't hold. Apparently, @bartjacobs doesn't believe me because he keeps arguing assuming this claim is wrong.

3- As a consequence of 2-, it is safe and guaranteed to work to write: try Some(int_of_string s) with Failure _ -> None

4- I still have doubts that Failure "message" should trigger warning 52, but I'll take that with the Caml dev team.

Comment author: @xavierleroy

So, any action necessary besides "throw the stdlib away and use something else"? If not I move to close this PR.

Comment author: @alainfrisch

So, any action necessary besides "throw the stdlib away and use something else"?

A related action has been to add option-returning variants of many functions in the stdlib. This will be in 4.05. It has also been discussed to promote the use of these new functions and deprecate in a future version the exception throwing variants.

The question about whether warning 52 should be triggered for Failure was discussed on caml-devel, and the general preference was to keep it.

I don't think there is anything more to do around this general discussion for now. I'm also in favor of closing this PR.

Comment author: bartjacobs

Fine by me to close.

Reason: My thinking has evolved. I now believe that while one can imagine scenarios where catching Failure _ is dangerous, in the typical use case it is not dangerous, because typically the exception handler is fallback code and the only precondition it assumes is that the OCaml runtime/stdlib is in a valid state (which I assume is always true; if the OCaml runtime or stdlib is broken, the process should be killed rather than throwing Failure); it does not rely on whatever precondition the thrower guarantees, so it doesn't matter who the actual thrower is. At least, that is true for the cases where I am being asked to catch Failure _ in my codebase by the new warning.

Sorry for the damage done!

Comment author: bartjacobs

Actually, I have to nuance my previous note. While catching Failure _ is probably typically safe from a correctness point of view, there is still the slight risk that it masks a programming error. Any programming error thus masked probably won't cause incorrect behavior (because the fallback code doesn't care about the effects, correct or otherwise, of the failed code), but it's non-ideal nonetheless.

Comment author: @xavierleroy

OK, thanks all for the civil discussion and the improvements it triggered. Closing as agreed.