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

Implement property accessors #987

Merged
merged 13 commits into from
Dec 31, 2020
Merged

Implement property accessors #987

merged 13 commits into from
Dec 31, 2020

Conversation

tofpie
Copy link
Contributor

@tofpie tofpie commented Dec 20, 2020

This Pull Request closes #986.

It changes the following:

  • Execution of property accessors

@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #987 (cef486e) into master (23bc476) will increase coverage by 0.24%.
The diff coverage is 80.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
+ Coverage   59.79%   60.03%   +0.24%     
==========================================
  Files         167      167              
  Lines       11087    11134      +47     
==========================================
+ Hits         6629     6684      +55     
+ Misses       4458     4450       -8     
Impacted Files Coverage Δ
boa/src/property/mod.rs 56.48% <ø> (+3.05%) ⬆️
...ax/ast/node/declaration/arrow_function_decl/mod.rs 60.86% <ø> (ø)
...c/syntax/ast/node/declaration/function_expr/mod.rs 61.29% <0.00%> (ø)
boa/src/context.rs 65.69% <41.93%> (+0.93%) ⬆️
boa/src/value/display.rs 85.00% <60.00%> (-3.64%) ⬇️
boa/src/builtins/map/mod.rs 72.02% <66.66%> (-0.44%) ⬇️
boa/src/environment/object_environment_record.rs 23.72% <66.66%> (+2.67%) ⬆️
boa/src/syntax/ast/node/object/mod.rs 58.92% <74.19%> (+21.78%) ⬆️
boa/src/object/gcobject.rs 65.00% <78.57%> (-0.24%) ⬇️
boa/src/builtins/object/mod.rs 67.26% <80.00%> (-0.41%) ⬇️
... and 29 more

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 23bc476...8a7ac4e. Read the comment docs.

@tofpie
Copy link
Contributor Author

tofpie commented Dec 20, 2020

@RageKnify I continue the conversation started on the issue.

In fact, property accessors are implemented both in Value::get/set_field and GcObject::get/set. As this PR has modified all the calling sites for Value::get/set_field (adding context and ?), I could also replace all calls to Value::get/set_field by to_object(..)?.get/set, it would not be such a big effort. What do you think?

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good!

Check my comments, some are unrelated but it would be nice to change in this PR :)

boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
boa/src/context.rs Outdated Show resolved Hide resolved
boa/src/context.rs Outdated Show resolved Hide resolved
boa/src/property/mod.rs Outdated Show resolved Hide resolved
boa/src/value/mod.rs Outdated Show resolved Hide resolved
@RageKnify
Copy link
Contributor

@RageKnify I continue the conversation started on the issue.

In fact, property accessors are implemented both in Value::get/set_field and GcObject::get/set. As this PR has modified all the calling sites for Value::get/set_field (adding context and ?), I could also replace all calls to Value::get/set_field by to_object(..)?.get/set, it would not be such a big effort. What do you think?

Your idea is that it will be a simple transition in the future, right? Makes sense.

We will also eventually remove Value::set_field and Value::get_field since only GcObject will have that kind of functionality, right?

@tofpie
Copy link
Contributor Author

tofpie commented Dec 20, 2020

@HalidOdat said it is not as simple as I initially thought:

Maybe I could change all the calls to Value::get/set_field to to_object(..)?.get/set as you have suggested in #577. Now that context has been added to get/set_field, the bulk of the work would be a search and replace. What do you think?

It's not what needs to be done .to_object() does some other things and would not be spec compliant, the calls to .get/.set should be reinforced by the type system as it does not make much sense to have set/get for non-object (Value type) values as explained in #577.

we can merge this as it's main purpose was to add property accessors, and replace .get_field and .set_field builtin function by builtin function, through incremental changes (with many PRs), Allowing .get_field/set_field to take context is the first step, the end goal should be to completely remove them, it's really hard otherwise.

I guess I will leave as it is, just implementing the suggestions and add some unit tests.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Once this is merged we can start removing the .set_field and .get_field functions from builtin functions :)

@HalidOdat HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request execution Issues or PRs related to code execution labels Dec 20, 2020
@HalidOdat HalidOdat added this to the v0.11.0 milestone Dec 20, 2020
@RageKnify RageKnify self-requested a review December 21, 2020 00:32
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.

Some minor things can be cleaned up and I have some questions on Value::new_object current state.

boa/src/builtins/map/mod.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/node/object/mod.rs Outdated Show resolved Hide resolved
boa/src/value/display.rs Outdated Show resolved Hide resolved
boa/src/value/mod.rs Outdated Show resolved Hide resolved
@Razican Razican requested review from RageKnify and Razican December 28, 2020 13:25
@HalidOdat
Copy link
Member

Test262 conformance changes:

Test result master count PR count difference
Total 78,493 78,493 0
Passed 19,787 20,855 +1,068
Ignored 15,585 15,585 0
Failed 43,121 42,053 -1,068
Panics 396 394 -2
Conformance 25.21 26.57 +1.36%

The conformance increase is really nice :)

@HalidOdat HalidOdat merged commit 4cede75 into boa-dev:master Dec 31, 2020
@tofpie tofpie deleted the property-accessors branch December 31, 2020 22:26
RageKnify added a commit that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement property accessors
3 participants