Normative: fix Function toString on builtins #1948

devsnek · 2020-04-14T16:16:28Z

Fixes #1941

bathos · 2020-04-14T17:30:41Z

Summarizing a bit from the original issue thread:

JSC & V8 currently don’t conform to these rules (builtin accessors end up having two identifiers)
Spidermonkey does conform (by omitting the "get"/"set" prefixes)
this change would clarify the expected behavior by codifying Spidermonkey’s solution

michaelficarra · 2020-04-14T19:01:55Z

Why don't we just have SetFunctionName set an internal slot with the name parameter instead of using the strange "without the prefix" wording?

devsnek · 2020-06-26T15:56:50Z

What if we got rid of the prose about length and name in the built in function definition area and added SetFunctionLength and SetFunctionName calls to CreateBuiltinFunction?

ljharb · 2020-06-26T15:59:48Z

The prose ensures name and length are the implicit values that would be intuitive, if I’m understanding what you’re referring to. How would you envision it looking with your change?

devsnek · 2020-06-26T16:06:08Z

@ljharb i was thinking something like this:

1. Let length be the length of the function specified by _steps_. Unless otherwise specified, this is largest number of named arguments shown in the subclause headings for _steps_. Optional parameters (which are indicated with brackets: [ ]) or rest parameters (which are shown using the form «...name») are not included in the default argument count.
1. Perform ! SetFunctionLength(_F_, _length_).
1. Let name be this value is the name that is given to the function specified by _steps_. Functions that are identified as anonymous functions use the empty String as the value of the "name" property. For functions that are specified as properties of objects, the name value is the property name string used to access the function.
1. If _F_ is specified as a get or set assessor function, let _prefix_ be "get " or "set ".
1. Perform ! SetFunctionName(_F_, _name_, _prefix_).

spec.html

gibson042 · 2020-07-21T20:00:26Z

#1941 (comment)

There appears to be a majority desire on the committee for native accessors to be represented similarly to author-defined accessors, which manifest differently based on how they are defined:

get PropertyName () { FunctionBody } in an object initializer evaluates with name matching "get " + PropertyName and [[SourceText]] containing the matched source text starting with "get" per 14.3 Method Definitions.

get () { FunctionBody } in a property descriptor evaluates with name "get" and [[SourceText]] containing the matched source text starting with "get" per the same.

get: function ( FormalParameters ) { FunctionBody } in a property descriptor evaluates with name "get" and [[SourceText]] containing the function expression starting with "function" per 12.2.6.8 Runtime Semantics: PropertyDefinitionEvaluation.

Given that built-in accessors have "get " or "set " prepended to their name per 17 ECMAScript Standard Built-in Objects, it seems like option 1 is the best fit and Map.prototype.__lookupGetter__('size').toString() should return something like "get size() { [native code] }" (without a "function" substring).

spec.html

ExE-Boss · 2020-07-21T22:26:21Z

spec.html

@@ -26469,7 +26475,7 @@ <h1>Function.prototype.toString ( )</h1>
        <p>When the `toString` method is called, the following steps are taken:</p>
        <emu-alg>
          1. Let _func_ be the *this* value.
-          1. If _func_ is a <emu-xref href="#sec-bound-function-exotic-objects">bound function exotic object</emu-xref> or a <emu-xref href="#sec-built-in-function-objects">built-in function object</emu-xref>, then return an implementation-defined String source code representation of _func_. The representation must have the syntax of a |NativeFunction|. Additionally, if _func_ is a <emu-xref href="#sec-well-known-intrinsic-objects">Well-known Intrinsic Object</emu-xref> and is not identified as an anonymous function, the portion of the returned String that would be matched by |PropertyName| must be the initial value of the *"name"* property of _func_.
+          1. If _func_ is a <emu-xref href="#sec-bound-function-exotic-objects">bound function exotic object</emu-xref> or a <emu-xref href="#sec-built-in-function-objects">built-in function object</emu-xref>, then return an implementation-defined String source code representation of _func_. The representation must have the syntax of a |NativeFunction|. Additionally, if _func_ has an [[InitialName]] internal slot and _func_.[[InitialName]] is a String, the portion of the returned String that would be matched by |NativeFunctionAccessor?| |PropertyName| must be the value of _func_.[[InitialName]].


This can be changed to:

Suggested change

1. If _func_ is a <emu-xref href="#sec-bound-function-exotic-objects">bound function exotic object</emu-xref> or a <emu-xref href="#sec-built-in-function-objects">built-in function object</emu-xref>, then return an implementation-defined String source code representation of _func_. The representation must have the syntax of a |NativeFunction|. Additionally, if _func_ has an [[InitialName]] internal slot and _func_.[[InitialName]] is a String, the portion of the returned String that would be matched by |NativeFunctionAccessor?| |PropertyName| must be the value of _func_.[[InitialName]].

1. If _func_ is a <emu-xref href="#sec-built-in-function-objects">built-in function object</emu-xref>, then return an implementation-defined String source code representation of _func_. The representation must have the syntax of a |NativeFunction|. Additionally, if _func_ has an [[InitialName]] internal slot and _func_.[[InitialName]] is a String, the portion of the returned String that would be matched by |NativeFunctionAccessor?| |PropertyName| must be the value of _func_.[[InitialName]].

Since Bound Function exotic objects can be handled by the same step that handles Proxy exotic objects and they don’t have [[InitialName]].

Should [[InitialName]] be carried over by bound function exotic objects? It seems like that was implied by the "initial name property"

I don’t think so given that bound function exotic objects aren’t intrinsics, and thus are already not covered by the current requirement for the PropertyName portion.

i think this is correct as is. The text puts the propertyname requirement only on functions which have an [[InitialName]] slot that has a string.

That being said the bound functions are all over the place too

'use strict'; const F = Object.getOwnPropertyDescriptor(Map.prototype, 'size').get; print('1', F.toString()); print('2', F.bind(null).toString());

┌────────────────┬───────────────────────────────────────────────┐ │ ChakraCore │ 1 get size │ │ │ 2 function() { │ │ │ [native code] │ │ │ } │ ├────────────────┼───────────────────────────────────────────────┤ │ engine262 │ 1 function get size() { [native code] } │ │ engine262+ │ 2 function bound get size() { [native code] } │ ├────────────────┼───────────────────────────────────────────────┤ │ GraalJS │ 1 function get size() { [native code] } │ │ │ 2 function bound() { [native code] } │ ├────────────────┼───────────────────────────────────────────────┤ │ JavaScriptCore │ 1 function get size() { │ │ │ [native code] │ │ │ } │ │ │ 2 function get size() { │ │ │ [native code] │ │ │ } │ ├────────────────┼───────────────────────────────────────────────┤ │ Moddable XS │ 1 function get size (){[native code]} │ │ │ 2 function bound get size (){[native code]} │ ├────────────────┼───────────────────────────────────────────────┤ │ QuickJS │ 1 function get size() { │ │ │ [native code] │ │ │ } │ │ │ 2 function bound get size() { │ │ │ [native code] │ │ │ } │ ├────────────────┼───────────────────────────────────────────────┤ │ SpiderMonkey │ 1 function size() { │ │ │ [native code] │ │ │ } │ │ │ 2 function() { │ │ │ [native code] │ │ │ } │ ├────────────────┼───────────────────────────────────────────────┤ │ V8 │ 1 function get size() { [native code] } │ │ │ 2 function () { [native code] } │ └────────────────┴───────────────────────────────────────────────┘

My expectation (which we discussed doing in a followon PR) is that all of those with "get" or "set", would basically be get x() { or bound get x() {.

devsnek · 2020-07-26T00:27:23Z

Tests PR: tc39/test262#2716

devsnek · 2020-07-26T04:37:16Z

Based on some testing we could actually refine NativeFunction more, if we wanted:

NativeFunction : `function` NativeFunctionAccessor? NativeFunctionName? `(` `)` `{` `[` `native` `code` `]` `}`
NativeFunctionAccessor :
  `get`
  `set`
NativeFunctionName :
  IdentifierName
  `[` `Symbol` `.` IdentifierName `]`

bathos · 2020-07-26T11:10:27Z

@devsnek Wouldn't NativeFunctionAccessor? NativeFunctionName? be ambiguous given IdentifierName includes "get" and "set"? (Or maybe that doesn't matter here, since unlike with rules used during parsing, it's not really necessary to know which production was matched, only that any of them would be..?)

michaelficarra · 2020-07-27T17:06:40Z

@devsnek That would require approval from committee again. I suggest we move forward with what was approved already.

michaelficarra · 2020-07-27T17:08:07Z

@bathos Yes, we do not require an unambiguous parse in this case.

syg · 2020-07-29T21:12:37Z

spec.html

@@ -24918,7 +24924,7 @@ <h1>ECMAScript Standard Built-in Objects</h1>
    <p>For example, the function object that is the initial value of the *"map"* property of the Array prototype object is described under the subclause heading &laquo;Array.prototype.map (callbackFn [ , thisArg])&raquo; which shows the two named arguments callbackFn and thisArg, the latter being optional; therefore the value of the *"length"* property of that function object is 1.</p>
  </emu-note>
  <p>Unless otherwise specified, the *"length"* property of a built-in function object has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *true* }.</p>
-  <p>Every built-in function object, including constructors, has a *"name"* property whose value is a String. Unless otherwise specified, this value is the name that is given to the function in this specification. Functions that are identified as anonymous functions use the empty String as the value of the *"name"* property. For functions that are specified as properties of objects, the name value is the property name string used to access the function. Functions that are specified as get or set accessor functions of built-in properties have *"get "* or *"set "* prepended to the property name string. The value of the *"name"* property is explicitly specified for each built-in functions whose property key is a Symbol value.</p>
+  <p>Every built-in function object, including constructors, has a *"name"* property whose value is a String. Unless otherwise specified, this value is the name that is given to the function in this specification. Functions that are identified as anonymous functions use the empty String as the value of the *"name"* property. For functions that are specified as properties of objects, the name value is the property name string used to access the function. Functions that are specified as get or set accessor functions of built-in properties have *"get "* or *"set "* prepended to the property name string. The value of the *"name"* property is explicitly specified for each built-in functions whose property key is a Symbol value. The *"name"* property is set using SetFunctionName.</p>


What actually calls SetFunctionName for the built-in functions? Does 8.2.2 need to be amended?

Discussed this on the editor call. Clause 17 should be amended to actually set [[InitialName]]. Clause 8.2.2 does not need to change, but could use clarification that it is not steps, but is a description of the how the values in the table were created.

What kind of change are you looking for here? I already amended it to explicitly say what logic should be used to set the name.

I guess with #2116 we don't need to explicitly say how to set the name here because we already say they should be created with CreateBuiltinFunction?

spec.html

syg · 2020-07-29T21:16:18Z

spec.html

@@ -8878,11 +8883,12 @@ <h1>CreateBuiltinFunction ( _steps_, _internalSlotsList_ [ , _realm_ [ , _protot
        1. If _realm_ is not present, set _realm_ to the current Realm Record.
        1. Assert: _realm_ is a Realm Record.
        1. If _prototype_ is not present, set _prototype_ to _realm_.[[Intrinsics]].[[%Function.prototype%]].
-        1. Let _func_ be a new built-in function object that when called performs the action described by _steps_. The new function object has internal slots whose names are the elements of _internalSlotsList_.
+        1. Let _func_ be a new built-in function object that when called performs the action described by _steps_. The new function object has internal slots whose names are the elements of _internalSlotsList_, and an [[InitialName]] internal slot.


Commenting here as a reminder. Don't need to be acted on for this PR.

This object creation is weird. Built-in objects can either be ordinary function objects or exotic. This AO tries to be generic for both, with loose language about the internal slots in prose. We should clean this up to be similar to other object allocation sites and, and perhaps also split the exotic version from the ordinary versoin.

(re PR tc39#1948)

michaelficarra added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels Apr 14, 2020

ljharb requested review from kmiller68, michaelficarra, msaboff, syg, ljharb, bakkot and a team April 14, 2020 18:31

ljharb added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Apr 14, 2020

devsnek force-pushed the fix-func-tostring branch 3 times, most recently from c7358d2 to b9f22de Compare April 16, 2020 02:24

devsnek force-pushed the fix-func-tostring branch from b9f22de to c050d55 Compare June 26, 2020 15:21

devsnek mentioned this pull request Jun 26, 2020

Do dotted intrinsics count as well‑known intrinsic objects? #2068

Open

devsnek force-pushed the fix-func-tostring branch 2 times, most recently from 03e9ece to 374eece Compare June 26, 2020 15:41

ljharb approved these changes Jun 26, 2020

View reviewed changes

ljharb mentioned this pull request Jun 26, 2020

Editorial: remove legacy intrinsics #2056

Merged

devsnek force-pushed the fix-func-tostring branch from 374eece to c0eb838 Compare June 26, 2020 16:15

ljharb reviewed Jun 26, 2020

View reviewed changes

spec.html Outdated Show resolved Hide resolved

devsnek force-pushed the fix-func-tostring branch from c0eb838 to 27af2e0 Compare June 26, 2020 16:29

michaelficarra reviewed Jun 26, 2020

View reviewed changes

spec.html Outdated Show resolved Hide resolved

devsnek force-pushed the fix-func-tostring branch from 27af2e0 to fd78f5c Compare June 26, 2020 18:08

michaelficarra reviewed Jun 26, 2020

View reviewed changes

spec.html Outdated Show resolved Hide resolved

ljharb reviewed Jul 21, 2020

View reviewed changes

spec.html Outdated Show resolved Hide resolved

michaelficarra approved these changes Jul 21, 2020

View reviewed changes

ExE-Boss reviewed Jul 21, 2020

View reviewed changes

devsnek force-pushed the fix-func-tostring branch from e417f39 to 7a1783b Compare July 22, 2020 15:04

ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jul 22, 2020

devsnek force-pushed the fix-func-tostring branch from 7a1783b to 7dd854e Compare July 26, 2020 00:12

devsnek mentioned this pull request Jul 26, 2020

New NativeFunction tests tc39/test262#2716

Merged

syg reviewed Jul 29, 2020

View reviewed changes

syg added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Jul 29, 2020

devsnek added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Aug 5, 2020

syg approved these changes Aug 5, 2020

View reviewed changes

ljharb self-assigned this Aug 5, 2020

Normative: fix Function toString on builtins (tc39#1948)

552407b

ljharb force-pushed the fix-func-tostring branch from 7dd854e to 552407b Compare August 5, 2020 22:22

ljharb merged commit 552407b into tc39:master Aug 5, 2020

ljharb mentioned this pull request Aug 5, 2020

Editorial: remove %Generator% and %AsyncGenerator% in favor of dotted forms #2065

Merged

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020

Editorial: Insert blank line between productions

4551216

(re PR tc39#1948)

jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020

Editorial: Insert blank line between productions

181cb8c

(re PR tc39#1948)

ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request Aug 11, 2020

Editorial: Insert blank line between productions (tc39#2129)

f3455c2

(re PR tc39#1948)

h2oche pushed a commit to h2oche/ecma262 that referenced this pull request Aug 16, 2020

Editorial: Insert blank line between productions (tc39#2129)

bdb41f0

(re PR tc39#1948)

devsnek deleted the fix-func-tostring branch April 12, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: fix Function toString on builtins #1948

Normative: fix Function toString on builtins #1948

devsnek commented Apr 14, 2020

bathos commented Apr 14, 2020

michaelficarra commented Apr 14, 2020

devsnek commented Jun 26, 2020

ljharb commented Jun 26, 2020

devsnek commented Jun 26, 2020 •

edited

Loading

gibson042 commented Jul 21, 2020

ExE-Boss Jul 21, 2020 •

edited

Loading

devsnek Jul 22, 2020

ExE-Boss Jul 23, 2020 •

edited

Loading

devsnek Jul 23, 2020

devsnek Jul 23, 2020

ljharb Jul 23, 2020

devsnek commented Jul 26, 2020

devsnek commented Jul 26, 2020

bathos commented Jul 26, 2020 •

edited

Loading

michaelficarra commented Jul 27, 2020

michaelficarra commented Jul 27, 2020

syg Jul 29, 2020

syg Jul 29, 2020

devsnek Jul 29, 2020

devsnek Jul 29, 2020

syg Jul 29, 2020

Normative: fix Function toString on builtins #1948

Normative: fix Function toString on builtins #1948

Conversation

devsnek commented Apr 14, 2020

bathos commented Apr 14, 2020

michaelficarra commented Apr 14, 2020

devsnek commented Jun 26, 2020

ljharb commented Jun 26, 2020

devsnek commented Jun 26, 2020 • edited Loading

gibson042 commented Jul 21, 2020

ExE-Boss Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Jul 23, 2020 • 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

devsnek commented Jul 26, 2020

devsnek commented Jul 26, 2020

bathos commented Jul 26, 2020 • edited Loading

michaelficarra commented Jul 27, 2020

michaelficarra commented Jul 27, 2020

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

devsnek commented Jun 26, 2020 •

edited

Loading

ExE-Boss Jul 21, 2020 •

edited

Loading

ExE-Boss Jul 23, 2020 •

edited

Loading

bathos commented Jul 26, 2020 •

edited

Loading