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

Fix bigint div/mod to throw native RangeError #7154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Nov 9, 2024

There is no infinity or nan for bigint.

No need to check int semantic IMHO, bigint works totally different with number (int & float) anyway

@mununki
Copy link
Member

mununki commented Nov 11, 2024

I don't think I fully understand the intention of this PR. Why do we remove the check for RangeError (Division by zero) that could occur at runtime?

@cometkim
Copy link
Member Author

I don't think I fully understand the intention of this PR. Why do we remove the check for RangeError (Division by zero) that could occur at runtime?

We threw a similar but own custom error Division_by_zero, which is used by integer operations

@cometkim
Copy link
Member Author

I think it should at least be distinguishable from regular integer errors.

@cometkim
Copy link
Member Author

cometkim commented Nov 15, 2024

Or I think it would be better to have per-module definitions of exceptions like Primitive_int.Divide_by_zero and Primitive_bigint.Divide_by_zero

Any thoughts on this suggestion? @cknitt @cristianoc

@cristianoc
Copy link
Collaborator

Or I think it would be better to have per-module definitions of exceptions like Primitive_int.Divide_by_zero and Primitive_bigint.Divide_by_zero

Any thoughts on this suggestion? @cknitt @cristianoc

Without sub typing it seems strange to have to know where the exception is coming from.

@cometkim
Copy link
Member Author

sub typing? int and bigint have nothing to do with each other.

@cristianoc
Copy link
Collaborator

sub typing? int and bigint have nothing to do with each other.

Exactly: there's no way to catch all division by zero other than listing them all. This imposes some refactoring overhead and possible foot guns when toggling between using int and bigint. That's the downsides. I'm not sure about upsides.
Pushed to the extreme, one could have one different exception for each different unsafe operation in core.

@cometkim
Copy link
Member Author

I was thinking a little more in terms of JavaScript. Dividing a regular number by 0 returns Infinity.

But bigint is more explicit about everything. All operations on bigint are incompatible with numbers, and there are even no implicit conversions between them.

You mentioned listing of errors, but that's actually the case. Since the two operations never mix, there are two ways to track its cause originally in Js: checking Number.isFinite vs catching RangeError. Mixing it all up into one error case feels like a loss of information to me.

@cometkim
Copy link
Member Author

Another problem is that RangeError is the native means expected by JavaScript applications for bigint operations. As we defining different behavior, we have to explicitly state that bigint in ReScript behaves differently than in JavaScript.

@cristianoc
Copy link
Collaborator

It is true that if one wants to publish a library for consumption by JS then one better not change the semantics.
What's an example of how to catch RangeError from ReScript?

The question remains of what to do with int as (1/0)|0 is 0 and I doubt we would want to ignore division by zero in that way.

@cometkim
Copy link
Member Author

What's an example of how to catch RangeError from ReScript?

That's another feature we need to support natively, for all of JS' built-in Error classes

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.

3 participants