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: Set "name" property for anonymous functions #1490

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

anba
Copy link
Contributor

@anba anba commented Mar 21, 2019

Fixes #1049

692cdb2

  • [[SourceText]] was not assigned for class expressions with an inferred name and async arrow functions. Fixed here because the next commits will change the same algorithms.

91d3ac1

  • Changes the NamedEvaluation operations to no longer forward to Evaluation and splits Evaluation for ClassExpression. This in preparation for the next commit where NamedEvaluation will directly pass the function name to FunctionCreate.

99bf205

  • Add name property even if the function doesn't have an explicit or inferred name (Should anonymous functions get a name data property? #1049).
  • Set the function name property as part of FunctionInitialize. This matches the behaviour of JSC, Graal and V8, which all three create (resp. iterate) the name property before prototype for functions.
// Use a strict function to omit non-standard "caller" and "arguments".
print(Reflect.ownKeys(function f(){"use strict"}));
Engine Output
ES2019 length,prototype,name
SpiderMonkey prototype,length,name
Chakra prototype,length,name
JSC length,name,prototype
Graal length,name,prototype
V8 length,name,prototype

With this PR the iteration order will be changed to "length, name, prototype".

V8 was the only engine returning the correct property iteration order for classes, so it seems to be safe to change it, too.

print(Reflect.ownKeys(class C {}));
Engine Output
ES2019 length,prototype,name
SpiderMonkey prototype,length,name
Chakra prototype,length,name
JSC length,name,prototype
Graal length,name,prototype
V8 length,prototype,name (← matches spec!)

c6d0113

  • Changes the definition of anonymous function to mean a function with a name whose value is the empty string value.
  • The explicit name property attributes for %ThrowTypeError% are needed to ensure the function stays frozen.

Note:

  • I didn't add ! in abstract operations which didn't already use ! to keep the usage within a single operation consistent.
  • There's function name property and function "length" property, so name is always used without double-quotes whereas "length" always uses quotes. Is this intentional?
  • And I wonder if CreateBuiltinFunction should be changed to directly set name and length instead of relying on the declarative description of their values.

@ljharb ljharb 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. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 web reality labels Mar 22, 2019
@gsathya
Copy link
Member

gsathya commented Apr 17, 2019

Would it be possible to remove NamedEvaluation completely and always pass an appropriate parameter (the correct name or "") to Evaluation?

@gsathya
Copy link
Member

gsathya commented Jun 4, 2019

This PR got consensus at the TC39 meeting today. It'd be great to get some reviews so we can merge this.

@gsathya gsathya added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jun 4, 2019
@ljharb ljharb force-pushed the anon-empty-name branch 2 times, most recently from bcf1869 to b9b1323 Compare June 4, 2019 20:46
@ljharb
Copy link
Member

ljharb commented Jun 4, 2019

Rebased this, and on top of #1569, since i hope to land that first.

spec.html Outdated
@@ -7519,6 +7520,7 @@ <h1>%ThrowTypeError% ( )</h1>
</emu-alg>
<p>The value of the [[Extensible]] internal slot of a %ThrowTypeError% function is *false*.</p>
<p>The `"length"` property of a %ThrowTypeError% function has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *false* }.</p>
<p>The `name` property of a %ThrowTypeError% function has the attributes { [[Writable]]: *false*, [[Enumerable]]: *false*, [[Configurable]]: *false* }.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Note that with the policy of #1302 we'll want to write

The `"name"` property

here and in a handful of newly-added lines below.

anba added a commit to anba/test262 that referenced this pull request Aug 15, 2019
leobalter pushed a commit to tc39/test262 that referenced this pull request Aug 15, 2019
@ljharb ljharb 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 15, 2019
@ljharb
Copy link
Member

ljharb commented Aug 15, 2019

@anba this needs a rebase, when you have a moment

@ljharb ljharb requested a review from a team August 15, 2019 15:16
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Mar 17, 2020

Do note that both SpiderMonkey and V8 already include the name property as an own property on %ThrowTypeError%.

I haven’t checked other engines, but I expect it to be the same.

@syg
Copy link
Contributor

syg commented Mar 17, 2020

Ah I see, I had missed that we already call %ThrowTypeError% an "anonymous function". In that case option 1 seems fine to me.

@jmdyck
Copy link
Collaborator

jmdyck commented Mar 19, 2020

The original comment says:

91d3ac1

  • Changes the NamedEvaluation operations to no longer forward to Evaluation and splits Evaluation for ClassExpression. This in preparation for the next commit where NamedEvaluation will directly pass the function name to FunctionCreate.

But the PR no longer has the change in which the function name is passed to OrdinaryFunctionCreate, and I don't see any discussion above that would have resulted in its disappearance. So I'm wondering why.

@anba
Copy link
Contributor Author

anba commented Mar 19, 2020

I've changed the PR to no longer modify OrdinaryFunctionCreate, instead the function name is set after OrdinaryFunctionCreate has been called. That way the PR should be less invasive. Any refactoring to invoke SetFunctionName directly within OrdinaryFunctionCreate can happen at a later time, if there's interest in a such a change.

@anba anba requested a review from syg March 19, 2020 20:12
@ljharb ljharb added the editor call to be discussed in the next editor call label Mar 19, 2020
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the change.

@ljharb ljharb changed the title Set "name" property for anonymous functions Normative: Set "name" property for anonymous functions Apr 8, 2020
@ljharb ljharb removed the editor call to be discussed in the next editor call label Apr 8, 2020
spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

LGTM other than comment.

@ljharb ljharb self-assigned this Apr 10, 2020
 - Changed NamedEvaluation to no longer forward to normal Evaluation
 - Set 'name' property for anonymous functions to the empty string
 - Create 'name' property before the 'prototype' property
 - Respecify 'anonymous function' to mean the 'name' property is the empty string instead of being absent
 - Change Evaluation to forward to NamedEvaluation for anonymous functions

Fixes tc39#1049.
@ljharb ljharb merged commit 74a9ef8 into tc39:master Apr 10, 2020
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 23, 2020
PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 24, 2020
PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 30, 2020
PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").
ljharb pushed a commit to jmdyck/ecma262 that referenced this pull request May 6, 2020
Editorial: Reinstate an SDO rule

PR tc39#1933 deleted SDO rules that are handled by the chain production rule,
but it also deleted this one which isn't.
(It has "TV" on the left and "TRV" on the right.)

Editorial: Move an <emu-note> element

PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").

Editorial: Delete <emu-note> in TimeClip clause

PR tc39#1827 (among other things) removed step 4 from the algorithm for TimeClip,
obsoleting the accompanying emu-note that describes "the point of step 4".

Conceivably, the note could be reworded to describe the effect of
'ToInteger' on step 3, but I don't think it'd be worth the bother.

Editorial: Change "Step 2.a" to "Step 2.b" in RepeatMatcher note

PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.

Editorial: Change step 7 to step 6 in SortCompare note

Commit 9c1e076 (2015-10-26) introduced the '?' abbreviation for ReturnIfAbrupt.
This caused the ToString call on step 7 to move to step 6,
but the note that referred to it wasn't updated.

Editorial: Fix typo: "Descritor" -> "Descriptor"

Editorial: Fix typo: "GeneratorObject" -> "generator object"

(There's no such thing as a GeneratorObject.)

Editorial: Delete "as a parameter" after "is present"

(It's the only place in the spec where we use that phrasing.)

Editorial: Change "which" to "that"

... in "{String,Array,Map,Set} methods which return such iterators"

Editorial: Insert a comma in SetDefaultGlobalBindings()

Formerly, it read like "containing" modified "the property",
when it actually modified "the property descriptor".

Editorial: Change "lexical environment" to "Environment Record"

... in FunctionDeclarationInstantiation,
to balance the NOTE in the other arm of the if-else,
and also for consistency with the NOTE at 27.a.

(I should have done this in PR tc39#1697.)

Editorial: Change "step 3" to "step 4" in Note

... that accompanies the NamedEvaluation semantics for
    ArrowFunction : ArrowParameters `=&gt;` ConciseBody

PR tc39#1870 (among other things) inserted a step before the former step 3,
but didn't update the note that referenced it.
ryanhaddad pushed a commit to WebKit/WebKit that referenced this pull request Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=204214

Reviewed by Yusuke Suzuki.

JSTests:

* test262/expectations.yaml:
Mark 20 test cases as passing.

* stress/builtin-function-name.js:
(shouldThrow):
* stress/private-name-as-anonymous-builtin.js:
Fix tests which are no longer accurate.

Source/JavaScriptCore:

Ensure that Function.prototype.name (exists and) is an empty string for various anonymous built-in functions,
following tc39/ecma262#1490.

Specifically:
1. for promises, resolve / reject / executor elements are lacking a name property
2. for proxies, the revocation function is lacking a name property
3. for certain Intl objects, function getters return a bound function named "bound <name>" instead of ""

This change also means that we no longer need the NameVisibility enum or isAnonymousBuiltinFunction logic.

* builtins/PromiseConstructor.js:
* builtins/PromiseOperations.js:
Ensure resolve / reject / executor elements have a name property.
(They were @-named, which resulted in no name property at all.)

* runtime/ProxyRevoke.cpp:
(JSC::ProxyRevoke::create):
(JSC::ProxyRevoke::finishCreation):
* runtime/ProxyRevoke.h:
Ensure revocation functions have a name property.
(NameVisibility existed solely to ensure this *wasn't* the case.)

* runtime/IntlCollatorPrototype.cpp:
(JSC::IntlCollatorPrototypeGetterCompare):
* runtime/IntlDateTimeFormatPrototype.cpp:
(JSC::IntlDateTimeFormatPrototypeGetterFormat):
* runtime/IntlNumberFormatPrototype.cpp:
(JSC::IntlNumberFormatPrototypeGetterFormat):
Give these bound functions an empty name.

* bytecode/UnlinkedFunctionExecutable.h:
* runtime/FunctionExecutable.h:
* runtime/FunctionRareData.cpp:
(JSC::FunctionRareData::create):
(JSC::FunctionRareData::FunctionRareData):
* runtime/FunctionRareData.h:
* runtime/JSFunction.cpp:
(JSC::JSFunction::allocateRareData):
(JSC::JSFunction::allocateAndInitializeRareData):
(JSC::JSFunction::reifyLazyBoundNameIfNeeded):
* runtime/JSFunction.h:
* runtime/JSFunctionInlines.h:
(JSC::JSFunction::hasReifiedName const): Ensure bound anonymous built-in functions can have an empty name.
(JSC::JSFunction::isAnonymousBuiltinFunction const): Deleted.
Get rid of isAnonymousBuiltinFunction logic.

* runtime/ArrayConstructor.cpp:
(JSC::ArrayConstructor::finishCreation):
* runtime/AsyncFunctionConstructor.cpp:
(JSC::AsyncFunctionConstructor::finishCreation):
* runtime/AsyncGeneratorFunctionConstructor.cpp:
(JSC::AsyncGeneratorFunctionConstructor::finishCreation):
* runtime/BigIntConstructor.cpp:
(JSC::BigIntConstructor::finishCreation):
* runtime/BooleanConstructor.cpp:
(JSC::BooleanConstructor::finishCreation):
* runtime/DateConstructor.cpp:
(JSC::DateConstructor::finishCreation):
* runtime/ErrorConstructor.cpp:
(JSC::ErrorConstructor::finishCreation):
* runtime/FunctionConstructor.cpp:
(JSC::FunctionConstructor::finishCreation):
* runtime/FunctionPrototype.cpp:
(JSC::FunctionPrototype::finishCreation):
* runtime/GeneratorFunctionConstructor.cpp:
(JSC::GeneratorFunctionConstructor::finishCreation):
* runtime/InternalFunction.cpp:
(JSC::InternalFunction::finishCreation):
* runtime/InternalFunction.h:
* runtime/IntlCollatorConstructor.cpp:
(JSC::IntlCollatorConstructor::finishCreation):
* runtime/IntlDateTimeFormatConstructor.cpp:
(JSC::IntlDateTimeFormatConstructor::finishCreation):
* runtime/IntlNumberFormatConstructor.cpp:
(JSC::IntlNumberFormatConstructor::finishCreation):
* runtime/IntlPluralRulesConstructor.cpp:
(JSC::IntlPluralRulesConstructor::finishCreation):
* runtime/JSArrayBufferConstructor.cpp:
(JSC::JSGenericArrayBufferConstructor<sharingMode>::finishCreation):
* runtime/JSGenericTypedArrayViewConstructorInlines.h:
(JSC::JSGenericTypedArrayViewConstructor<ViewClass>::finishCreation):
* runtime/JSTypedArrayViewConstructor.cpp:
(JSC::JSTypedArrayViewConstructor::finishCreation):
* runtime/MapConstructor.cpp:
(JSC::MapConstructor::finishCreation):
* runtime/NativeErrorConstructor.cpp:
(JSC::NativeErrorConstructorBase::finishCreation):
* runtime/NullGetterFunction.h:
* runtime/NullSetterFunction.h:
* runtime/NumberConstructor.cpp:
(JSC::NumberConstructor::finishCreation):
* runtime/ObjectConstructor.cpp:
(JSC::ObjectConstructor::finishCreation):
* runtime/ProxyConstructor.cpp:
(JSC::ProxyConstructor::finishCreation):
* runtime/RegExpConstructor.cpp:
(JSC::RegExpConstructor::finishCreation):
* runtime/SetConstructor.cpp:
(JSC::SetConstructor::finishCreation):
* runtime/StringConstructor.cpp:
(JSC::StringConstructor::finishCreation):
* runtime/SymbolConstructor.cpp:
(JSC::SymbolConstructor::finishCreation):
* runtime/WeakMapConstructor.cpp:
(JSC::WeakMapConstructor::finishCreation):
* runtime/WeakObjectRefConstructor.cpp:
(JSC::WeakObjectRefConstructor::finishCreation):
* runtime/WeakSetConstructor.cpp:
(JSC::WeakSetConstructor::finishCreation):
* wasm/js/WebAssemblyCompileErrorConstructor.cpp:
(JSC::WebAssemblyCompileErrorConstructor::finishCreation):
* wasm/js/WebAssemblyInstanceConstructor.cpp:
(JSC::WebAssemblyInstanceConstructor::finishCreation):
* wasm/js/WebAssemblyLinkErrorConstructor.cpp:
(JSC::WebAssemblyLinkErrorConstructor::finishCreation):
* wasm/js/WebAssemblyMemoryConstructor.cpp:
(JSC::WebAssemblyMemoryConstructor::finishCreation):
* wasm/js/WebAssemblyModuleConstructor.cpp:
(JSC::WebAssemblyModuleConstructor::finishCreation):
* wasm/js/WebAssemblyRuntimeErrorConstructor.cpp:
(JSC::WebAssemblyRuntimeErrorConstructor::finishCreation):
* wasm/js/WebAssemblyTableConstructor.cpp:
(JSC::WebAssemblyTableConstructor::finishCreation):
Get rid of NameVisibility enum.


Canonical link: https://commits.webkit.org/217555@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252520 268f45cc-cd09-0410-ab3c-d52691b4dbfc
TimothyGu added a commit to TimothyGu/test262 that referenced this pull request Jun 20, 2021
Since tc39/ecma262#1490, the "length" and "name"
properties of a class are defined before any static methods. This is
tested by tc39#2057, in test/language/computed-property-names of all places.

At the same time, static methods with "name" as the name would overwrite
the original property, but retain the original property enumeration
order. This was not previously tested. In fact, the overwriting behavior
was not tested at all for the "length" property.

This commit mends both holes in test coverage.
TimothyGu added a commit to TimothyGu/test262 that referenced this pull request Jun 20, 2021
Since tc39/ecma262#1490, the "length" and "name"
properties of a class are defined before any static methods. This is
tested by tc39#2057, in test/language/computed-property-names of all places.

At the same time, static methods with "name" as the name would overwrite
the original property, but retain the original property enumeration
order. This was not previously tested. In fact, the overwriting behavior
was not tested at all for the "length" property.

This commit mends both holes in test coverage.
targos pushed a commit to targos/abi-stable-v8 that referenced this pull request Jun 23, 2021
tc39/ecma262#1490 changed the spec so that the
"name" property of a class should be installed after "length" but before
"prototype". This CL adapts accordingly.

After this change, there is now no need for the separate code path to
set the "name" accessor at runtime. Delete the relevant runtime code as
well.

Bug: v8:8771
Change-Id: I8f809b45bf209c899cf5df76d0ebf6d9a45a6d4e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2974772
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75340}
TimothyGu added a commit to TimothyGu/test262 that referenced this pull request Jul 2, 2021
Since tc39/ecma262#1490, the "length" and "name"
properties of a class are defined before any static methods. This is
tested by tc39#2057, in test/language/computed-property-names of all places.

At the same time, static methods with "name" as the name would overwrite
the original property, but retain the original property enumeration
order. This was not previously tested. In fact, the overwriting behavior
was not tested at all for the "length" property.

This commit mends both holes in test coverage.
rwaldron pushed a commit to tc39/test262 that referenced this pull request Jul 16, 2021
Since tc39/ecma262#1490, the "length" and "name"
properties of a class are defined before any static methods. This is
tested by #2057, in test/language/computed-property-names of all places.

At the same time, static methods with "name" as the name would overwrite
the original property, but retain the original property enumeration
order. This was not previously tested. In fact, the overwriting behavior
was not tested at all for the "length" property.

This commit mends both holes in test coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should anonymous functions get a name data property?