Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 36.4k
lib: add bound apply variants of varargs primordials#37005
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -21,10 +21,36 @@ const { | ||
| // `uncurryThis` is equivalent to `func => Function.prototype.call.bind(func)`. | ||
| // It is using `bind.bind(call)` to avoid using `Function.prototype.bind` | ||
| // and `Function.prototype.call` after it may have been mutated by users. | ||
| const { bind, call } = Function.prototype; | ||
| const { apply, bind, call } = Function.prototype; | ||
| const uncurryThis = bind.bind(call); | ||
| primordials.uncurryThis = uncurryThis; | ||
| // `applyBind` is equivalent to `func => Function.prototype.apply.bind(func)`. | ||
| // It is using `bind.bind(apply)` to avoid using `Function.prototype.bind` | ||
| // and `Function.prototype.apply` after it may have been mutated by users. | ||
| const applyBind = bind.bind(apply); | ||
| primordials.applyBind = applyBind; | ||
| // Methods that accept a variable number of arguments, and thus it's useful to | ||
| // also create `${prefix}${key}Apply`, which uses `Function.prototype.apply`, | ||
| // instead of `Function.prototype.call`, and thus doesn't require iterator | ||
| // destructuring. | ||
| const varargsMethods = [ | ||
| // 'ArrayPrototypeConcat' is omitted, because it performs the spread | ||
| // on its own for arrays and array-likes with a truthy | ||
| // @@isConcatSpreadable symbol property. | ||
| 'ArrayOf', | ||
| 'ArrayPrototypePush', | ||
| 'ArrayPrototypeUnshift', | ||
| // 'FunctionPrototypeCall' is omitted, since there's 'ReflectApply' | ||
| // and 'FunctionPrototypeApply'. | ||
| 'MathHypot', | ||
| 'MathMax', | ||
| 'MathMin', | ||
| 'StringPrototypeConcat', | ||
| 'TypedArrayOf', | ||
| ]; | ||
| function getNewKey(key) { | ||
| return typeof key === 'symbol' ? | ||
| `Symbol${key.description[7].toUpperCase()}${key.description.slice(8)}` : | ||
| @@ -51,7 +77,16 @@ function copyPropsRenamed(src, dest, prefix) { | ||
| if ('get' in desc) { | ||
| copyAccessor(dest, prefix, newKey, desc); | ||
| } else { | ||
| ReflectDefineProperty(dest, `${prefix}${newKey}`, desc); | ||
| const name = `${prefix}${newKey}`; | ||
| ReflectDefineProperty(dest, name, desc); | ||
| if (varargsMethods.includes(name)) { | ||
| ReflectDefineProperty(dest, `${name}Apply`, { | ||
| // `src` is bound as the `this` so that the static `this` points | ||
| // to the object it was defined on, | ||
| // e.g.: `ArrayOfApply` gets a `this` of `Array`: | ||
| value: applyBind(desc.value, src), | ||
aduh95 marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| @@ -63,10 +98,18 @@ function copyPropsRenamedBound(src, dest, prefix) { | ||
| if ('get' in desc) { | ||
| copyAccessor(dest, prefix, newKey, desc); | ||
| } else { | ||
| if (typeof desc.value === 'function') { | ||
| desc.value = desc.value.bind(src); | ||
| const { value } = desc; | ||
| if (typeof value === 'function') { | ||
| desc.value = value.bind(src); | ||
ExE-Boss marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| const name = `${prefix}${newKey}`; | ||
| ReflectDefineProperty(dest, name, desc); | ||
| if (varargsMethods.includes(name)) { | ||
| ReflectDefineProperty(dest, `${name}Apply`, { | ||
| value: applyBind(value, src), | ||
aduh95 marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| }); | ||
| } | ||
| ReflectDefineProperty(dest, `${prefix}${newKey}`, desc); | ||
| } | ||
| } | ||
| } | ||
| @@ -78,10 +121,18 @@ function copyPrototype(src, dest, prefix) { | ||
| if ('get' in desc) { | ||
| copyAccessor(dest, prefix, newKey, desc); | ||
| } else { | ||
| if (typeof desc.value === 'function') { | ||
| desc.value = uncurryThis(desc.value); | ||
| const { value } = desc; | ||
| if (typeof value === 'function') { | ||
| desc.value = uncurryThis(value); | ||
ExE-Boss marked this conversation as resolved.
Outdated
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| const name = `${prefix}${newKey}`; | ||
| ReflectDefineProperty(dest, name, desc); | ||
| if (varargsMethods.includes(name)) { | ||
| ReflectDefineProperty(dest, `${name}Apply`, { | ||
| value: applyBind(value), | ||
| }); | ||
| } | ||
| ReflectDefineProperty(dest, `${prefix}${newKey}`, desc); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| // Flags: --expose-internals | ||
| 'use strict'; | ||
| require('../common'); | ||
| const assert = require('assert'); | ||
| const { | ||
| ArrayOfApply, | ||
| ArrayPrototypePushApply, | ||
| ArrayPrototypeUnshiftApply, | ||
| MathMaxApply, | ||
| MathMinApply, | ||
| StringPrototypeConcatApply, | ||
| TypedArrayOfApply, | ||
| } = require('internal/test/binding').primordials; | ||
| { | ||
| const arr1 = [1, 2, 3]; | ||
| const arr2 = ArrayOfApply(arr1); | ||
| assert.deepStrictEqual(arr2, arr1); | ||
| assert.notStrictEqual(arr2, arr1); | ||
| } | ||
| { | ||
| const array = [1, 2, 3]; | ||
| const i32Array = TypedArrayOfApply(Int32Array, array); | ||
| assert(i32Array instanceof Int32Array); | ||
| assert.strictEqual(i32Array.length, array.length); | ||
| for (let i = 0, { length } = array; i < length; i++) { | ||
| assert.strictEqual(i32Array[i], array[i], `i32Array[${i}] === array[${i}]`); | ||
| } | ||
| } | ||
| { | ||
| const arr1 = [1, 2, 3]; | ||
| const arr2 = [4, 5, 6]; | ||
| const expected = [...arr1, ...arr2]; | ||
| assert.strictEqual(ArrayPrototypePushApply(arr1, arr2), expected.length); | ||
| assert.deepStrictEqual(arr1, expected); | ||
| } | ||
| { | ||
| const arr1 = [1, 2, 3]; | ||
| const arr2 = [4, 5, 6]; | ||
| const expected = [...arr2, ...arr1]; | ||
| assert.strictEqual(ArrayPrototypeUnshiftApply(arr1, arr2), expected.length); | ||
| assert.deepStrictEqual(arr1, expected); | ||
| } | ||
| { | ||
| const array = [1, 2, 3]; | ||
| assert.strictEqual(MathMaxApply(array), 3); | ||
| assert.strictEqual(MathMinApply(array), 1); | ||
| } | ||
| { | ||
| let hint; | ||
| const obj = { [Symbol.toPrimitive](h) { | ||
| hint = h; | ||
| return '[object Object]'; | ||
| } }; | ||
| const args = ['foo ', obj, ' bar']; | ||
| const result = StringPrototypeConcatApply('', args); | ||
| assert.strictEqual(hint, 'string'); | ||
| assert.strictEqual(result, 'foo [object Object] bar'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment like the
uncurryThisone which explains this is equivalent toThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it’s closer to:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, shouldn't we prefer using
Reflect.applyinstead ofFunction.prototype.apply? I think if someone calls those methods without a argument array, it's almost certainly a bug, and usingReflect.applywould help spot that. Or is there a performance reason to use one or the other?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well,
Reflect.applyisn’t very optimised in comparison to boundFunction.prototype.callandFunction.prototype.applycalls.Also, engines are able to generate better code for native bound functions compared to arrow functions that replicate the behaviour of a bound function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to use
const { apply } = Reflectcompared toconst { apply } = Function.prototype.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wouldn’t work, since then
applyBind(foo)would create a function that roughly does:(...args) => Reflect.apply.call(foo, ...args), instead of(...args) => Reflect.apply(foo, ...args).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, the equivalent would have to be:
Is that generally true statement or is it specific to V8? When reading the spec,
Reflect.applyseems to be doing less things thanFunction.prototype.apply, that's a bit counter-intuitive.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s mostly specific to V8, but I assume that it also applies to other C++-based engines, since V8 hasn’t put as much effort into optimising
Reflectas into optimisingObjectandFunctionmethods.Refs:#36740 (comment)