search

printfn / fend

Arbitrary-precision unit-aware calculator
https://printfn.github.io/fend
MIT License
642 stars 51 forks source link

Modulo operator has broken associativity #298

Open neeshy opened 3 months ago

neeshy commented 3 months ago 
> 16 mod (16 mebi)
0
> 16 mod (6 mebi)
4

It seems that trying to force associativity drops the units within the parentheses. The expression is evaluated as simply x mod y. Without parentheses the expression:

> 16 mod 6 mebi
4194304

Evaluates as (16 mod 6) mebi

As a comparison:

> 16 + 16 mebi == 16 + (16 mebi)
true
> 16 mod 6 mebi == (16 mod 6) mebi
true
printfn commented 3 months ago

Thanks for reporting the issue. Thankfully this shouldn't be too hard to fix.

printfn commented 3 months ago

I've now fixed the issue where units were being ignored.

One thing I'm unsure about is the associativity. Currently:

> 16 mod 6 mebi
4194304
> (16 mod 6) mebi
4194304
> 16 mod (6 mebi)
16

I can't really decide if this should be changed or not. Since most units (kg etc.) don't work inside a modulo expression anyway, I think the current associativity might be fine?

neeshy commented 3 months ago

My two cents: mod having higher precedence than units feels inconsistent with the usage of other operators. Consider:

16 mod 6 kg

Without knowing anything else about fend, I would expect this expression to produce an error. Especially considering the behavior of other operators. I guess it would decrease convenience (it's unlikely that the user actually meant 16 mod (16 kg) if he did write this), but nonetheless it seems more predictable.

With dimensionless units, I would especially expect mod to have lower precedence than the unit (since in that case, it's a valid expression). The person who writes 16 mod 6 mebi likely does mean 16 mod (6 mebi).

EDIT: Actually, division works similarly to modulo w.r.t. precedence:

> 16 / 4 kg
4 kg

Feels like it should evaluate to 4 kg^-1 but IDK, could go either way in that case. For reference, both rink and numbat evaluate to 4 kg^-1.

neeshy commented 3 months ago

Just found this:

https://github.com/printfn/fend/issues/247#issuecomment-1868373153

I guess this would be more involved.

printfn commented 3 months ago

I've now released v1.4.9 which fixes the bug with dropping units, but I'll leave this issue open for the associativity problems.