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

Adjust EnumerableOwnPropertyNames to use all String type property keys #1502

Merged
merged 3 commits into from
Aug 24, 2021

Conversation

raskad
Copy link
Member

@raskad raskad commented Aug 23, 2021

This Pull Request fixes/closes #1501.

It changes the following:

  • Adjust EnumerableOwnPropertyNames to use all String type property keys

This looks different from the spec, because the meaning of String in 4.a: If Type(key) is String, then is different than our internal PropertyKey enum types String, Symbol and Index. Our internal Index type is a String type in the context of the spec.
This PR is only a fix for this specific function. We should change our [[OwnPropertyKeys]] to be spec compliant, but I think we should wait with that until #1354 is merged, because [[OwnPropertyKeys]] is defined multiple times for different object types.

@raskad
Copy link
Member Author

raskad commented Aug 23, 2021

The broken test is actually broken and should not have passed before ;)

Test262 conformance changes:

Test result master count PR count difference
Total 78,897 78,897 0
Passed 31,283 31,287 +4
Ignored 15,612 15,612 0
Failed 32,002 31,998 -4
Panics 2 2 0
Conformance 39.65% 39.66% +0.01%
Fixed tests:
test/built-ins/Object/keys/15.2.3.14-5-14.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-5-14.js (previously Failed)
test/built-ins/Object/keys/15.2.3.14-3-4.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-3-4.js (previously Failed)
test/built-ins/Object/keys/15.2.3.14-3-3.js [strict mode] (previously Failed)
test/built-ins/Object/keys/15.2.3.14-3-3.js (previously Failed)
Broken tests:
test/built-ins/Object/keys/15.2.3.14-5-11.js [strict mode] (previously Passed)
test/built-ins/Object/keys/15.2.3.14-5-11.js (previously Passed)

boa/src/object/internal_methods.rs Outdated Show resolved Hide resolved
boa/src/object/internal_methods.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added bug Something isn't working execution Issues or PRs related to code execution labels Aug 23, 2021
@HalidOdat HalidOdat added this to the v0.13.0 milestone Aug 23, 2021
@raskad raskad force-pushed the enumerate-prop-names-index branch from dd02a18 to 8dd6846 Compare August 23, 2021 18:57
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks very good, nice catch!

@Razican Razican merged commit 0f66054 into boa-dev:master Aug 24, 2021
@raskad raskad deleted the enumerate-prop-names-index branch September 10, 2021 23:23
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.

Object.keys does not work with index property keys
3 participants