Support maximum_fraction_digits, and fix a panic on 32 bit systems

[dae]

Hi there,

Firstly, let me say thank you - I recently discovered Fluent, and it's a breath of fresh air after using gettext for a number of years. Thank you for all the thought and effort that has gone into it!

I had a user today report that our app was crashing when trying to render a translation with a floating point number. This turned out to be caused by an f64 with more than 9 decimal places - on a 32 bit system, it won't fit in a usize, so PluralOperands::try_from returns an error, and fluent-bundle .expect()s it not to.

The following patch adds support for maximum_fraction_digits, and caps it to 9 on 32 bit systems to ensure the usize won't overflow. I'm not sure if this is the best way to resolve the issue, but thought I'd share it in case it is useful.

[zbraniecki]

Hi @dae! Thank you for kind words and the PR!

[zbraniecki]

@dae - why did you decide to patch it in fluent-bundle rather than in https://github.com/zbraniecki/pluralrules/tree/master/intl_pluralrules ? It seems that, if I'm not mistaken, you're looking to prevent TryInto<PluralOperands> from panicking, no?

[dae]

I first saw https://github.com/zbraniecki/pluralrules/blob/master/intl_pluralrules/src/operands.rs#L185 and thought about implementing a conversion from f64, but if my understanding is correct (and it may well not be!), I don't see how that could work, since intl_pluralrules won't be aware of the formatting options that are in use when displaying the string.

I needed the ability to specify a maximum number of decimals anyway, so this was just the fastest way of addressing both issues. I guess https://github.com/zbraniecki/pluralrules/blob/master/intl_pluralrules/src/operands.rs#L93 could be adjusted to limit the number of digits instead.

Are you amenable to adding the maximum_fraction_digits part of the patch? If so, I can split this up and submit a separate request in intl_pluralrules for the capping part if that's what you'd prefer.

[zbraniecki]

Hi, I dove into your patch and realized that you exposed a bug in the intl_pluralrules crate.

Fixing it here would make fluent not affected, but keep the original crate with that bug. So I went ahead and wrote a PR for pluralrules. I tested that it fixes it (with a minor change here) for fluent as well.

Let me know if that solution is acceptable for you!

[dae]

I tried testing it, but I presume you haven't pushed the changes required to this repo yet:

error[E0308]: mismatched types
   --> fluent-bundle/src/types/number.rs:225:31
    |
225 |                 operands.f *= 10_usize.pow(mfd as u32 - operands.v as u32);
    |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `u64`, found `usize`

error[E0277]: cannot multiply-assign `usize` to `u64`
   --> fluent-bundle/src/types/number.rs:225:28
    |
225 |                 operands.f *= 10_usize.pow(mfd as u32 - operands.v as u32);
    |                            ^^ no implementation for `u64 *= usize`
    |
    = help: the trait `std::ops::MulAssign<usize>` is not implemented for `u64`

If it addresses the panics then that's great :-)

Re issues #32 and #31 mentioned on #33, I fear I am still missing something - don't the plural rules depend on how the number is to be formatted? For example if you have a float 1.0, if maximum decimal places were 2 then it would be "1.00 apples", but if they were 0, it would be "1 apple". So don't you either need to pass float in as a pre-formatted string, or make pluralrules aware of how the number is going to be formatted at the same time?

[zbraniecki]

I tried testing it, but I presume you haven't pushed the changes required to this repo yet:

You just need to change 10_usize to 10_u64.

[zbraniecki]

So don't you either need to pass float in as a pre-formatted string, or make pluralrules aware of how the number is going to be formatted at the same time?

You may be right! I may have to extend it there ^^!

[dae]

Thanks, I can confirm that my previous test no longer panics when using the updated pluralrules.

The thing that struck me as potentially difficult about starting with a float is that when the decimal places haven't been rigidly set by the caller, you'd need to determine how many decimal places are appropriate, and it seems like you'd end up reimplementing the logic in format that does so

assert_eq!(&format!("{}", 3.020000000000000007), "3.02");

The logic looks to be non-trivial:

https://github.com/rust-lang/rust/blob/master/src/libcore/num/flt2dec/mod.rs

[zbraniecki]

Yes, I agree. It's just a separate issue for me from a crash :)

[dae]

Closing this, as the panic issue has been fixed, and it's possible to enforce a maximum digit limit in a custom formatter instead.