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

asIntN and asUintN should both throw RangeError when n is less than zero #39

Closed
AnyhowStep opened this issue Feb 23, 2020 · 5 comments · Fixed by #65
Closed

asIntN and asUintN should both throw RangeError when n is less than zero #39

AnyhowStep opened this issue Feb 23, 2020 · 5 comments · Fixed by #65

Comments

@AnyhowStep
Copy link

With native BigInt,

BigInt.asIntN(-1, 1n)
> Uncaught RangeError: Invalid value: not (convertible to) a safe integer
BigInt.asUintN(-1, 1n)
> Uncaught RangeError: Invalid value: not (convertible to) a safe integer

With jsbi,

JSBI.asIntN(-1, JSBI.BigInt(1))
> "0"
JSBI.asUintN(-1, JSBI.BigInt(1))
> "0"

https://runkit.com/embed/tjflr2xtmtou

@AnyhowStep AnyhowStep changed the title asIntN and asUintN should both throw when n is less than zero asIntN and asUintN should both throw RangeError when n is less than zero Feb 23, 2020
@jakobkummerow
Copy link
Collaborator

Thanks for the report.

I'm actually not sure we want to fix this. There are several places where JSBI kind of assumes that you know what you're doing, and in the interest of performance doesn't check for nonsensical inputs. .asIntN(-1, ...) is an example of obviously nonsensical code. It's not just just negative numbers that aren't checked there; per spec we'd have to do a full ToIndex conversion (which would e.g. cover strings as input, as in .asIntN("4", ...)), but JSBI doesn't do that either, because the typical use case is to pass a small positive number there (probably 32 or 64 in the vast majority of cases), so that's what the implementation optimizes for.

I'm open to opinions here. Is the above a reasonable implementation policy? Or is it more useful to be 100% error-checking compatible with native BigInts for nonsensical, exception-throwing cases, even if that costs performance and bundle size?

@mathiasbynens
Copy link
Member

I agree we should be consistent and either add validation everywhere (which would be bad for performance) or continue to assume that the user knows what they're doing. Perhaps the way forward here is to document this philosophy a little bit more clearly, and provide a list of examples such as the one in OP's post. WDYT?

@joshxyzhimself
Copy link

Perhaps the way forward here is to document this philosophy a little bit more clearly, and provide a list of examples such as the one in OP's post. WDYT?

These would be wonderful lol! In my experience (or more like lack of experience with BigInt stuff) I am having a hard time comparing this JSBI and the four below:

As also discussed in web3/web3.js#2171

@jakobkummerow
Copy link
Collaborator

Sure, we can add more documentation and examples. @joshxyzhimself , would you like to draft something that you think would be useful?

Regarding JSBI's general motivation, we've tried to sum that up in the existing README.md. In short, it's meant to be a transitionary solution until native BigInt support is available in all browsers, which significantly informs its design, e.g. JSBI intentionally doesn't support anything that can't be easily mapped onto native BigInt behavior.

If there is demand for a fully type-checked version, I would be fine with adding a layer that does that (e.g. duplicate all JSBI.foo functions as TypeCheckedJSBI.foo or somesuch?). It's not clear to me how much demand there is for that, and it's unlikely that I'll have time to implement that myself any time soon, but I'd be happy to accept PRs. (If you want to start working on this, I'd recommend to go piecewise, e.g. start with just one function to explore what things would look like.)

@Yaffle
Copy link
Contributor

Yaffle commented Aug 10, 2021

JSBI.BigInt('-0o0') does not throw
is it a valid bug?

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 a pull request may close this issue.

5 participants