Skip to content

feat: give unary % operator (percentage) higher precedence than binary % operator (modulus) #3432

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

Conversation

kiprobinsonknack
Copy link

@kiprobinsonknack kiprobinsonknack commented Mar 26, 2025

This is a fix/change to #3349

This gives unary % operator (percentage) higher precedence than binary % operator (modulo). This also makes unary % operator have higher precedence than *, /, .*, ./, and mod operators.

This partially rolls back changes from #3311; however, intended behavior from that change is preserved: the precedence of binary % operator (modulo) has not changed. For example, 11*12%13 is still treated as (11*12)%13, whereas in MathJS v13 it was treated as 11*(12%13).

Breaking Change

This is a BREAKING CHANGE and should get a new major version:

  • v13: 100 / 50% = 100 / (50%) = 100 / 0.5 = 200
  • v14: 100 / 50% = (100 / 50)% = 2% = 0.02
  • this PR: 100 / 50% = 100 / (50%) = 100 / 0.5 = 200 (restores v13 behavior in this scenario)

Another example where 13/14/PR behavior are all different:

  • MathJS v13: 10 / 200 % % 3 = 10 / ((200%) % 3) = 10 / (2 % 3) = 10 / 2 = 5
  • MathJS v14: 10 / 200 % % 3 = ((10 / 200)%) % 3 = ((0.05)%) % 3 = 0.0005 % 3 = 0.0005
  • This PR: 10 / 200 % % 3 = (10 / (200%)) % 3 = (10 / 2) % 3 = 5 % 3 = 2

Additional change:

200%% previously failed with Unexpected end of expression (char 5) error; with this PR it fails with Unexpected operator % (char 5) error. (This was intentional as I believe it is a clearer error.)


Additional comments

I looked into ways of handling something like 200%%% = (200%)%)% = ((2)%)% = (0.02)% = 0.0002, but I couldn't find a way to make it work without breaking other tests. Given that it was already an error in previous version, and it's unlikely (in my estimation) that someone would actually want to do that, I didn't investigate further into fixing.

Additionally, I found that 5%(13) is treated as 5 mod 13 rather than (5%) * 13 (implicit multiplication). My PR doesn't change this behavior, and I believe it is actually desirable because the parser doesn't care about whitespace so 5%(13) and 5% (13) and 5 % (13) are all handled the same way. But it may technically be wrong based on the operator precedence documented in syntax.md.

@kiprobinsonknack kiprobinsonknack changed the title feat: give unary % operator higher precedence than binary % operator (modulus) feat: give unary % operator (percentage) higher precedence than binary % operator (modulus) Mar 26, 2025
@josdejong
Copy link
Owner

Thanks Kip, this looks good at fist sight! I'll review and test this in more indepth next week.

@kiprobinsonknack
Copy link
Author

Thanks Kip, this looks good at fist sight! I'll review and test this in more indepth next week.

@josdejong any update?

@josdejong
Copy link
Owner

Yeah sorry for the delay, I'm having trouble keeping up with all the mathjs issues lately but it is definitely on my list. Thanks for the reminder.

@josdejong josdejong mentioned this pull request Apr 16, 2025
8 tasks
@josdejong josdejong added this to the v15 milestone Apr 16, 2025
@josdejong josdejong changed the base branch from develop to v15 April 16, 2025 09:22
Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kip! The PR looks well taken care of. I really like this improvement/fix, it makes sense.

About cases like 200%%%: thanks for improving the error message. This makes sense. If someone really want to do this calculation, he/she, can add parenthesis like ((200%)%)%.

I made one inline remark to simplify parseUnaryPercentage a bit, can you have a look into that?

Also, I changed the base of the PR to a new v15 branch, since this will be a breaking change.

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kiprobinsonknack, I'll merge your PR now. I do not know when we'll release v15, but I keep track of that in #3453.

@josdejong josdejong merged commit 5a27c11 into josdejong:v15 Apr 16, 2025
8 checks passed
gwhitney pushed a commit that referenced this pull request Apr 22, 2025
gwhitney pushed a commit that referenced this pull request Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants