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

Allow Value.set_field to throw #1329

Merged
merged 1 commit into from
Jun 18, 2021
Merged

Conversation

raskad
Copy link
Member

@raskad raskad commented Jun 16, 2021

While working on some RegExp functions I noticed, that some tests currently fail because Value.set_field always silently no-ops if the property is not writable. For example match sets the lastIndex property multiple times (6.c and 6.f.iii.3.c). It uses the abstract 7.3.4 Set ( O, P, V, Throw ) operation that takes a boolean argument throw and should throw if the property is not writable.

I adjusted the Value.set_field function to make it somewhat similar to 7.3.4 Set ( O, P, V, Throw ). I adjusted all uses of the function to match the spec, if it could be referenced. For uses without clear spec reference, I just set throw to false, so that the current behavior is not impacted.

I'm not sure if changing Value.set_field is the best way forward, but I think an internal function that matches the implementation of 7.3.4 Set ( O, P, V, Throw ) is needed.

@Razican
Copy link
Member

Razican commented Jun 16, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,897 78,897 0
Passed 26,865 26,871 +6
Ignored 15,628 15,628 0
Failed 36,404 36,398 -6
Panics 0 0 0
Conformance 34.05% 34.06% +0.01%
Fixed tests:
test/built-ins/RegExp/prototype/exec/y-fail-lastindex-no-write.js [strict mode] (previously Failed)
test/built-ins/RegExp/prototype/exec/y-fail-lastindex-no-write.js (previously Failed)
test/built-ins/RegExp/prototype/Symbol.search/set-lastindex-init-err.js [strict mode] (previously Failed)
test/built-ins/RegExp/prototype/Symbol.search/set-lastindex-init-err.js (previously Failed)
test/built-ins/RegExp/prototype/test/y-fail-lastindex-no-write.js [strict mode] (previously Failed)
test/built-ins/RegExp/prototype/test/y-fail-lastindex-no-write.js (previously Failed)

@Razican Razican added bug Something isn't working execution Issues or PRs related to code execution labels Jun 16, 2021
@Razican Razican added this to the v0.13.0 milestone Jun 16, 2021
Copy link
Contributor

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

Seems right to me, I think most of this will change with separation of Value and Object, but it's still nice to have this to fix the tests while that isn't done.

@Razican Razican changed the title Allow Value.set_field to thow Allow Value.set_field to throw Jun 17, 2021
@Razican Razican merged commit 8963938 into boa-dev:master Jun 18, 2021
@raskad raskad deleted the set-field-throw branch August 14, 2021 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants