rust-num / num-complex

Complex numbers for Rust
Apache License 2.0
232 stars 49 forks source link

Error in from_str_radix if radix > 18; avoid unfortunate interaction #90

Closed bluss closed 2 years ago

bluss commented 3 years ago

from_str_radix: Error if radix is > 18

Radix 19 and 20 introduce 'i' and 'j' as digits and this conflicts with how complex are parsed. Before this fix, it could parse Num::from_str_radix("1ij + 1ij", 20) in a pretty surprising way.

The Num docs already describe a rule for our situation - we might not support every radix in our implementation; we can return an error in this case instead.

A new (internal) error kind is introduced for this.

It is necessary to preserve the previous panicking behaviour(?) for radix > 36 (this is what primitive types do - but other types are allowed to do differently, including never panicking).

bluss commented 3 years ago

Where and if to panic and where to check for the error involves some nonobvious choices. I'd prefer if "radix <= 18" is a precondition to calling "from_str_generic", i.e that this function doesn't have to care for possible 'i' as a digit and so on.

I'd also be fine to just have radix <= 16 as the requirement if that just seems easier to understand.

One alternative not explored here is to invent a new format for complex numbers - and use that for radix > 18. I don't have a great suggestion, but imagine if it was something like 1 + 0# where # is the imaginary unit - it needs to be some symbol that's not in ascii alphanumerics, unfortunately.

Playground link - showcase weird parses

As an example of a weird parse, "1i" is 38 as a real number in the type i32 with radix 20. But "1i + 1i" parses as the complex 38 + i in type Complex<i32> radix 20.

radix: 20 input: 1i i32 Ok(38) num_complex::Complex Ok(Complex { re: 0, im: 1 }) radix: 20 input: 1j i32 Ok(39) num_complex::Complex Ok(Complex { re: 0, im: 1 }) radix: 20 input: 1i + 1i i32 Err(ParseIntError { kind: InvalidDigit }) num_complex::Complex Ok(Complex { re: 38, im: 1 }) radix: 20 input: 1ij i32 Ok(779) num_complex::Complex Ok(Complex { re: 0, im: 38 }) radix: 20 input: 1ij + 1ij i32 Err(ParseIntError { kind: InvalidDigit }) num_complex::Complex Ok(Complex { re: 779, im: 38 }) radix: 20 input: 1ji i32 Ok(798) num_complex::Complex Err(ParseComplexError { kind: ExprError }) radix: 20 input: 1ji + 1ji i32 Err(ParseIntError { kind: InvalidDigit }) num_complex::Complex Err(ParseComplexError { kind: ExprError })
cuviper commented 3 years ago

That's an unfortunate oversight, but I agree that an error is the most reasonable thing we can do.

cuviper commented 3 years ago

So I'm curious, how did you come to realize this? If it was in real use, then someone really did want a larger radix?

bluss commented 3 years ago

Oh, for some reason I was reading the from_str implementation and looked at how it handles i vs j (it looks hacky, but seems to cover all the cases). So I just noticed it, and I don't know any use case for this. Instead of an annoying bug report I wrote a PR directly.

I have a feeling that the "<= 18" limit is irregular and it would seem better to only support <= 16

cuviper commented 2 years ago

bors r+

bors[bot] commented 2 years ago

Build succeeded: