Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modulo operator has broken associativity #298

Open
neeshy opened this issue Jun 16, 2024 · 5 comments
Open

Modulo operator has broken associativity #298

neeshy opened this issue Jun 16, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@neeshy
Copy link

neeshy commented Jun 16, 2024

> 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
Copy link
Owner

printfn commented Jun 19, 2024

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

@printfn printfn added the bug Something isn't working label Jun 19, 2024
printfn added a commit that referenced this issue Jun 19, 2024
@printfn
Copy link
Owner

printfn commented Jun 19, 2024

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
Copy link
Author

neeshy commented Jun 20, 2024

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
Copy link
Author

neeshy commented Jun 20, 2024

Just found this:

#247 (comment)

I guess this would be more involved.

@printfn
Copy link
Owner

printfn commented Jun 20, 2024

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.

@printfn printfn changed the title Modulo operator has broken associativity (and drops units?) Modulo operator has broken associativity Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants