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

fix #780 fix Object.assign when undefined value and inextensible #1186

Merged
merged 2 commits into from
Mar 5, 2022

Conversation

tuchida
Copy link
Contributor

@tuchida tuchida commented Feb 19, 2022

ref. #780

Object.keys(Object.assign({a:undefined}, {b:undefined})).join()
// expected:<a[,b]> but was:<a[]>

I found another problem with it, so I fixed it while I was at it.

var obj = Object.freeze({});
Object.assign(obj, { a: 1 });
// An error to be thrown, but nothing was thrown.

@p-bakker
Copy link
Collaborator

Haven't looked at the changes, but haven't these fixes improved the test262 results?

@tuchida
Copy link
Contributor Author

tuchida commented Feb 19, 2022

The undefined test does not seem to be in test262.
The Object.freeze test also does not seem to be in the one used by Rhino. The latest version has it, but it requires the implementation of Computed Property Names.

@p-bakker
Copy link
Collaborator

Right...

The latest version has it, but it requires the implementation of Computed Property Names.

Not just Computed Property Names though. Also at least a proper const impl and unicode regex

*
* <p>https://262.ecma-international.org/12.0/#sec-set-o-p-v-throw
*/
public static void putOrThrow(Context cx, Scriptable obj, String name, Object value) {
Copy link
Collaborator

@p-bakker p-bakker Feb 19, 2022

Choose a reason for hiding this comment

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

Abstract Object Operations as defined in the ecmascript spec ought to be placed in AbstractEcmaObjectOperations.

Please heck the guidelines at the top of the file for some instructions on how to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref. 204cd5f
I made to move to AbstractEcmaObjectOperations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rebased it.

@gbrail
Copy link
Collaborator

gbrail commented Mar 5, 2022

Yes -- this looks good. Thanks!

@gbrail gbrail merged commit c275c35 into mozilla:master Mar 5, 2022
rbri added a commit to rbri/rhino that referenced this pull request Mar 20, 2022
@rbri rbri mentioned this pull request Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants