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 Object.defineProperty() #1005

Merged
merged 1 commit into from
Jan 1, 2021
Merged

Fix Object.defineProperty() #1005

merged 1 commit into from
Jan 1, 2021

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Dec 29, 2020

It changes the following:

  • Fix panic if first argument is not supplied.
  • Fix panic if second argument is not supplied.
  • Fix bug when the second argument is not a object.
  • Implemented DefinePropertyOrThrow()

@HalidOdat HalidOdat added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics labels Dec 29, 2020
@HalidOdat HalidOdat added this to the v0.11.0 milestone Dec 29, 2020
@github-actions
Copy link

github-actions bot commented Dec 29, 2020

Test262 conformance changes:

Test result master count PR count difference
Total 78,493 78,493 0
Passed 20,904 20,882 -22
Ignored 15,585 15,585 0
Failed 42,004 42,026 +22
Panics 388 388 0
Conformance 26.63 26.60 -0.03%

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #1005 (ac136fd) into master (4cede75) will increase coverage by 0.02%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
+ Coverage   60.07%   60.10%   +0.02%     
==========================================
  Files         167      167              
  Lines       11132    11142      +10     
==========================================
+ Hits         6688     6697       +9     
- Misses       4444     4445       +1     
Impacted Files Coverage Δ
boa/src/object/gcobject.rs 65.86% <85.71%> (+0.49%) ⬆️
boa/src/builtins/object/mod.rs 67.85% <90.90%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cede75...ac136fd. Read the comment docs.

@github-actions
Copy link

Benchmark for b7894f2

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 304.7±4.92ns 302.3±0.13ns +0.79%
Arithmetic operations (Full) 205.6±0.42µs 206.4±0.27µs -0.39%
Array access (Execution) 7.1±0.05µs 7.1±0.02µs 0.00%
Array access (Full) 223.9±0.49µs 223.4±0.39µs +0.22%
Array creation (Execution) 2.6±0.00ms 2.4±0.00ms +8.33%
Array creation (Full) 2.7±0.00ms 2.7±0.00ms 0.00%
Array pop (Execution) 966.8±2.32µs 882.4±2.71µs +9.56%
Array pop (Full) 1312.5±4.22µs 1301.8±2.20µs +0.82%
Boolean Object Access (Execution) 4.3±0.00µs 4.3±0.01µs 0.00%
Boolean Object Access (Full) 213.2±0.35µs 213.2±0.29µs 0.00%
Clean js (Execution) 638.9±5.32µs 628.3±3.67µs +1.69%
Clean js (Full) 887.0±4.36µs 883.5±4.18µs +0.40%
Clean js (Parser) 31.0±0.12µs 30.9±0.03µs +0.32%
Create Realm 409.5±1.98ns 406.5±4.32ns +0.74%
Dynamic Object Property Access (Execution) 4.9±0.01µs 4.9±0.01µs 0.00%
Dynamic Object Property Access (Full) 220.0±2.08µs 219.2±0.36µs +0.36%
Expression (Parser) 5.8±0.00µs 5.8±0.01µs 0.00%
Fibonacci (Execution) 655.4±1.82µs 738.9±0.76µs -11.30%
Fibonacci (Full) 980.0±1.19µs 979.1±1.04µs +0.09%
For loop (Execution) 19.7±0.08µs 19.9±0.04µs -1.01%
For loop (Full) 238.6±0.60µs 238.1±0.61µs +0.21%
For loop (Parser) 15.1±0.03µs 15.1±0.03µs 0.00%
Goal Symbols (Parser) 10.3±0.02µs 10.2±0.04µs +0.98%
Hello World (Parser) 2.6±0.03µs 2.6±0.03µs 0.00%
Long file (Parser) 694.4±0.37ns 691.3±1.51ns +0.45%
Mini js (Execution) 577.0±5.42µs 570.4±3.82µs +1.16%
Mini js (Full) 805.1±2.60µs 805.9±4.09µs -0.10%
Mini js (Parser) 27.3±0.05µs 27.2±0.02µs +0.37%
Number Object Access (Execution) 3.4±0.00µs 3.3±0.00µs +3.03%
Number Object Access (Full) 211.0±0.98µs 213.9±0.33µs -1.36%
Object Creation (Execution) 4.2±0.01µs 4.2±0.01µs 0.00%
Object Creation (Full) 215.9±0.29µs 216.0±0.35µs -0.05%
RegExp (Execution) 8.3±0.12µs 8.3±0.04µs 0.00%
RegExp (Full) 226.0±0.43µs 226.1±0.42µs -0.04%
RegExp Literal (Execution) 9.3±0.03µs 9.3±0.03µs 0.00%
RegExp Literal (Full) 224.2±0.36µs 224.1±0.28µs +0.04%
RegExp Literal Creation (Execution) 8.3±0.03µs 8.3±0.05µs 0.00%
RegExp Literal Creation (Full) 220.0±0.44µs 218.7±0.35µs +0.59%
Static Object Property Access (Execution) 4.5±0.01µs 4.5±0.01µs 0.00%
Static Object Property Access (Full) 215.7±0.41µs 217.0±0.33µs -0.60%
String Object Access (Execution) 5.9±0.01µs 5.9±0.01µs 0.00%
String Object Access (Full) 218.1±0.47µs 218.9±0.30µs -0.37%
String comparison (Execution) 5.7±0.02µs 5.8±0.02µs -1.72%
String comparison (Full) 219.7±0.40µs 220.0±0.50µs -0.14%
String concatenation (Execution) 4.7±0.08µs 4.6±0.02µs +2.17%
String concatenation (Full) 214.6±0.24µs 214.0±0.24µs +0.28%
String copy (Execution) 3.6±0.01µs 3.6±0.01µs 0.00%
String copy (Full) 209.6±0.40µs 209.9±0.42µs -0.14%
Symbols (Execution) 3.0±0.01µs 3.0±0.00µs 0.00%
Symbols (Full) 205.0±2.82µs 204.8±0.62µs +0.10%

@github-actions
Copy link

Benchmark for d00f633

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 366.7±1.15ns 363.1±0.38ns +0.99%
Arithmetic operations (Full) 244.5±0.34µs 243.9±0.55µs +0.25%
Array access (Execution) 8.4±0.03µs 8.4±0.04µs 0.00%
Array access (Full) 264.1±0.42µs 265.7±0.71µs -0.60%
Array creation (Execution) 3.0±0.01ms 3.1±0.01ms -3.23%
Array creation (Full) 3.3±0.02ms 3.3±0.01ms 0.00%
Array pop (Execution) 1105.3±4.25µs 1130.0±3.75µs -2.19%
Array pop (Full) 1574.3±22.69µs 1568.7±3.52µs +0.36%
Boolean Object Access (Execution) 5.1±0.01µs 5.1±0.01µs 0.00%
Boolean Object Access (Full) 256.8±0.77µs 255.7±0.78µs +0.43%
Clean js (Execution) 760.5±5.91µs 758.4±5.35µs +0.28%
Clean js (Full) 1062.5±7.74µs 1059.8±8.96µs +0.25%
Clean js (Parser) 35.4±0.11µs 35.4±0.22µs 0.00%
Create Realm 488.1±0.76ns 496.5±4.52ns -1.69%
Dynamic Object Property Access (Execution) 5.9±0.02µs 5.9±0.02µs 0.00%
Dynamic Object Property Access (Full) 260.5±0.71µs 260.2±0.65µs +0.12%
Expression (Parser) 6.9±0.01µs 6.9±0.01µs 0.00%
Fibonacci (Execution) 885.6±15.11µs 885.5±11.36µs +0.01%
Fibonacci (Full) 1171.3±1.34µs 1169.0±5.07µs +0.20%
For loop (Execution) 23.1±0.07µs 23.5±0.11µs -1.70%
For loop (Full) 283.4±1.25µs 284.2±0.72µs -0.28%
For loop (Parser) 17.7±0.10µs 17.6±0.08µs +0.57%
Goal Symbols (Parser) 11.8±0.02µs 11.8±0.04µs 0.00%
Hello World (Parser) 3.1±0.01µs 3.1±0.01µs 0.00%
Long file (Parser) 799.9±1.40ns 802.8±1.42ns -0.36%
Mini js (Execution) 680.8±4.17µs 689.4±3.93µs -1.25%
Mini js (Full) 963.6±4.17µs 960.8±4.88µs +0.29%
Mini js (Parser) 31.2±0.06µs 31.2±0.19µs 0.00%
Number Object Access (Execution) 4.0±0.01µs 4.0±0.03µs 0.00%
Number Object Access (Full) 250.5±1.33µs 253.4±0.91µs -1.14%
Object Creation (Execution) 5.0±0.02µs 5.1±0.02µs -1.96%
Object Creation (Full) 255.6±0.43µs 255.8±1.21µs -0.08%
RegExp (Execution) 9.5±0.04µs 9.5±0.05µs 0.00%
RegExp (Full) 266.9±0.47µs 264.8±1.40µs +0.79%
RegExp Literal (Execution) 10.8±0.07µs 10.8±0.06µs 0.00%
RegExp Literal (Full) 264.6±0.74µs 265.6±0.52µs -0.38%
RegExp Literal Creation (Execution) 9.6±0.08µs 9.6±0.04µs 0.00%
RegExp Literal Creation (Full) 260.2±0.87µs 260.0±0.69µs +0.08%
Static Object Property Access (Execution) 5.3±0.02µs 5.3±0.03µs 0.00%
Static Object Property Access (Full) 256.5±0.81µs 256.9±1.34µs -0.16%
String Object Access (Execution) 7.1±0.06µs 7.1±0.02µs 0.00%
String Object Access (Full) 260.0±0.76µs 259.3±0.86µs +0.27%
String comparison (Execution) 6.9±0.04µs 6.8±0.04µs +1.47%
String comparison (Full) 259.6±0.74µs 260.6±0.58µs -0.38%
String concatenation (Execution) 5.6±0.03µs 5.5±0.03µs +1.82%
String concatenation (Full) 254.6±0.62µs 255.2±0.47µs -0.24%
String copy (Execution) 4.3±0.01µs 4.2±0.01µs +2.38%
String copy (Full) 248.3±0.64µs 250.6±0.43µs -0.92%
Symbols (Execution) 3.6±0.01µs 3.6±0.02µs 0.00%
Symbols (Full) 243.0±0.51µs 243.0±0.63µs 0.00%

@RageKnify
Copy link
Member

Somehow 680579a changed 2 test262 tests from passing to panicking, the change can be seen by looking at the edit history of the comment showing the results, #1005 (comment)

Don't understand how that change could introduce panicking.

@HalidOdat
Copy link
Member Author

HalidOdat commented Dec 29, 2020

Somehow 680579a changed 2 test262 tests from passing to panicking, the change can be seen by looking at the edit history of the comment showing the results, #1005 (comment)

Don't understand how that change could introduce panicking.

It is probably due to another unrelated bug. Object.defineProperty should return the object.

@tofpie
Copy link
Contributor

tofpie commented Dec 29, 2020

The new panic is caused by the test 15.2.3.6-4-187.

This test calls Object.defineProperty on the property length of an Array setting writable to false but without defining a value.

As defineProperty now returns the object, it is displayed, and the panic occurs here, as length is undefined:

ObjectData::Array => {
let len = v
.get_own_property(&PropertyKey::from("length"))
// TODO: do this in a better way `unwrap`
.unwrap()
// FIXME: handle accessor descriptors
.as_data_descriptor()
.unwrap()
.value()
.as_number()
.unwrap() as i32;

It seems that setting only writable in a property should not change its value, so the length should remain a number.

 - Fix panic if first argument is not supplied.
 - Fix panic if second argument is not supplied.
 - Fix bug when the object is not a object.
 - Implemented `DefinePropertyOrThrow()`
@HalidOdat HalidOdat merged commit 4625c1a into master Jan 1, 2021
@HalidOdat HalidOdat deleted the fix/object-defineproperty branch January 1, 2021 01:48
@github-actions
Copy link

github-actions bot commented Jan 1, 2021

Benchmark for d29ca82

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 340.3±15.83ns 362.6±19.42ns -6.15%
Arithmetic operations (Full) 228.2±10.08µs 246.4±38.05µs -7.39%
Array access (Execution) 7.5±0.53µs 7.5±0.37µs 0.00%
Array access (Full) 250.5±19.23µs 270.1±16.16µs -7.26%
Array creation (Execution) 2.8±0.12ms 2.9±0.16ms -3.45%
Array creation (Full) 3.2±0.15ms 3.1±0.14ms +3.23%
Array pop (Execution) 1036.8±60.48µs 1070.2±46.34µs -3.12%
Array pop (Full) 1577.3±61.80µs 1511.4±33.61µs +4.36%
Boolean Object Access (Execution) 5.0±0.14µs 5.1±0.19µs -1.96%
Boolean Object Access (Full) 239.1±10.63µs 254.1±9.67µs -5.90%
Clean js (Execution) 713.8±59.31µs 751.6±30.01µs -5.03%
Clean js (Full) 1015.5±72.14µs 1092.1±90.11µs -7.01%
Clean js (Parser) 32.8±2.15µs 35.5±2.83µs -7.61%
Create Realm 450.2±16.29ns 464.5±22.84ns -3.08%
Dynamic Object Property Access (Execution) 5.0±0.47µs 5.0±0.24µs 0.00%
Dynamic Object Property Access (Full) 253.2±13.82µs 256.8±14.04µs -1.40%
Expression (Parser) 6.4±0.31µs 6.8±0.41µs -5.88%
Fibonacci (Execution) 831.8±36.92µs 840.3±51.21µs -1.01%
Fibonacci (Full) 1118.2±55.86µs 1185.0±40.48µs -5.64%
For loop (Execution) 21.6±1.02µs 22.5±1.25µs -4.00%
For loop (Full) 281.6±13.59µs 284.8±6.53µs -1.12%
For loop (Parser) 16.1±0.77µs 17.3±1.01µs -6.94%
Goal Symbols (Parser) 11.1±0.64µs 11.6±0.35µs -4.31%
Hello World (Parser) 2.9±0.18µs 3.1±0.13µs -6.45%
Long file (Parser) 744.4±31.91ns 794.6±37.03ns -6.32%
Mini js (Execution) 688.9±75.80µs 700.2±49.49µs -1.61%
Mini js (Full) 917.9±39.83µs 959.0±36.95µs -4.29%
Mini js (Parser) 29.6±2.51µs 30.8±1.04µs -3.90%
Number Object Access (Execution) 3.9±0.15µs 4.1±0.13µs -4.88%
Number Object Access (Full) 236.7±10.06µs 255.8±12.98µs -7.47%
Object Creation (Execution) 4.2±0.22µs 4.3±0.12µs -2.33%
Object Creation (Full) 254.9±17.09µs 255.2±15.46µs -0.12%
RegExp (Execution) 8.7±0.53µs 9.4±0.58µs -7.45%
RegExp (Full) 254.1±12.83µs 270.7±16.16µs -6.13%
RegExp Literal (Execution) 10.0±0.56µs 11.0±0.83µs -9.09%
RegExp Literal (Full) 255.9±18.08µs 262.2±10.83µs -2.40%
RegExp Literal Creation (Execution) 8.7±0.47µs 9.1±0.47µs -4.40%
RegExp Literal Creation (Full) 248.7±12.40µs 260.4±33.71µs -4.49%
Static Object Property Access (Execution) 4.4±0.20µs 4.6±0.49µs -4.35%
Static Object Property Access (Full) 246.9±8.13µs 255.6±9.19µs -3.40%
String Object Access (Execution) 7.0±0.62µs 7.2±0.29µs -2.78%
String Object Access (Full) 241.5±11.61µs 261.3±19.80µs -7.58%
String comparison (Execution) 6.2±0.60µs 6.2±0.30µs 0.00%
String comparison (Full) 244.0±9.03µs 261.4±7.05µs -6.66%
String concatenation (Execution) 4.9±0.19µs 5.1±0.27µs -3.92%
String concatenation (Full) 236.0±9.24µs 257.8±15.79µs -8.46%
String copy (Execution) 3.7±0.21µs 4.0±0.19µs -7.50%
String copy (Full) 234.0±9.51µs 249.8±12.81µs -6.33%
Symbols (Execution) 3.0±0.15µs 3.2±0.37µs -6.25%
Symbols (Full) 242.7±10.68µs 246.3±7.09µs -1.46%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants