Fallible conversion from string

[JanBeh]

Throwing Invalid_argument in Z.of_string, Q.of_string, and related functions seems to be an unfortunate API choice.

In the OCaml manual, Invalid_argument is an exception that is not meant to be caught:

exception Invalid_argument of string

Exception raised by library functions to signal that the given arguments do not make sense. The string gives some information to the programmer. As a general rule, this exception should not be caught, it denotes a programming error and the code should be modified not to trigger it.

Compare float_of_string, which consequently raises a Failure exception on invalid input, which is an exception meant to be thrown and caught under normal operations.

As there is no Q.of_string_opt or Q.is_valid_string function or similar, a user either has to implement their own code to check string validity or has to catch Invalid_argument, which is generally not intended to be done.

It would likely be more idiomatic to throw Failure instead of Invalid_argument on wrong (user) input. See also this discussion/post on discuss.ocaml.org on a different subject.

Maybe it is worth investigating whether catching Invalid_argument should be considered deprecated, and users of the zarith library could be advised to catch both Invalid_argument and Failure to allow for a transition in a future version of the API.

[xavierleroy]

Thanks for your report. I agree that Failure would be more appropriate than Invalid_argument to report syntax errors in conversion functions from strings, for the same reasons that float_of_string and int_of_string from OCaml's standard library use Failure. Technically, it would be a breaking change in Zarith, but practically I think it would work just fine.

@antoinemine @pascal-cuoq : what do you think about this?

[antoinemine]

I'd think that Invalid_argument makes more sense. The condition occurs when the client passes an argument that does not satisfy a precise pre-condition, completely deterministic and provided in the documentation (or should be). The client code can (and should) make sure that the string obeys this condition, whether or not we provide a function to do the check at run-time. If the client obeys this principle, it does not have to rely on the exception, and should not catch it as part of its normal execution.
On the contrary, I'd say that a Failure is an error that is, at least partially, out of the control of the client. It cannot foresee it, and has no reliable way to ensure that it will not occur by checking a condition on the arguments. So, it may need to catch it.

That said, this is not a strong opinion. I suppose some code (possibly a lot) relies on exceptions to check the validity of the argument. Having consistency there would be more pleasing to programmers: no need to adapt the way of programming for newcomers to Zarith when switching data-types.

However, breaking changes are bad. I don't want to ask the programmer change their code over such a simple matter. I especially don't want to ask the programer to support both the old and the new, incompatible versions. I'd like the evolutions of the library to be able to support both new code, and old code (without change, if possible).

[JanBeh]

Right now, the precise rules for when an argument is valid are rather complex and not (formally) documented. The documentation refers to "decimal point and scientific notations". I assume this refers to how OCaml typically parses these numbers? But it's not clear. Also, the behavior of empty strings are a bit surprising. (Or maybe not?) Consider these:

• "4_5" -> 45
• "4___5" -> 45
• "0xp0" -> 0
• "12/" -> inf
• "/5" -> 0
• "/5_" -> 0
• "/_5" -> Invalid_argument
• "/" -> undef
• "" -> 0

Also compare with int_of_string "", for example, which gives Failure, and not zero.

It is at least not trivial to check which combinations exactly will result in an exception and which won't. (Of course, one could be more strict than necessary in the pre-check.)

I don't think Invalid_argument is meant to report the result of such a (relatively complex) parsing result. As I understand, it is typically thrown when arguments "do not make sense" (which is that the manual says), and which the programmer can easily avoid.

But the input for a parsing function often originates from I/O (e.g. user input), and as such, I would argue, even a badly formatted input is something that may reasonably occur in a program, but the output isn't defined for that input, which is exactly what Failure is for:

exception Failure of string

Exception raised by library functions to signal that they are undefined on the given arguments. [...]

But semantics aside, given the fact that Invalid_argument shouldn't be caught, is the stronger reason why I believe Invalid_argument is not a good choice. Maybe the OCaml manual should be more clear in that matter and giving better aid in distinguishing between these two exceptions (which is another issue, of course).

I suppose some code (possibly a lot) relies on exceptions to check the validity of the argument.

I see this from two perspectives:

• Consistency and elegancy of code
• Practical matters

I personally would opt for consistency and fixing previously made mistakes, but that's of course not my decision to make (I just shared my opinion here, which I hope is alright). However, I also see practical issues from the programmer's side: Reading the current documentation, both the OCaml manual and the zarith documentation, I am not entirly sure what to do:

• Is the "general" rule that Invalid_argument should not be caught meant to have exemptions? I.e. should I disregard what I learned about Invalid_argument and just be practical here (and maybe run into problems later, because I was never supposed to catch this exception)?
• Should I really write my own syntax checker (and possibly implement something redundantly, just because I want to follow some "general" advice)?

It would be nice if the user of the library gets some guidance here. If the exception was Failure, it would be clear what to do. If you really want to stick to Invalid_argument (for whichever reason), maybe there could be at least some advice what to do.

However, breaking changes are bad.

I understand, but I think it's also not ideal to carry the consequences of unfortunate design decisions far into the future. In my opinion this problem could likely be solved with relatively little effort – especially considering that programmers shouldn’t have caught the exception in the first place. But it's a matter of priorities, stability vs consistency, of course.

[JanBeh]

Given the complexity of validity checking, I plan to use the following workaround in my own code, at least until there is some clarity on how fallible conversion is supposed to be handled:

try Q.of_string str
with Invalid_argument _ | Failure _ ->
  (* See https://github.com/ocaml/Zarith/issues/170 *)
  failwith "Invalid string representation of rational number"

[antoinemine]

I still think that this change could be dangerous. A cursory analysis shows there are currently 666 opam files in opam-repository containing the word zarith. Any of these packages, or the packages they depend on, could rely on Z.of_string or Q.of_string raising an Invalid_argument, as this is what the documentation says it does, or maybe none of them do. Until I have strong evidence that we are not breaking anything with the change you suggest, I remain against it.
There are other conversion functions raising an Invalid_argument: bool_of_string, Char.chr, String.escaped.

[JanBeh]

I understand if you don't want to solve the issue in a specific way. However, you might want to provide a recommendation what to do to perform a fallible conversion from string. What would you suggest? Simply catch Invalid_argument, even if the OCaml manual (generally) advises against it? Write an own test function? Or some other solution? If you are concerned about breaking changes, maybe introduce a Q.of_string_opt or Q.is_valid_string or something like that?

[JanBeh]

Regarding the various options I proposed, note that recommending users to catch Invalid_argument would make a potential migration path (as outlined in my OP) even more difficult in future. Deprecating catching the current exception would not break code per-se.

[antoinemine]

Adding _opt versions for conversion functions that may fail in Z and Q is a good solution to error handling. I'm not convinced depreciation or migration paths are necessary, we could keep existing functions as they are, in parallel with the new functions.

[JanBeh]

Even if I still don't agree on Invalid_argument being a good choice (the examples bool_of_string, Char.chr, String.escaped didn't convince me either, even though bool_of_string is somewhat odd in the stdlib, also referred to as "odd duck" here), adding the _opt versions would give an answer to the question of "how to perform conversion from strings in a fallible manner". 👍

Note that I'm not against using Invalid_argument per-se. My PR #172 suggests using Invalid_argument when the input argument for Q.mul2exp is out of range (doc patch).

Either way, the _opt functions would solve my concerns.

[pascal-cuoq]

Hello @xavierleroy

sorry for this extremely late reply.

If we were discussing the interface of a brand-new library, I would argue against re-using any exception from the OCaml runtime, as any function can also raise these. A couple of fresh exceptions per module are a ridiculously cheap price to pay for the certainty that one is catching only exceptions coming from the function(s) one intends.

But we're not, and the only thing I can say at this point is that it would be very annoying to me personally if the exceptions raised by existing functions were changed. I am almost certain that we do not have regression tests for all situations (regression tests checking, for instance, that a clean error message is the result of passing a string that doesn't represent an integer to a commandline option that expects one, are boring to write and even more boring to maintain; they always seem to need updating once you have them).