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 for #616 #617

Merged
merged 2 commits into from Nov 25, 2019
Merged

Fix for #616 #617

merged 2 commits into from Nov 25, 2019

Conversation

stijnkliemesch
Copy link
Contributor

Affects strict mode only.

No longer prematurely throws a TypeError in ScriptableObject.putImpl(Object,int,Scriptable,Object) when instance member isExtensible is false.

Instead, now it first queries the slotmap, so that if a GetterSlot instance is returned, any present setter may be called. All other outcomes of this method should be unaffected.

Affects strict mode only.

No longer prematurely throws a TypeError in ScriptableObject.putImpl(Object,int,Scriptable,Object) when instance member isExtensible is false.

Instead, now it first queries the slotmap, so that if a GetterSlot instance is returned, any present setter may be called. All other outcomes of this method should be unaffected.
@stijnkliemesch
Copy link
Contributor Author

stijnkliemesch commented Nov 8, 2019

Do I add the testcase as a new .js file somewhere in test262/test/built-ins/Object/ like in freeze/ or preventExtensions/ ?

@stijnkliemesch
Copy link
Contributor Author

Also, after this commit, the tests show the following output:

org.mozilla.javascript.tests.Test262SuiteTest STANDARD_OUT
    Test is marked as failing but it does not: test262/test/built-ins/Object/freeze/15.2.3.9-2-c-3.js
    Test is marked as failing but it does not: test262/test/built-ins/Object/freeze/15.2.3.9-2-c-2.js
    Test is marked as failing but it does not: test262/test/built-ins/Object/freeze/15.2.3.9-2-c-4.js

Yet the tests seem to "succeed" despite this. Is this a blocking issue?

@gbrail
Copy link
Collaborator

gbrail commented Nov 11, 2019

Those messages are from the Test262 driver. Basically, some tests are disabled in the file test262.properties, but for some reason (I don't recall why) they were run anyway and passed.

That's a good thing -- it means that we pass more of the test262 suite now -- and we want to preserve our progress by re-enabling those tests so that we don't regress.

Can you take a look at test262 properties and see if modifying that file makes the warnings go away? If you can't figure out I can probably do it later.

@stijnkliemesch
Copy link
Contributor Author

Do these 3 extra activated Test262 testcases already cover the issue well enough, or is there still a testcase required for showing #616 is fixed ?

Copy link
Collaborator

@rbri rbri left a comment

Choose a reason for hiding this comment

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

Looks god for me - only the curly brackets for the if statements are missing from my point of view.
Have done some simple performance checks on my local machine and i found no negative impact.

@gbrail please merge as soon as possible :-)

@gbrail gbrail merged commit 7c68424 into mozilla:master Nov 25, 2019
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.

None yet

3 participants