Skip to content
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

Normative: Allow toString of a built-in function to output a computed property name #2695

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gibson042
Copy link
Contributor

e.g., as output by XS:

$ eshost -se 'Proxy.revocable({}, {}).revoke.toString()'
#### ChakraCore, SpiderMonkey
function() {
    [native code]
}

#### engine262, GraalJS, Hermes, V8
function () { [native code] }

#### JavaScriptCore
function () {
    [native code]
}

#### Moddable XS
function [""] (){[native code]}

@gibson042 gibson042 added needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 16, 2022
@@ -30110,19 +30110,47 @@ <h1>Function.prototype.toString ( )</h1>
1. Let _func_ be the *this* value.
1. If Type(_func_) is Object and _func_ has a [[SourceText]] internal slot and _func_.[[SourceText]] is a sequence of Unicode code points and HostHasSourceTextAvailable(_func_) is *true*, then
1. Return CodePointsToString(_func_.[[SourceText]]).
1. If _func_ is a <emu-xref href="#sec-built-in-function-objects">built-in function object</emu-xref>, 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>, 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, then calling NativeFunctionName with the returned String as the argument must return the value of _func_.[[InitialName]].
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the normative change is rejected, we should probably keep these structural changes because the current text is incoherent in multiple ways:

  1. It doesn't seem correct to talk about something being matched by a sequence of grammar nonterminals.
  2. Grammar nonterminals match against sequences of code points, not against Strings.
  3. When func.[[InitialName]] is an empty String and the returned value is like "function () {…}", then wouldn't the matched portion be undefined and therefore fail to match it?

@ljharb
Copy link
Member

ljharb commented Mar 16, 2022

Does anyone but xs do this? This doesn't really seem to match the intention of toString reform, altho "it can't parse" certainly means it validates the letter.

It seems like perhaps a simpler approach is to add test262 tests to prohibit what xs is doing, assuming they're willing to change their behavior?

@gibson042
Copy link
Contributor Author

gibson042 commented Mar 16, 2022

That's possible, but note that every major engine and most minor ones also output similar strings in certain scenarios (albeit not necessarily for built-in functions):

$ eshost -se '(function(){
  const sym1 = Symbol("foo"), sym2 = Symbol("bar"), obj = { [sym1](){}, get [sym2](){} };
  const m1 = obj[sym1], m2 = Object.getOwnPropertyDescriptor(obj, sym2).get;
  return [m1, m2, m1.bind(), m2.bind()].map(
    fn => `${fn} # => ${JSON.stringify(fn.name)}`.replace(/\s*([(){}])\s*/g, " $1 ").replace(/\s+/g, " ")
  ).join("\n");
})()'
#### ChakraCore

TypeError: No implicit conversion of Symbol to String

#### engine262, SpiderMonkey, V8
[sym1] ( ) { } # => "[foo]"
get [sym2] ( ) { } # => "get [bar]"
function ( ) { [native code] } # => "bound [foo]"
function ( ) { [native code] } # => "bound get [bar]"

#### JavaScriptCore
function ( ) { } # => "[foo]"
function ( ) { } # => "get [bar]"
function [foo] ( ) { [native code] } # => "bound [foo]"
function get [bar] ( ) { [native code] } # => "bound get [bar]"

#### Moddable XS
function ["[foo]"] ( ) { [native code] } # => "[foo]"
function ["get [bar]"] ( ) { [native code] } # => "get [bar]"
function ["bound [foo]"] ( ) { [native code] } # => "bound [foo]"
function ["bound get [bar]"] ( ) { [native code] } # => "bound get [bar]"

@michaelficarra
Copy link
Member

The title of this PR is misleading. Computed property names are already allowed in Function.prototype.toString output. The grammar changes here are simply a refactoring. Can you update the title to describe the actual normative requirement change?

@gibson042
Copy link
Contributor Author

The title of this PR is misleading. Computed property names are already allowed in Function.prototype.toString output. The grammar changes here are simply a refactoring. Can you update the title to describe the actual normative requirement change?

The current output for built-in functions (with a String-valued [[InitialName]]) is constrained by "the portion of the returned String that would be matched by |NativeFunctionAccessor?| |PropertyName| must be the value of func.[[InitialName]]" (emphasis mine), which does not permit a computed property name because String contents like [""] do not match the value of "" (even though they are grammatically permitted by |NativeFunction|).

So I believe that the title already describes this normative change, although I'm open to alternative suggestions.

1. Let _propertyNameNode_ be the |PropertyName| of _name_.
1. Let _propertyName_ be the result of evaluating _propertyNameNode_.
1. Assert: Type(_propertyName_) is String.
1. NOTE: The |PropertyName| production in general may be a |ComputedPropertyName| evaluating to a Symbol (e.g., `[Symbol()]`), but this is not the case for any built-in function that is directly accessible to ECMAScript code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this restriction coming from? I don't think this is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only executed in fulfillment of an assertion about [[InitialName]], which is only set in SetFunctionName via CreateBuiltinFunction—and whose requirements are somewhat scattered. Every explicit use of CreateBuiltinFunction specifies name as a string, and every implicit use is covered by the combination of CreateIntrinsics ("All values that are built-in function objects are created by performing CreateBuiltinFunction(steps, length, name, slots, realmRec, prototype) where… name is the initial value of the function's "name" property") and ECMAScript Standard Built-in Objects:

Each built-in function defined in this specification is created by calling the CreateBuiltinFunction abstract operation (10.3.3). The values of the length and name parameters are the initial values of the "length" and "name" properties as discussed below. The values of the prefix parameter are similarly discussed below.

Every built-in function object, including constructors, has a "name" property whose value is a String.

The value of the "name" property is explicitly specified for each built-in functions whose property key is a Symbol value. [for example, Symbol.prototype [ @@toPrimitive ] and get RegExp [ @@species ]]

@michaelficarra
Copy link
Member

I don't like the strategy of parsing and evaluating the property name here, especially if all of PropertyName is allowed, which potentially includes arbitrary expressions. If we're introducing a new AO anyway, just change NativeFunctionName to permit a subset of the PropertyName grammar and introduce an SDO that extracts the name from that. If it's not clear what I mean, we can get on a call or you can join the weekly editor call.

Copy link
Contributor Author

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the strategy of parsing and evaluating the property name here, especially if all of PropertyName is allowed, which potentially includes arbitrary expressions. If we're introducing a new AO anyway, just change NativeFunctionName to permit a subset of the PropertyName grammar and introduce an SDO that extracts the name from that.

Done.

1. Let _propertyNameNode_ be the |PropertyName| of _name_.
1. Let _propertyName_ be the result of evaluating _propertyNameNode_.
1. Assert: Type(_propertyName_) is String.
1. NOTE: The |PropertyName| production in general may be a |ComputedPropertyName| evaluating to a Symbol (e.g., `[Symbol()]`), but this is not the case for any built-in function that is directly accessible to ECMAScript code.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only executed in fulfillment of an assertion about [[InitialName]], which is only set in SetFunctionName via CreateBuiltinFunction—and whose requirements are somewhat scattered. Every explicit use of CreateBuiltinFunction specifies name as a string, and every implicit use is covered by the combination of CreateIntrinsics ("All values that are built-in function objects are created by performing CreateBuiltinFunction(steps, length, name, slots, realmRec, prototype) where… name is the initial value of the function's "name" property") and ECMAScript Standard Built-in Objects:

Each built-in function defined in this specification is created by calling the CreateBuiltinFunction abstract operation (10.3.3). The values of the length and name parameters are the initial values of the "length" and "name" properties as discussed below. The values of the prefix parameter are similarly discussed below.

Every built-in function object, including constructors, has a "name" property whose value is a String.

The value of the "name" property is explicitly specified for each built-in functions whose property key is a Symbol value. [for example, Symbol.prototype [ @@toPrimitive ] and get RegExp [ @@species ]]

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Jul 13, 2022
@ljharb
Copy link
Member

ljharb commented Jul 13, 2022

@gibson042 separate from the mechanism of doing the change, can you elaborate on why this is desirable?

</dl>
<emu-alg>
1. Let _function_ be ParseText(_sourceText_, |NativeFunction|).
1. If _name_ Contains |NativeFunctionName| is *false*, return *""*.
Copy link
Member

@michaelficarra michaelficarra Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_name_ isn't introduced yet here. Also, don't use Contains for this since FormalParameters can contain arbitrary expressions. You can say something like "if |NativeFunctionName| is present in _function_".

P.S. this is why an SDO would have been better here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've updated accordingly.

1. Throw a *TypeError* exception.
1. Assert: ParseText(_sourceText_, |NativeFunction|) is a Parse Node.
1. If _func_ has an [[InitialName]] internal slot and _func_.[[InitialName]] is a String, then
1. Assert: NativeFunctionName(_sourceText_) is _func_.[[InitialName]].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asserts are used to describe existing invariants. The current text imposes the normative requirement on implementations that the [[InitialName]] must be present in the string representation which this current PR loses. I don't have a concrete suggestion right now but the new text should directly communicate the normative requirement to have the [[InitialName]].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@gibson042
Copy link
Contributor Author

gibson042 commented Jul 14, 2022

separate from the mechanism of doing the change, can you elaborate on why this is desirable?

There are editorial aspects which are important to make the current text actually coherent (which I will split out into a separate PR: #2828), and normative aspects relating to consistency of valid output from Function.prototype.toString(callableWithoutASourceTextSlot)... right now the output for a built-in function is much more constrained than the output for other such callables such as proxies and/or bound versions of accessor and/or symbol-named functions (cf. #2695 (comment) ) in that the [[InitialName]] requirement (seems to) preclude a computed name, but not so constrained that it is actually deterministic (in that the presence and form of whitespace is unspecified).

But if there is implementer appetite for tackling this in the other direction (making Function.prototype.toString output for built-in and/or other non-[[SourceText]] callables more deterministic), I would be happy to abandon this PR in favor of such a replacement.

@michaelficarra
Copy link
Member

@gibson042
Copy link
Contributor Author

Right. That issue was never resolved, and it's not clear to me how it would be without implementer commitment to align.

@gibson042 gibson042 force-pushed the 2022-03-buitin-function-tostring-name branch from 9f5369e to 4be4db2 Compare July 15, 2022 19:31
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jul 27, 2022
@gibson042
Copy link
Contributor Author

Per the July 2022 TC39 meeting, this should be replaced with https://github.com/tc39/proposal-stricter-function-tostring .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants