Skip to content
This repository has been archived by the owner on Oct 8, 2024. It is now read-only.

Throw RangeError on bad numeric arguments to take/drop? #169

Closed
benjamingr opened this issue Jan 21, 2022 · 3 comments · Fixed by #181
Closed

Throw RangeError on bad numeric arguments to take/drop? #169

benjamingr opened this issue Jan 21, 2022 · 3 comments · Fixed by #181

Comments

@benjamingr
Copy link
Contributor

Currently, this proposal coerces arguments passed to drop and take to integers without range checks other than for negative numbers because ToIntegerOrInfinity is used to coerce the argument passed in.

This has the potential to create confusing behavior - for example naturals().take(NaN) and naturals().take((x) => x < 15) and naturals().take('aaa') all return an empty iterable.

I think most other places the spec does this get away with it by checking an explicit range (e.g. toString on numbers requiring the radix being at least 2) but admittedly some places don't (e.g. toExponential happily accepting NaNs).

I think it would provide better developer experience to get RangeErrors on NaNs (basically ToNumber the argument first and throw a RangeError on NaNs).

@bakkot
Copy link
Collaborator

bakkot commented Jan 30, 2022

Most places in the spec don't guard - .slice(NaN) will work, for example, and it's the closest thing to drop and take currently in the language. It's a pretty strong precedent. So this amounts to, do we think it's worth breaking with that precedent?

I think there's a pretty good case to be made for checking for NaN in particular - I don't think there's any good reason to want to treat NaN as zero; it's pretty much always a bug, and I think it's better for bugs to be loud (though much of the design of the language is not consistent with that principle). And this is a new kind of thing, so the consistency argument is less strong than it is for e.g. new methods on Array.

I'd be in favor of throwing when the argument coerces to NaN.

@bathos
Copy link

bathos commented Jan 30, 2022

I don't think there's any good reason to want to treat NaN as zero; it's pretty much always a bug

NaN as direct input, probably, but not as undefined. Pretty much everything that accepts an offset, index, or length allows omission/undefined because of ToIntegerOrInfinity: undefined -> NaN -> 0. So it's not just that slice(NaN) works, it's also why slice() works.

@bakkot
Copy link
Collaborator

bakkot commented Jan 30, 2022

NaN as direct input, probably, but not as undefined

Passing undefined here (or omitting the argument) is also pretty much always a bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants