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

Do records (record<>s) purposefully throw on any object with an enumerable Symbol-named property? #294

Open
bzbarsky opened this issue Feb 3, 2017 · 14 comments

Comments

@bzbarsky
Copy link
Collaborator

bzbarsky commented Feb 3, 2017

Walking through https://heycam.github.io/webidl/#es-to-record for an object with an enumerable Symbol-named property, what happens?

  1. [[OwnPropertyKeys]] includes the symbol in the list.
  2. desc.[[Enumerable]] is true.
  3. Converting to the key type calls https://heycam.github.io/webidl/#es-to-DOMString (or one of the other string conversion functions), which calls ToString, which lands in https://tc39.github.io/ecma262/#sec-tostring and throws.

Is this by-design?

// cc @jyasskin @domenic @tobie @heycam @annevk

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Feb 3, 2017

Oh, and as a data point, Gecko current equivalent (MozMap) skips Symbol-named props instead.

@bzbarsky bzbarsky changed the title Do records purposefully throw on any object with an enumerable Symbol-named property? Do records (record<>s) purposefully throw on any object with an enumerable Symbol-named property? Feb 3, 2017
@jyasskin
Copy link
Member

jyasskin commented Feb 3, 2017

I didn't consider the case of Symbol-named properties. The existing implementation in Mozilla is a good argument to allow-and-ignore Symbol-named properties. On the other hand, we meant folks to create the ES object specifically to pass into the record<> argument (evidence: we didn't search the prototype chain), and accidental Symbols are unlikely in that case, which argues for diagnosing them.

@domenic
Copy link
Member

domenic commented May 11, 2017

Let's call this by-design :)

@domenic domenic closed this as completed May 11, 2017
@bzbarsky
Copy link
Collaborator Author

@domenic Do we have tests for this by-design behavior?

@domenic
Copy link
Member

domenic commented May 18, 2017

Probably not... let's reopen until we can be sure.

@domenic domenic reopened this May 18, 2017
@bzbarsky
Copy link
Collaborator Author

I'm going to write some tests in https://bugzilla.mozilla.org/show_bug.cgi?id=1366032 fwiw.

@bzbarsky
Copy link
Collaborator Author

But note that right now no browsers throw here, so it could be a compat risk.... We'll see.

@bzbarsky
Copy link
Collaborator Author

In particular, Chrome's handling of this right now seems to match Firefox's: the symbol-named props are ignored.

@bzbarsky
Copy link
Collaborator Author

And Safari's matches in terms of not throwing, though not some of the other MOP details.

@rakuco
Copy link
Member

rakuco commented May 19, 2017

In particular, Chrome's handling of this right now seems to match Firefox's: the symbol-named props are ignored.

I purposefully followed Gecko and what was discussed here when adding support for records in Blink. In any case, record support landed in M59, which is still in beta so I guess it's still easier to change the behavior if necessary.

@rakuco
Copy link
Member

rakuco commented May 19, 2017

I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=724481 to keep track of this in Blink.

@yuki3
Copy link

yuki3 commented May 22, 2017

Is this consistent with ECMAScript? I'm not an expert, but neither of 7.3.21EnumerableOwnProperties nor 13.7.5.15EnumerateObjectProperties seem throwing an exception. Probably I should look at different places. I'm just wondering if it's consistent with ECMAScript or not, and if not, we can update the spec instead of the implementations.

@domenic
Copy link
Member

domenic commented May 22, 2017

The proper thing to compare with is the public built-in ECMAScript APIs, not the internal spec operations that they use. The only case that seems to accept an arbitrary object with user-supplied keys is Object.defineProperties (and Object.create(), which reuses the same logic). I suppose maybe Object.assign as well.

However, I'm not sure how helpful these comparisons are, because in both cases symbols are a reasonable value. There's no conversion performed, whereas with record<DOMString, x>, or for that matter record<ByteString, x>, we explicitly need to ensure that all the keys are DOMStrings (or ByteStrings).

We have a choice as to how we do this: do we ignore things that do not fit the preferred type, or do we throw? I think throwing is most consistent with other parts of Web IDL. Besides, for the record<ByteString, x> case, it would be very weird to ignore keys that could not be converted by a ByteString (e.g. keys with non-ASCII characters); throwing makes sense there. And so I think it also makes sense for keys, like symbols, that cannot be converted to DOMStrings.

@yuki3
Copy link

yuki3 commented May 23, 2017

Thanks for the explanation. Makes sense.

MXEBot pushed a commit to mirror/chromium that referenced this issue May 24, 2017
We were previously skipping all symbols when calling GetOwnPropertyNames()
due to whatwg/webidl#294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2900743002
Cr-Commit-Position: refs/heads/master@{#473905}
jeffcarp pushed a commit to web-platform-tests/wpt that referenced this issue May 24, 2017
We were previously skipping all symbols when calling GetOwnPropertyNames()
due to whatwg/webidl#294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2900743002
Cr-Commit-Position: refs/heads/master@{#473905}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 24, 2017
We were previously skipping all symbols when calling GetOwnPropertyNames()
due to whatwg/webidl#294 being unresolved at the
time the code was written, as well as to maintain compatibility with Gecko,
which did the same at the time.

That GitHub issue has since been resolved: enumerable symbols should not be
skipped but rather just throw a TypeError when a string conversion function
is called on it. Gecko is also being updated, and this CL adjusts our code
accordingly.

The new tests being added to headers-record.html come from Boris Zbarsky
<bzbarsky@mit.edu> and were originally written in
https://bugzilla.mozilla.org/show_bug.cgi?id=1366032. They are being
included here mostly to both help test the CL, as I believe they will be
landed in Gecko and synced to web-platform-tests before this CL lands and we
end up upstreaming the changes.

While here, remove a duplicate test case in NativeValueTraitsImplTest.

BUG=724481
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2900743002
Cr-Commit-Position: refs/heads/master@{#473905}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants