Normative: Wrapped function should reuse target name and length #339

leobalter · 2021-11-02T16:55:20Z

Closes #338

This makes WrappedFunctionCreate more consistent with Function.prototype.bind

legendecas · 2021-11-02T17:09:35Z

spec.html

+					1. Set _L_ to max(_targetLenAsInt_, 0).
+			1. Perform ! SetFunctionLength(_wrapped_, _L_).
+			1. Let _name_ be ? Get(_Target_, *"name"*).
+			1. Perform ! SetFunctionName(_wrapped_, _name_, *"wrapped"*).


This makes the wrapping back and forth between several shadow realms observable from user code, like "wrapped wrapped wrapped function". Is this behavior expected?

I didn't think of that, and I'm not sure what's the best alternative other than just removing the prefix.

It wouldn't be different to Function#bind

function fn() {} var bound = fn.bind().bind().bind(); bound.name; // 'bound bound bound fn'

Right, it is the same with bind. I'm just confirming that this doesn't break any security assumptions.

Since anyone could at any time overwrite the name to be "wrapped wrapped wrapped anything", anyone with runtime assumptions about what a configurable name implies is already broken.

btw, the spec was written in a way that implementers can optimize the wrapping to wrap the original target rather than the wrapped one to avoid the different jumps, and we should preserve that. And I believe we should be careful to preserve that.

Prefixing should not prevent any call optimization, right? The name is defined at wrapping time, where the target resolution is at call time.

legendecas · 2021-11-02T17:35:57Z

spec.html

+			1. Let _L_ be 0.
+			1. Let targetHasLength be ? HasOwnProperty(_Target_, *"length"*).
+			1. If _targetHasLength_ is *true*, then
+			1. Let _targetLen_ be ? Get(_Target_, *"length"*).


Nit: indentation for steps nested with the condition If _targetHasLength_ is *true*, then?

ljharb · 2021-11-02T17:35:00Z

spec.html

+					1. Set _L_ to max(_targetLenAsInt_, 0).
+			1. Perform ! SetFunctionLength(_wrapped_, _L_).
+			1. Let _name_ be ? Get(_Target_, *"name"*).
+			1. Perform ! SetFunctionName(_wrapped_, _name_, *"wrapped"*).


Note that name can be an object. Consider:

function f() {} Object.defineProperty(f, 'name', { value: f }); f.name === f

ljharb · 2021-11-02T17:36:30Z

spec.html

+			1. Let _targetLen_ be ? Get(_Target_, *"length"*).
+			1. If Type(_targetLen_) is Number, then
+				1. If _targetLen_ is *+&infin;*<sub>𝔽</sub>, set _L_ to +&infin;.
+				1. Else if _targetLen_ is *-&infin;*<sub>𝔽</sub>, set _L_ to 0.
+				1. Else,
+				1. Let _targetLenAsInt_ be ! ToIntegerOrInfinity(_targetLen_).
+					1. Assert: _targetLenAsInt_ is finite.
+					1. Set _L_ to max(_targetLenAsInt_, 0).


These lines need to be indented.

ljharb · 2021-11-02T17:37:17Z

spec.html

+			1. Let _L_ be 0.
+			1. Let targetHasLength be ? HasOwnProperty(_Target_, *"length"*).
+			1. If _targetHasLength_ is *true*, then
+			1. Let _targetLen_ be ? Get(_Target_, *"length"*).
+			1. If Type(_targetLen_) is Number, then
+				1. If _targetLen_ is *+&infin;*<sub>𝔽</sub>, set _L_ to +&infin;.
+				1. Else if _targetLen_ is *-&infin;*<sub>𝔽</sub>, set _L_ to 0.
+				1. Else,
+				1. Let _targetLenAsInt_ be ! ToIntegerOrInfinity(_targetLen_).
+					1. Assert: _targetLenAsInt_ is finite.
+					1. Set _L_ to max(_targetLenAsInt_, 0).
+			1. Perform ! SetFunctionLength(_wrapped_, _L_).


Since these exact steps are now repeated both here and in function bind, perhaps it should be factored into an AO?

I'm fine doing that but I'd like to first define the final steps of this, as it seems like some tostring will be necessary inbetween.

I don’t think converting to a string would make sense since afaik bind doesn’t do that - i think you’d want to throw if it was a non-passable value.

Makes sense to wait for the refactor until you see what steps are shared.

ljharb · 2021-11-02T17:38:05Z

spec.html

+					1. Set _L_ to max(_targetLenAsInt_, 0).
+			1. Perform ! SetFunctionLength(_wrapped_, _L_).
+			1. Let _name_ be ? Get(_Target_, *"name"*).
+			1. Perform ! SetFunctionName(_wrapped_, _name_, *"wrapped"*).


Since anyone could at any time overwrite the name to be "wrapped wrapped wrapped anything", anyone with runtime assumptions about what a configurable name implies is already broken.

Fixes #338

leobalter · 2021-11-02T19:10:58Z

spec.html

+			1. Let _targetName_ be ? Get(_Target_, *"name"*).
+			1. If Type(_targetName_) is not String, set _targetName_ to the empty String.


@ljharb this is interesting! Function#bind normalizes to only copy string names, so I also applied this change here as you mentioned in the comments above.

There is a catch where we need to avoid bringing error objects from the target function and that can happen on the HasOwnProperty and Get calls.

I believe that doing the common abstraction - as you suggested - would make it easier to handle this. I'll follow up with a new commit soon.

leobalter · 2021-11-02T19:44:17Z

@ljharb PTAL

ljharb · 2021-11-02T20:24:49Z

spec.html

+		</emu-clause>
+
+		<emu-clause id="sec-copynameandlength" aoid="CopyNameAndLength">
+			<h1>CopyNameAndLength ( _F_, _Target_, _prefix_ [ , _argCount_ ] )</h1>


Suggested change

<h1>CopyNameAndLength ( _F_, _Target_, _prefix_ [ , _argCount_ ] )</h1>

<h1>

CopyNameAndLength (

_F_: a function object,

_Target_: a function object,

_prefix_: a String,

optional _argCount_: a Number,

)

</h1>

is this a new notation?

yes, the entire spec uses it for AOs since tc39/ecma262#545 was merged in July.

ljharb · 2021-11-02T20:27:07Z

spec.html

+				1. Let _result_ be CopyNameAndLength(_wrapped_, _Target_, *"wrapped"*).
+				1. If _result_ is an Abrupt Completion, throw a TypeError exception.


Suggested change

1. Let _result_ be CopyNameAndLength(_wrapped_, _Target_, *"wrapped"*).

1. If _result_ is an Abrupt Completion, throw a TypeError exception.

1. Perform ? CopyNameAndLength(_wrapped_, _Target_, *"wrapped"*).

currently this is catching the almost-certainly-TypeError and throwing a TypeError, which would be masking the actual cause

the issue here is not with TypeError, is not leaking cross realms error objects.

ah, i see. shouldn't the message, or the cause, or the type (if it's a builtin) be preserved if possible?

there is no definition for the message, same as in PerformShadowRealmEval.

There is no logic yet for the cause, and I'd prefer to do that in a separate PR and tackle all these points throwing TypeError.

ljharb · 2021-11-02T20:28:18Z

spec.html

 </emu-clause>

 <emu-clause id="sec-shadowrealm-objects">
 	<h1>ShadowRealm Objects</h1>
 	<emu-clause id="sec-shadowrealm-abstracts">
 		<h1>ShadowRealm Abstract Operations</h1>

+		<emu-clause id="sec-wrappedfunctioncreate" aoid="WrappedFunctionCreate">
+			<h1>WrappedFunctionCreate ( _callerRealm_, _Target_ )</h1>


Suggested change

<h1>WrappedFunctionCreate ( _callerRealm_, _Target_ )</h1>

<h1>

WrappedFunctionCreate (

_callerRealm_: a Realm Record,

_Target_: an ECMAScript language value,

)

</h1>

and then the assertion on line 134 can be removed.

leobalter · 2021-11-02T21:55:42Z

@ljharb we have the formatted header now. Is there anything else for this PR?

Jack-Works · 2021-11-03T04:01:18Z

What's the prior art of this change? If it's performance consideration, I think it should be host defined, host either copy the name and length or do nothings

mhofman · 2021-11-03T04:09:36Z

I'd be opposed to host defined behavior. We want more predictable behavior, not less

jdalton · 2021-11-03T23:20:16Z

I like the being similar to bind approach too.

caridy

LGTM

leobalter · 2021-11-09T22:34:42Z

This PR should observe any outcome from #338 (comment)

leobalter · 2022-01-19T17:46:20Z

This achieved consensus during the TC39 plenary in Dec 2021.

leobalter added 2 commits November 2, 2021 09:53

Editorial: Rename targetFunction to Target

b920761

This makes WrappedFunctionCreate more consistent with Function.prototype.bind

Editorial: Rename obj to wrapped

4671b87

leobalter requested review from rwaldron, caridy, philomates, syg, Jack-Works and legendecas November 2, 2021 16:55

leobalter added the needs consensus label Nov 2, 2021

leobalter mentioned this pull request Nov 2, 2021

Are wrapped functions supposed to have "name" and "length" instance properties? #338

Closed

legendecas reviewed Nov 2, 2021

View reviewed changes

ljharb reviewed Nov 2, 2021

View reviewed changes

Normative: Wrapped function should reuse target name and length

66470ac

Fixes #338

leobalter force-pushed the 338-leo/wrapped-name-length branch from 16733e7 to 66470ac Compare November 2, 2021 19:02

Normalize target name only acceptable as a string

bc5195c

leobalter commented Nov 2, 2021

View reviewed changes

Move WrappedFunctionCreate to the abstractions section

c712d70

ljharb reviewed Nov 2, 2021

View reviewed changes

leobalter added 2 commits November 2, 2021 13:41

Nit pick, flex abstraction return

6fcb0a5

Apply review feedback

2239a3e

ljharb approved these changes Nov 3, 2021

View reviewed changes

caridy approved these changes Nov 4, 2021

View reviewed changes

leobalter removed the needs consensus label Jan 19, 2022

rwaldron merged commit b73a1dc into main Jan 19, 2022

rwaldron deleted the 338-leo/wrapped-name-length branch January 19, 2022 17:47

leobalter mentioned this pull request Jan 31, 2022

Normative: Remove the name prefix for wrapped functions #348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Wrapped function should reuse target name and length #339

Normative: Wrapped function should reuse target name and length #339

leobalter commented Nov 2, 2021 •

edited

Loading

legendecas Nov 2, 2021

leobalter Nov 2, 2021

leobalter Nov 2, 2021

legendecas Nov 2, 2021

ljharb Nov 2, 2021

caridy Nov 3, 2021

mhofman Nov 3, 2021 •

edited

Loading

legendecas Nov 2, 2021

ljharb Nov 2, 2021

ljharb Nov 2, 2021

ljharb Nov 2, 2021

leobalter Nov 2, 2021

ljharb Nov 2, 2021 •

edited

Loading

ljharb Nov 2, 2021

leobalter Nov 2, 2021

leobalter commented Nov 2, 2021

ljharb Nov 2, 2021

leobalter Nov 2, 2021

ljharb Nov 2, 2021

ljharb Nov 2, 2021

leobalter Nov 2, 2021

ljharb Nov 2, 2021

leobalter Nov 2, 2021

ljharb Nov 2, 2021

leobalter commented Nov 2, 2021

Jack-Works commented Nov 3, 2021

mhofman commented Nov 3, 2021

jdalton commented Nov 3, 2021

caridy left a comment

leobalter commented Nov 9, 2021

leobalter commented Jan 19, 2022

		1. Let _targetName_ be ? Get(_Target_, "name").
		1. If Type(_targetName_) is not String, set _targetName_ to the empty String.

-			<h1>CopyNameAndLength ( _F_, _Target_, _prefix_ [ , _argCount_ ] )</h1>
+			<h1>
+			  CopyNameAndLength (
+			    _F_: a function object,
+			    _Target_: a function object,
+			    _prefix_: a String,
+			    optional _argCount_: a Number,
+			  )
+			</h1>

		1. Let _result_ be CopyNameAndLength(_wrapped_, _Target_, "wrapped").
		1. If _result_ is an Abrupt Completion, throw a TypeError exception.

	1. Let _result_ be CopyNameAndLength(_wrapped_, _Target_, "wrapped").
	1. If _result_ is an Abrupt Completion, throw a TypeError exception.
	1. Perform ? CopyNameAndLength(_wrapped_, _Target_, "wrapped").

Normative: Wrapped function should reuse target name and length #339

Normative: Wrapped function should reuse target name and length #339

Conversation

leobalter commented Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Nov 2, 2021

Jack-Works commented Nov 3, 2021

mhofman commented Nov 3, 2021

jdalton commented Nov 3, 2021

caridy left a comment

Choose a reason for hiding this comment

leobalter commented Nov 9, 2021

leobalter commented Jan 19, 2022

leobalter commented Nov 2, 2021 •

edited

Loading

mhofman Nov 3, 2021 •

edited

Loading

ljharb Nov 2, 2021 •

edited

Loading