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

Cannot use Symbol as key in computed key accessor property: var x = {get [Symbol()](){ /* some code */ }} #1614

Open
p-bakker opened this issue Sep 7, 2024 · 2 comments
Labels
bug Issues considered a bug Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec

Comments

@p-bakker
Copy link
Collaborator

p-bakker commented Sep 7, 2024

See #1610 (comment)

I locally fixed it by patching ScriptRuntime.newObjectLiteral(...) by adding a check for Symbol and following a different codepath:
image

In the id instanceof Symbol branch, I'm calling defineOwnProperty, whereas for all other type of keys it uses setGetterOrSetter, which is tailored to handling just String and int keys and directly modifies the slot map

Using defineOwnProperty however requires to instantiate a descriptor ScriptableObject, which then follows a different path to modify the slot map.

Thoughts on this approach? Feels a bit messy to me, so like to get some other opinions @gbrail @rbri @andreabergia

@p-bakker p-bakker changed the title Cannot use Symbol as key in computed key accessor var x = {get [Symbol()](){ /* some code */ }} Cannot use Symbol as key in computed key accessor property: var x = {get [Symbol()](){ /* some code */ }} Sep 7, 2024
@p-bakker p-bakker added bug Issues considered a bug Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec labels Sep 7, 2024
@p-bakker
Copy link
Collaborator Author

p-bakker commented Sep 7, 2024

Another approach seems to be changing the type of the name param of setGetterOrSetter to Object instead of String: most usage of the name param are calls to methods that expect an Object anyway

@rbri
Copy link
Collaborator

rbri commented Sep 7, 2024

@p-bakker sorry, I'm on vacation now, have my brain switched off 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues considered a bug Ecma Incompatibility Issues about Rhino being incompatible with the EcmaScript spec
Projects
None yet
Development

No branches or pull requests

2 participants