Fix out-of-bounds when converting from FixedDecimal to PluralOperands

[sffc]

FixedDecimal support numbers that are very large or very small. However, PluralOperands only supports numbers with integer and fraction parts that fit inside a u64. Discuss how to resolve cases when the FixedDecimal is larger than what the PluralOperands can support.

[filmil]

Perhaps `Try` -> TryFrom` is all that is needed. As an optimization, FixedDecimal could provide a flag that is set if any of its components would overflow u64. Another possible option, is that in the context of plural operands, not all digits are actually significant. Perhaps we could make do by keeping in PluralOperands only those digits that actually matter for plural formation. For example, in Serbian 12345689110 and 110 are equivalent as far as plurality is concerned.

…

On Fri, Oct 2, 2020 at 3:04 PM Shane F. Carr ***@***.***> wrote: FixedDecimal support numbers that are very large or very small. However, PluralOperands only supports numbers with integer and fraction parts that fit inside a u64. Discuss how to resolve cases when the FixedDecimal is larger than what the PluralOperands can support. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#290>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB4GMBHPZG6H54GAV7GF3DSIZE7PANCNFSM4SCGJVAA> .

[sffc]

I lean toward saying that we just throw away digits that are out of bounds. We could cap the number of digits at 18 integer and 18 fraction or something like that.

[zbraniecki]

In stdlib those are usually having lossy in the name. I'd suggest tryfrom and lossy and or panicking

[sffc]

It's technically lossy, but for all real situations, keeping 18 digits on both side of the decimal separator is far more than enough to run plural rules. Panicking is definitely not the right behavior. I'm not a fan of TryFrom because this is a hot code path. I suppose we could add TryFrom in addition to From (if that's allowed, given that TryFrom has a default implementation based on From).

[zbraniecki]

It's technically lossy, but for all real situations, keeping 18 digits on both side of the decimal separator is far more than enough to run plural rules.

Sure, so then calling it from_fixed_decimal_lossy gives you:
a) correct description
b) communicates that it is lossy
c) appropriate overhead/perf

It's the same as From but with additional bit of information about lossiness.

[sffc]

Do you suggest we do not implement From<FixedDecimal> for PluralOperands and instead implement a from_fixed_decimal_lossy function?

[zbraniecki]

Hmm, I'm torn. I see that clamping to 18 digits may be seen not as lossy here, but sort of loss of precision much like u64 as u32 is ot isize as usize is, rather than what String::*_lossy_* would do by potentially altering the string to cut out malformed chars.

And I understand the convenience value of using From here rather than just method name.

My only hiccup is that I'd like us to be able to have a path forward if later we want to add a fallible version because in some scenario we need to handle the overflow differently.

If having both, TryFrom and From is not possible, then
maybe then we could add a fallible constructor like try_from_fixed_decimal?

[filmil]

Hmm, I'm torn. I see that clamping to 18 digits may be seen not as lossy here, but sort of loss of precision much like u64 as u32 is ot isize as usize is, rather than what String::*_lossy_* would do by potentially altering the string to cut out malformed chars.

FWIW, I don't think this conversion is actually lossy: it does remove information of the original exact number, but that does not lose information about its plurality. It is a one-way transform: PluralOperands is as much lossy to FixedDecimal as PluralCategory is to PluralOperands. You can't recover 23 from PluralCategory::Few, but we don't call the transform select_lossy.

(By taking the least significant digits from both the integer and fractional part, we will probably remove any distinction between the "full" and "lossy" conversions.)

[zbraniecki]

I'm convincend. From should work then and we shouldn't worry about lossiness.

[sffc]

One other thought I had. We don't want overflowed PluralOperands to select =0 and =1 rules. For example, if you have 1e20+1, you don't want to get the =1 rule.

At what point in the process do you think clients are intended to support =0 and =1?

[zbraniecki]

At what point in the process do you think clients are intended to support =0 and =1?

As in MessageFormat variant matching?