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: Match ECMA‑262 function property enumeration order #914

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

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Sep 3, 2020

This makes the order of the "length", "name" and "prototype" properties on WebIDL function objects match web reality (at least Firefox’s behaviour) and ECMA‑262 as of tc39/ecma262#1490 and tc39/ecma262#2116, which also changed CreateBuiltinFunction so that it takes the additional length and name required parameters.


(See WHATWG Working Mode: Changes for more details.)


review?(@annevk, @domenic, @littledan, @ljharb)

Fixes #1106


Preview | Diff

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 19, 2021

tc39/ecma262#2116 has been merged, so this PR has been updated to pass the length and name parameters to CreateBuiltinFunction directly.

@annevk
Copy link
Member

annevk commented Feb 19, 2021

Great work @ExE-Boss! I'll leave this to others to review, but this looks like a really nice cleanup. Do we have some tests for this as well?

Copy link
Member

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

Looks good as far as the spec goes, but each of these calls needs to be tested. The values are probably mostly covered by idlharness, but the order of iteration probably isn't.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Just wondering @ExE-Boss, have you had any progress on writing tests for this PR?

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
1. Perform [=!=] [$CreateDataProperty$](|handler|, "<code>preventExtensions</code>", |preventExtensions|).
1. Let |set| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-set]], « », |realm|).
1. Let |set| be [=!=] [$CreateBuiltinFunction$](the steps from [[#es-observable-array-set]], the number of non-optional parameters from [[#es-observable-array-set]], "<code>set</code>", « », |realm|).
1. Perform [=!=] [$CreateDataProperty$](|handler|, "<code>set</code>", |set|).
Copy link
Member

Choose a reason for hiding this comment

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

For readability, may I suggest a tabular format like this:

  1. For each row in the following table:

    Name Section Length
    "defineProperty" § 3.10.1 defineProperty 3
    "deleteProperty" § 3.10.2 deleteProperty 2
    1. Let (name, section, length) be the entries of the current row.
    2. Let F be ! CreateBuiltinFunction(the steps from section, length, name, « », realm).
    3. Perform ! CreateDataProperty(handler, name, F).

This helps make length changes more visible to implementors and folks writing tests, and also seems to be generally more readable.

Copy link
Member

Choose a reason for hiding this comment

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

@domenic could you take a look at this part of the PR if have some time?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I kind of like the way the PR currently does it for lengths, in that it doesn't require syncing the length column with the steps like your proposal would.

Since the proxy handlers are an unobservable internal implementation detail anyway, I think we could just set the length to 0 and the name to "" for all of them, with a <p class="note"> explaining?

The table/loop format would improve the section in general, but I'm not sure it needs to be done in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

@TimothyGu should we merge as-is? Or go for the 0/"" solution maybe?

@TimothyGu TimothyGu added the needs tests Moving the issue forward requires someone to write tests label Jun 30, 2021
@ExE-Boss ExE-Boss marked this pull request as draft August 12, 2021 09:24
@ExE-Boss ExE-Boss marked this pull request as ready for review September 3, 2021 13:44
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Sep 3, 2021

Tests are now written in web-platform-tests/wpt#30333.

@domenic
Copy link
Member

domenic commented Sep 3, 2021

Based on web-platform-tests/wpt#30333 it looks like this matches Firefox but mismatches Chrome and Safari (for constructors). So we should make sure at least one of those browsers is interested in changing. @yuki3 @shvaikalesh what do you think?

@bakkot
Copy link
Contributor

bakkot commented Sep 3, 2021

Reflect.ownKeys(Blob):

  • Chrome: ["length", "name", "arguments", "caller", "prototype"]
  • Firefox: [ "length", "name", "prototype", Symbol("Symbol.hasInstance") ]
  • Safari: ["prototype", "name", "length"]

So, Chrome has additional arguments and caller properties, which surprises me. (They appear to be non-functional, at least - (function f(){ new Blob([], { get type(){ console.log(Blob.caller, Blob.arguments) } }) })() prints null twice.)

But Chrome does match the enumeration order for the length and name properties, which I think is the only change actually implied by this PR. In particular, ecma262 specifically refrains from specifying the full enumeration order, including of prototype; it only specifies that length appears before name. (The test262 tests for the corresponding ecma262 PR are much more permissive than the WPT linked above.)

So, possibly the WPT is just over-zealous for the actual change implied by this PR, and could be changed to match the test262 tests. Then both Chrome and Firefox would pass.

@domenic
Copy link
Member

domenic commented Sep 3, 2021

@bakkot to check my understanding, it seems like the correct test would be that name appears right after length, right? So the test262 test is not as fully strict as it could be.

I think Web IDL, being more imperative than the ES spec, actually gives a full ordering between length/name/prototype that matches the tests written. But in terms of ensuring implementer interest for this particular change, I agree we're probably OK.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Sep 3, 2021

it seems like the correct test would be that name appears right after length, right? So the test262 test is not as fully strict as it could be.

Actually, the test262 test checks exactly that using: nameIndex === lengthIndex + 1

assert(lengthIndex >= 0 && nameIndex === lengthIndex + 1,
	"The `length` property comes before the `name` property on built-in functions");

@domenic
Copy link
Member

domenic commented Sep 3, 2021

Sorry, I totally skimmed the test and misread it. My bad.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Sep 3, 2021

@bakkot

So, possibly the WPT is just over-zealous for the actual change implied by this PR, and could be changed to match the test262 tests. Then both Chrome and Firefox would pass.

With this PR, the prototype property is the first property that’s added to constructors after the CreateBuiltinFunction call in create an interface object.

@bakkot
Copy link
Contributor

bakkot commented Sep 3, 2021

it seems like the correct test would be that name appears right after length, right?

Right.

I think Web IDL, being more imperative than the ES spec, actually gives a full ordering between length/name/prototype that matches the tests written.

Ah, so it does.

@shvaikalesh
Copy link
Contributor

I like this change, both editorial-wise and the way it improves enumeration order consistency. Please use this bug to follow WebKit implementation.

As for caller and arguments, there is a proposal to move them to Function.prototype, which was recently implemented in WebKit to match Firefox.

@yuki3
Copy link

yuki3 commented Sep 22, 2021

Based on web-platform-tests/wpt#30333 it looks like this matches Firefox but mismatches Chrome and Safari (for constructors). So we should make sure at least one of those browsers is interested in changing. @yuki3 @shvaikalesh what do you think?

I'm very sorry that I somehow overlooked the mention. I'm fine with the change to make the order of property enumeration consistent. However, I'm afraid that I cannot help much on this. IIUC, the order of these standard(ish?) properties on function objects is controlled by V8. We'd better ask V8 team to make the change. @verwaest, any thoughts?

@verwaest
Copy link

The proposed changes sound alright. Making this more uniform makes sense.

It seems like we're invalidly adding special accessors for caller/arguments but will always return null for native functions. We can change that too.

As for moving caller/arguments to the prototype chain: Currently v8 returns the caller of the "holder" on which the property is installed. With this I mean that if you put a function with a .caller on the prototype chain of e.g., a sloppy-mode arrow function, we won't return the caller of the arrow function, but the caller of the function on the prototype chain. What are the expected semantics there? (The change itself sounds like an improvement though!)

@shvaikalesh
Copy link
Contributor

shvaikalesh commented Sep 23, 2021

@verwaest The proposal sets up Function.prototype.{arguments,caller} as getters (spec), so they can return null or real caller or even throw a TypeError depending on a holder (receiver).

One important bit of the proposal I kinda hope V8 would implement is returning null for (async) generators to match the JSC: we can't return real caller for them and it seems web-compatible.

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Sep 29, 2021
…erparts

https://bugs.webkit.org/show_bug.cgi?id=230584

Reviewed by Alex Christensen.

LayoutTests/imported/w3c:

This is being upstreamed at web-platform-tests/wpt#30333.

* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any-expected.txt: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.html: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.js: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker-expected.txt: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker.html: Added.

Source/WebCore:

This patch implements spec proposal [1] on matching property order of DOM constructors
with ECMA-262 functions: "length", "name", "prototype". Aligns WebKit with Blink and Gecko.
Also, groups property puts to remove 2 extra `$interface->isNamespaceObject` checks.

No behavior change except for enumeration order.

[1] whatwg/webidl#914

Tests: imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.html
       imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker.html

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateConstructorHelperMethods):
* bindings/scripts/test/JS/*: Updated.


Canonical link: https://commits.webkit.org/242275@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283233 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Cwiiis pushed a commit to Cwiiis/webkit-deprecated that referenced this pull request Oct 4, 2021
…erparts

https://bugs.webkit.org/show_bug.cgi?id=230584

Reviewed by Alex Christensen.

LayoutTests/imported/w3c:

This is being upstreamed at web-platform-tests/wpt#30333.

* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any-expected.txt: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.html: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.js: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker-expected.txt: Added.
* web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker.html: Added.

Source/WebCore:

This patch implements spec proposal [1] on matching property order of DOM constructors
with ECMA-262 functions: "length", "name", "prototype". Aligns WebKit with Blink and Gecko.
Also, groups property puts to remove 2 extra `$interface->isNamespaceObject` checks.

No behavior change except for enumeration order.

[1] whatwg/webidl#914

Tests: imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.html
       imported/w3c/web-platform-tests/WebIDL/ecmascript-binding/builtin-function-properties.any.worker.html

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateConstructorHelperMethods):
* bindings/scripts/test/JS/*: Updated.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283233 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Oct 8, 2021

I’ve filed bug 1257969 to get this fixed in Chrome as well.

@ExE-Boss
Copy link
Contributor Author

The “needs tests” label is wrong, as web-platform-tests/wpt#30333 has been merged.

@Ms2ger
Copy link
Member

Ms2ger commented Apr 24, 2023

Argh, apologies for merging the tests ahead of time. Would be good to get this landed anyway, though.

@annevk
Copy link
Member

annevk commented Apr 24, 2023

That's blocked on one of two things:

  1. @ExE-Boss updates https://github.com/whatwg/participant-data/blob/main/individuals.json with their real name. (See Can contributors use pseudonyms to sign the participation agreement? sg#93 (comment) for context.)
  2. Someone else makes this contribution in a new PR.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 24, 2024
…ry functions.

Implements whatwg/webidl#914 for legacy factory functions.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629803 is the bug that would fix
this in SpiderMonkey, working around that for now.

Differential Revision: https://phabricator.services.mozilla.com/D205805

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1887721
gecko-commit: 0594801ef8e281f5be66076d27a72c5c8c9496e9
gecko-reviewers: saschanaz
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 25, 2024
…n legacy factory functions. r=saschanaz

Implements whatwg/webidl#914 for legacy factory functions.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629803 is the bug that would fix
this in SpiderMonkey, working around that for now.

Differential Revision: https://phabricator.services.mozilla.com/D205805
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 25, 2024
…ry functions.

Implements whatwg/webidl#914 for legacy factory functions.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629803 is the bug that would fix
this in SpiderMonkey, working around that for now.

Differential Revision: https://phabricator.services.mozilla.com/D205805

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1887721
gecko-commit: 0594801ef8e281f5be66076d27a72c5c8c9496e9
gecko-reviewers: saschanaz
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 25, 2024
…n legacy factory functions. r=saschanaz

Implements whatwg/webidl#914 for legacy factory functions.
https://bugzilla.mozilla.org/show_bug.cgi?id=1629803 is the bug that would fix
this in SpiderMonkey, working around that for now.

Differential Revision: https://phabricator.services.mozilla.com/D205805
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests
Development

Successfully merging this pull request may close these issues.

CreateBuiltinFunction’s signature has changed
9 participants