from_str_radix panics for some values of radix

[BurntSushi]

The from_str_radix methods on the various number types panics if radix < 2 or if radix > 36: https://doc.rust-lang.org/src/core/num/mod.rs.html#2693

I think we should fix this through one of two means:

1. Decide that it is a contract violation and document the cases where the function panics.
2. Decide that it is a normal error, be thankful that the ParseIntError internals aren't exposed, and return an error when radix is invalid. Finally, document the error condition.

cc @rust-lang/libs

[alexcrichton]

IIRC this is intentional, because the radix is 99% of the time a literal.

[sanmai-NL]

How can failing to parse something potentially untrusted ever be construed as a contract violation? In my eyes it is logical to return an Err instead here.

Other use-cases of assert! include testing and enforcing run-time invariants in safe code (whose violation cannot result in unsafety).

The contents of a string does not observe any invariant other than being a string.

[sanmai-NL]

@alexcrichton: be it as it may that the expected use case on string literals makes the assertion more acceptable, since this method is already fallible, is there a great cost to not using the assertion there?

[sanmai-NL]

If returning Results is found too costly for a parsing function, then perhaps it can be implemented to panic given a &'static str argument. That is, assuming that is deemed trusted input. This is a general suggestion, the API can't be changed so easily of course. For sure, the current implementation isn't consistent in its error handling approach.

[BurntSushi]

How can failing to parse something potentially untrusted ever be construed as a contract violation?

The contract violation is in the set of allowable values for the radix parameter, not the string.

[sanmai-NL]

@BurntSushi: Sorry, I was completely mistaken! Seems I misread there. 😕

I still wonder whether replacing the assertion with an early return has a significant performance cost at all. From my perspective, avoiding a panic is worth a lot, but perhaps some people with high performance requirements disagree?

[BurntSushi]

Why do you think performance has anything to do with this? Doesn't seem related to me.

[sanmai-NL]

Do you see another reason why the assertion was put in (1) instead of an Err return (2)? Do you agree 1 has the downside making handling errors harder? I do not see any downsides to 2 in this case. So I take it there must be a very good motivation to implement 1. When abort is the panic strategy, I believe 1 is less costly in the non-error case than 2. But I do not have benchmarks to back this up.

[bluss]

The radix parameter is a typical case for "contract violation" clause error handling, which is used when passing an invalid parameter is a programmer error / program bug and not an error encountered in normal operation. (That assumes the text can be unsanitized program input and the radix is not.)

Mixing panic and result error handling in the same call is tricky — do we have any other good examples of doing that? The worst of both those worlds is to not have it documented at all 😉

[tbu-]

Fun. When referencing the function in the other issue, I also noticed it didn't document the error so I went ahead and made a pull request (#43563).

[aturon]

+1 for declaring it a contract violation

[bluss]

With the merge of #43563, this was resolved, so any objection needs to be swift. The issue seems to be resolved otherwise.

[Phlosioneer]

Triage: I think this should be closed? No real decision was made; however, the PR to document it landed, and that seems to make everyone happy.

[bluss]

Thanks @Phlosioneer, this issue is resolved. The possible panic was documented in #43563.