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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Fix PropertyKey index parse #2843

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Fix PropertyKey index parse #2843

wants to merge 2 commits into from

Conversation

HalidOdat
Copy link
Member

Fixes incorrect parsing of index property keys, such as:

  • "+0" is converted to an integer index 0, should be a string
  • "00" is converted to an integer index 0, should be a string
  • "01" is converted to an integer index 1, should be a string

@HalidOdat HalidOdat added bug Something isn't working execution Issues or PRs related to code execution labels Apr 19, 2023
@HalidOdat HalidOdat added this to the v0.17.0 milestone Apr 19, 2023
@github-actions
Copy link

github-actions bot commented Apr 19, 2023 •
edited
Loading

Test262 conformance changes

Test result main count PR count difference
Total 94,591 94,591 0
Passed 71,835 71,859 +24
Ignored 17,530 17,530 0
Failed 5,226 5,202 -24
Panics 0 0 0
Conformance 75.94% 75.97% +0.03%
Fixed tests (24): 
test/built-ins/TypedArrayConstructors/internals/Set/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Set/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Set/BigInt/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Set/BigInt/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/HasProperty/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/HasProperty/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/HasProperty/BigInt/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/HasProperty/BigInt/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/DefineOwnProperty/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/DefineOwnProperty/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/DefineOwnProperty/BigInt/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/DefineOwnProperty/BigInt/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Get/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Get/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Get/BigInt/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Get/BigInt/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Delete/key-is-not-canonical-index-strict.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Delete/key-is-not-canonical-index-non-strict.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Delete/BigInt/key-is-not-canonical-index-strict.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/Delete/BigInt/key-is-not-canonical-index-non-strict.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/GetOwnProperty/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/GetOwnProperty/key-is-not-canonical-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/GetOwnProperty/BigInt/key-is-not-canonical-index.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/GetOwnProperty/BigInt/key-is-not-canonical-index.js (previously Failed)

@codecov
Copy link

codecov bot commented Apr 19, 2023 •
edited
Loading

Codecov Report

Merging #2843 (f7fe0b9) into main (65f9105) will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main    #2843      +/-   ##
==========================================
+ Coverage   51.11%   51.13%   +0.01%     
==========================================
  Files         419      419              
  Lines       41609    41632      +23     
==========================================
+ Hits        21269    21287      +18     
- Misses      20340    20345       +5
Impacted Files Coverage Δ
boa_engine/src/property/mod.rs 83.26% <83.33%> (-0.07%) ⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@HalidOdat HalidOdat added the run-benchmark Label used to run banchmarks on PRs label Apr 20, 2023
Razican
Razican approved these changes Apr 20, 2023
View reviewed changes
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 great to me!

@HalidOdat HalidOdat force-pushed the fix/property-key-index-parse branch from ecdfdd0 to f7fe0b9 Compare April 20, 2023 08:22
@github-actions
Copy link

Benchmark for c4093a1

Click to view benchmark
Test Base PR %
Arithmetic operations (Compiler) 564.6±1.08ns 554.2±0.97ns -1.84%
Arithmetic operations (Execution) 458.7±0.18ns 459.9±0.29ns +0.26%
Arithmetic operations (Parser) 7.3±0.01µs 7.2±0.01µs -1.37%
Array access (Compiler) 1703.7±2.15ns 1646.6±1.76ns -3.35%
Array access (Execution) 7.1±0.01µs 7.1±0.01µs 0.00%
Array access (Parser) 15.0±0.02µs 15.2±0.02µs +1.33%
Array creation (Compiler) 2.4±0.01µs 2.4±0.00µs 0.00%
Array creation (Execution) 1005.3±3.08µs 963.5±2.47µs -4.16%
Array creation (Parser) 18.3±0.03µs 18.2±0.03µs -0.55%
Array pop (Compiler) 4.3±0.01µs 4.3±0.01µs 0.00%
Array pop (Execution) 579.9±2.61µs 578.7±10.16µs -0.21%
Array pop (Parser) 154.4±0.23µs 154.5±0.10µs +0.06%
Boolean Object Access (Compiler) 1223.7±1.99ns 1228.8±2.78ns +0.42%
Boolean Object Access (Execution) 4.8±0.01µs 4.6±0.02µs -4.17%
Boolean Object Access (Parser) 18.7±0.05µs 18.6±0.08µs -0.53%
Clean js (Compiler) 4.9±0.01µs 4.9±0.01µs 0.00%
Clean js (Execution) 754.1±1.84µs 727.2±2.86µs -3.57%
Clean js (Parser) 37.7±0.04µs 37.9±0.05µs +0.53%
Create Realm 574.1±6.58µs 517.5±1.60µs -9.86%
Dynamic Object Property Access (Compiler) 2.0±0.00µs 1975.1±2.81ns -1.24%
Dynamic Object Property Access (Execution) 4.5±0.01µs 4.4±0.01µs -2.22%
Dynamic Object Property Access (Parser) 13.5±0.02µs 13.5±0.02µs 0.00%
Fibonacci (Compiler) 3.1±0.00µs 3.0±0.01µs -3.23%
Fibonacci (Execution) 1031.5±2.98µs 1030.9±2.73µs -0.06%
Fibonacci (Parser) 21.3±0.02µs 21.5±0.71µs +0.94%
For loop (Compiler) 2.7±0.01µs 2.7±0.02µs 0.00%
For loop (Execution) 15.3±0.03µs 15.2±0.02µs -0.65%
For loop (Parser) 18.6±0.04µs 18.6±0.19µs 0.00%
Mini js (Compiler) 4.4±0.01µs 4.3±0.01µs -2.27%
Mini js (Execution) 707.8±3.72µs 676.5±2.38µs -4.42%
Mini js (Parser) 33.1±0.03µs 33.1±0.05µs 0.00%
Number Object Access (Compiler) 1145.3±2.76ns 1144.2±1.85ns -0.10%
Number Object Access (Execution) 3.7±0.01µs 3.6±0.02µs -2.70%
Number Object Access (Parser) 14.2±0.03µs 14.1±0.02µs -0.70%
Object Creation (Compiler) 1802.4±5.81ns 1778.8±4.57ns -1.31%
Object Creation (Execution) 4.3±0.01µs 4.2±0.01µs -2.33%
Object Creation (Parser) 11.8±0.02µs 11.9±0.01µs +0.85%
RegExp (Compiler) 2.0±0.00µs 2.0±0.00µs 0.00%
RegExp (Execution) 12.7±0.05µs 12.3±0.03µs -3.15%
RegExp (Parser) 13.0±0.02µs 13.0±0.02µs 0.00%
RegExp Creation (Compiler) 1782.3±3.50ns 1779.9±4.57ns -0.13%
RegExp Creation (Execution) 9.0±0.02µs 8.5±0.02µs -5.56%
RegExp Creation (Parser) 11.0±0.01µs 11.1±0.02µs +0.91%
RegExp Literal (Compiler) 2.0±0.01µs 2.0±0.01µs 0.00%
RegExp Literal (Execution) 12.8±0.04µs 12.3±0.04µs -3.91%
RegExp Literal (Parser) 14.2±0.03µs 14.0±0.03µs -1.41%
RegExp Literal Creation (Compiler) 1797.9±3.58ns 1776.8±11.09ns -1.17%
RegExp Literal Creation (Execution) 9.0±0.02µs 8.5±0.02µs -5.56%
RegExp Literal Creation (Parser) 12.0±0.03µs 11.9±0.01µs -0.83%
Static Object Property Access (Compiler) 1835.6±5.24ns 1804.7±3.99ns -1.68%
Static Object Property Access (Execution) 4.4±0.02µs 4.4±0.01µs 0.00%
Static Object Property Access (Parser) 12.7±0.02µs 12.8±0.03µs +0.79%
String Object Access (Compiler) 1545.9±12.63ns 1592.0±6.41ns +2.98%
String Object Access (Execution) 6.3±0.01µs 6.2±0.03µs -1.59%
String Object Access (Parser) 17.9±0.04µs 17.9±0.02µs 0.00%
String comparison (Compiler) 2.5±0.00µs 2.5±0.00µs 0.00%
String comparison (Execution) 4.0±0.03µs 3.9±0.01µs -2.50%
String comparison (Parser) 14.7±0.01µs 14.8±0.05µs +0.68%
String concatenation (Compiler) 2.0±0.00µs 2.0±0.00µs 0.00%
String concatenation (Execution) 3.8±0.03µs 3.7±0.01µs -2.63%
String concatenation (Parser) 10.1±0.17µs 10.2±0.02µs +0.99%
String copy (Compiler) 1671.0±2.81ns 1677.2±3.01ns +0.37%
String copy (Execution) 3.5±0.01µs 3.5±0.01µs 0.00%
String copy (Parser) 7.5±0.01µs 7.6±0.01µs +1.33%
Symbols (Compiler) 1227.4±2.02ns 1209.8±2.07ns -1.43%
Symbols (Execution) 3.6±0.01µs 3.6±0.01µs 0.00%
Symbols (Parser) 5.9±0.03µs 5.9±0.01µs 0.00%

@HalidOdat HalidOdat marked this pull request as ready for review April 21, 2023 00:32
Razican
Razican approved these changes Apr 23, 2023
View reviewed changes
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.

LGTM!

@Razican
Copy link
Member

Razican commented Apr 23, 2023

bors r+

bors bot pushed a commit that referenced this pull request Apr 23, 2023
@HalidOdat
Fix PropertyKey index parse (#2843)
9159f90 
Fixes incorrect parsing of index property keys, such as:

- `"+0"` is converted to an integer index `0`, should be a string
- `"00"` is converted to an integer index `0`, should be a string
- `"01"` is converted to an integer index `1`, should be a string
@bors
Copy link

bors bot commented Apr 23, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix PropertyKey index parse [Merged by Bors] - Fix PropertyKey index parse Apr 23, 2023
@bors bors bot closed this Apr 23, 2023
@bors bors bot deleted the fix/property-key-index-parse branch April 23, 2023 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Razican Razican Razican approved these changes

@raskad raskad raskad approved these changes

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@RageKnify RageKnify Awaiting requested review from RageKnify

@jedel1043 jedel1043 Awaiting requested review from jedel1043

@nekevss nekevss Awaiting requested review from nekevss

Assignees
No one assigned
Labels
bug Something isn't working execution Issues or PRs related to code execution run-benchmark Label used to run banchmarks on PRs
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@HalidOdat @Razican @raskad