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

[Merged by Bors] - fix computed property methods can call super methods #2274

Closed
wants to merge 7 commits into from

Conversation

creampnx-x
Copy link
Contributor

@creampnx-x creampnx-x commented Sep 9, 2022

This Pull Request fixes: https://github.com/tc39/test262/blob/79e3bc5176b6f29a5aed3b7164a9c623a3a9a63b/test/language/computed-property-names/object/method/super.js

This PR solves the bug of using the super keyword in the method attribute of object. When the environment is None, the vm gets the top element of vm.stack as this, such as:

var a = {
    f() {
        return super.m();
    }
};

var b = {
    m() {
        retrun "super";
    }
};

Object.setPrototypeOf(a, b);

a.f(); // the top of stack is `a`
let f = a.f;
f(); // the top of stack is `global_this`, so `super` cannot be used

Can be improved

What I think is that when I use object.method(), the engine should bind this_object to the environment, instead of using vm.stack.last()....

TODOS

  1. super need to look for properties all the way to the end.

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #2274 (56e4f2a) into main (92ecbda) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2274      +/-   ##
==========================================
+ Coverage   41.33%   41.66%   +0.32%     
==========================================
  Files         234      234              
  Lines       22019    21992      -27     
==========================================
+ Hits         9101     9162      +61     
+ Misses      12918    12830      -88     
Impacted Files Coverage Δ
boa_engine/src/vm/mod.rs 46.93% <100.00%> (+0.84%) ⬆️
...syntax/parser/statement/iteration/for_statement.rs 37.15% <0.00%> (-2.38%) ⬇️
...engine/src/syntax/parser/expression/primary/mod.rs 35.40% <0.00%> (-1.39%) ⬇️
boa_engine/src/syntax/parser/statement/mod.rs 29.51% <0.00%> (-0.77%) ⬇️
...rser/expression/primary/function_expression/mod.rs 40.90% <0.00%> (-0.56%) ⬇️
boa_engine/src/builtins/function/mod.rs 23.43% <0.00%> (-0.04%) ⬇️
boa_interner/src/sym.rs 0.00% <0.00%> (ø)
boa_engine/src/syntax/ast/node/mod.rs 62.30% <0.00%> (+0.39%) ⬆️
boa_engine/src/bytecompiler/mod.rs 29.20% <0.00%> (+0.43%) ⬆️
... and 8 more

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

@Razican
Copy link
Member

Razican commented Sep 9, 2022

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 91,733 91,733 0
Passed 64,910 64,940 +30
Ignored 16,580 16,580 0
Failed 10,243 10,213 -30
Panics 0 0 0
Conformance 70.76% 70.79% +0.03%
Fixed tests (30):
test/language/computed-property-names/object/accessor/setter-super.js [strict mode] (previously Failed)
test/language/computed-property-names/object/accessor/setter-super.js (previously Failed)
test/language/computed-property-names/object/accessor/getter-super.js [strict mode] (previously Failed)
test/language/computed-property-names/object/accessor/getter-super.js (previously Failed)
test/language/computed-property-names/object/method/super.js [strict mode] (previously Failed)
test/language/computed-property-names/object/method/super.js (previously Failed)
test/language/statements/class/elements/super-access-inside-a-private-getter.js [strict mode] (previously Failed)
test/language/statements/class/elements/super-access-inside-a-private-getter.js (previously Failed)
test/language/statements/class/elements/super-access-inside-a-private-setter.js [strict mode] (previously Failed)
test/language/statements/class/elements/super-access-inside-a-private-setter.js (previously Failed)
test/language/expressions/super/prop-dot-obj-val.js [strict mode] (previously Failed)
test/language/expressions/super/prop-dot-obj-val.js (previously Failed)
test/language/expressions/super/prop-expr-obj-key-err.js [strict mode] (previously Failed)
test/language/expressions/super/prop-expr-obj-key-err.js (previously Failed)
test/language/expressions/super/prop-poisoned-underscore-proto.js [strict mode] (previously Failed)
test/language/expressions/super/prop-poisoned-underscore-proto.js (previously Failed)
test/language/expressions/super/prop-expr-obj-val-from-arrow.js [strict mode] (previously Failed)
test/language/expressions/super/prop-expr-obj-val-from-arrow.js (previously Failed)
test/language/expressions/super/prop-dot-obj-val-from-arrow.js [strict mode] (previously Failed)
test/language/expressions/super/prop-dot-obj-val-from-arrow.js (previously Failed)
test/language/expressions/super/prop-expr-obj-val.js [strict mode] (previously Failed)
test/language/expressions/super/prop-expr-obj-val.js (previously Failed)
test/language/expressions/object/method.js [strict mode] (previously Failed)
test/language/expressions/object/method.js (previously Failed)
test/language/expressions/object/getter-super-prop.js [strict mode] (previously Failed)
test/language/expressions/object/getter-super-prop.js (previously Failed)
test/language/expressions/object/method-definition/name-super-prop-param.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/name-super-prop-param.js (previously Failed)
test/language/expressions/object/method-definition/name-super-prop-body.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/name-super-prop-body.js (previously Failed)

@Razican
Copy link
Member

Razican commented Sep 10, 2022

Hi @creampnx-x! thanks for the contribution :) good catch. It seems that this is creating some new panics, unfortunately. You can check the panicking tests by using our guide in our contribution documentation. Most probably these panics are coming from that unwrap() call, where it would be better to change it for an undefined or return an error, didn't check the spec thoroughly.

Let us know if you need some guidance, and once that's fixed and you're ready, you can put the PR as "Ready for review", and we will give it a further look.

@creampnx-x creampnx-x marked this pull request as ready for review September 11, 2022 13:56
@creampnx-x
Copy link
Contributor Author

It seems these panics have been resolved when running the tests locally.Hope to review it if you have time. thanks.

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.

Thank you for the fix :) looks very good to me! I added a suggestion, but I'm happy to merge as-is too :) LGTM!

boa_engine/src/vm/mod.rs Outdated Show resolved Hide resolved
@Razican Razican added bug Something isn't working execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine. labels Sep 11, 2022
@Razican Razican added this to the v0.16.0 milestone Sep 11, 2022
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Nice catch!

Co-authored-by: Iban Eguia <razican@protonmail.ch>
@creampnx-x
Copy link
Contributor Author

Thank you for the fix :) looks very good to me! I added a suggestion, but I'm happy to merge as-is too :) LGTM!

Your suggestion has been submitted.

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 11, 2022
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel neccesary.
--->

This Pull Request fixes: https://github.com/tc39/test262/blob/79e3bc5176b6f29a5aed3b7164a9c623a3a9a63b/test/language/computed-property-names/object/method/super.js

This PR solves the bug of using the `super` keyword in the method attribute of object. When the environment is `None`, the `vm` gets the top element  of `vm.stack` as `this`, such as:
```js
var a = {
    f() {
        return super.m();
    }
};

var b = {
    m() {
        retrun "super";
    }
};

Object.setPrototypeOf(a, b);

a.f(); // the top of stack is `a`
let f = a.f;
f(); // the top of stack is `global_this`, so `super` cannot be used
```

### Can be improved

What I think is that when I use `object.method()`, the engine should bind `this_object`  to the `environment`, instead of using `vm.stack.last()...`.

### TODOS
1. `super` need to look for properties all the way to the end.

Co-authored-by: creampnx_x <2270436024@qq.com>
@bors
Copy link

bors bot commented Sep 11, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title fix computed property methods can call super methods [Merged by Bors] - fix computed property methods can call super methods Sep 11, 2022
@bors bors bot closed this Sep 11, 2022
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 vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants