Avoid RangeError in arrayBind foreign implementation#314
Merged
natefaubion merged 5 commits intopurescript:masterfrom Feb 15, 2025
Merged
Conversation
Demonstrating that the current implementation of `Array`'s `Bind` instance causes `RangeError: Maximum call stack size exceeded` when the output of `f` in `ma >>= f` is sufficiently large. This is due to usage of `Function.prototype.apply`. From [MDN](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply#using_apply_and_built-in_functions): > But beware: by using apply() (or the spread syntax) with an arbitrarily long arguments list, you run the risk of exceeding the JavaScript engine's argument length limit. > The consequences of calling a function with too many arguments (that is, more than tens of thousands of arguments) is unspecified and varies across engines. (The JavaScriptCore engine has a hard-coded [argument limit of 65536](https://webkit.org/b/80797).) Node v20.18.1 seems to have a higher limit around 106,000.
natefaubion
requested changes
Feb 14, 2025
pete-murphy
commented
Feb 15, 2025
Using static check to determine if `Array.prototype.flatMap` is available, and use `var` instead of `let` in for loop to match existing code style.
pete-murphy
force-pushed
the
pete/309-array-bind-argument-limit
branch
from
f878325 to
fbb41ce
Compare
natefaubion
approved these changes
Feb 15, 2025
garyb
approved these changes
Feb 15, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of the change
Fixes #309. See the related discussion on Discourse.
Array.prototype.push.applyflatMapif available in thearrayBindimplementationArray.prototype.push.applyoptimizationI don't know the motivation behind that optimization, so I am likely not aware of the tradeoffs. I tried some benchmarks in various JS run times, not seeing wildly-different results between existing solution (
#ff725c),flatMap(#4269d0), and simplified version added in e9ad550 (#efb118):See also #311
Checklist: