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 internal methods #1076

Closed
wants to merge 6 commits into from
Closed

Fix internal methods #1076

wants to merge 6 commits into from

Conversation

tofpie
Copy link
Contributor

@tofpie tofpie commented Jan 17, 2021

This Pull Request fixes #591 and #602.

It changes the following:

  • All internal methods of GcObject take a &Context and return a Result
  • Implementation of specific internal methods for String
  • Context becomes externally immutable, with only interior mutability
  • LexicalEnvironment becomes externally immutable, with only interior mutability

# Conflicts:
#	boa/src/builtins/array/array_iterator.rs
#	boa/src/builtins/iterable/mod.rs
#	boa/src/builtins/map/map_iterator.rs
#	boa/src/builtins/object/for_in_iterator.rs
#	boa/src/builtins/string/string_iterator.rs
#	boa/src/realm.rs
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #1076 (8446bb0) into master (d15f55e) will increase coverage by 0.06%.
The diff coverage is 67.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   58.68%   58.75%   +0.06%     
==========================================
  Files         176      175       -1     
  Lines       12407    12459      +52     
==========================================
+ Hits         7281     7320      +39     
- Misses       5126     5139      +13     
Impacted Files Coverage Δ
boa/examples/classes.rs 0.00% <0.00%> (ø)
boa/src/class.rs 0.00% <0.00%> (ø)
boa/src/exec/mod.rs 57.14% <ø> (ø)
boa/src/syntax/ast/node/await_expr/mod.rs 0.00% <0.00%> (ø)
...ax/ast/node/declaration/async_function_decl/mod.rs 22.22% <0.00%> (ø)
...ax/ast/node/declaration/async_function_expr/mod.rs 24.00% <0.00%> (ø)
boa/src/vm/mod.rs 0.00% <0.00%> (ø)
boa_cli/src/main.rs 33.33% <ø> (ø)
boa_tester/src/exec.rs 0.00% <0.00%> (ø)
boa_wasm/src/lib.rs 0.00% <0.00%> (ø)
... and 87 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 d15f55e...f7a8517. Read the comment docs.

@Razican
Copy link
Member

Razican commented Jan 17, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,805 24,857 +52
Ignored 15,587 15,587 0
Failed 38,105 38,053 -52
Panics 26 26 0
Conformance 31.60 31.67 +0.07%

@RageKnify RageKnify added builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution labels Jan 17, 2021
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.

Minor mistakest to fix. Can you explain the reason for:

  • Context becomes externally immutable, with only interior mutability

Sounds, reasonable just trying to understand the reasoning.

boa/src/context.rs Outdated Show resolved Hide resolved
boa/src/builtins/json/mod.rs Outdated Show resolved Hide resolved
boa/src/object/internal_methods.rs Outdated Show resolved Hide resolved
boa/src/object/internal_methods.rs Outdated Show resolved Hide resolved
@tofpie
Copy link
Contributor Author

tofpie commented Jan 18, 2021

All internal methods (as defined by the spec, that is [[Get]], [[Set]] or [[DefineOwnProperty]] for instance) of a GcObject should have a parameter &mut Context as in some exotic objects implementation (such as Proxy) they can make calls to functions. If you add &mut Context to internal methods, then you must also add it to nearly all methods of EnvironmentTrait as the environment is sometimes itself an object (in ObjectEnvironmentRecord or GlobalEnvironmentRecord), but then each time the environment is modified (when declaring a variable or calling a function), then you have to borrow Context mutably twice, once to get the environment and another time when you call a method of EnvironmentTrait. And this is of course not allowed by the Rust compiler. And this is the same for LexicalEnvironment.

@RageKnify RageKnify added this to the v0.12.0 milestone Jan 19, 2021
# Conflicts:
#	boa/src/value/mod.rs
@HalidOdat
Copy link
Member

I would like to try to fix the context/LexicalEnvironment mutable borrow problem, without introducing so much interior mutability.

Hopefully I'll be done with this by tomorrow and create a PR, now that I have a little more time to spend on the project :)

@Razican
Copy link
Member

Razican commented May 10, 2021

I would like to try to fix the context/LexicalEnvironment mutable borrow problem, without introducing so much interior mutability.

Hopefully I'll be done with this by tomorrow and create a PR, now that I have a little more time to spend on the project :)

How is this going?

BTW, this needs a rebase with all the changes in master now.

@HalidOdat
Copy link
Member

I would like to try to fix the context/LexicalEnvironment mutable borrow problem, without introducing so much interior mutability.
Hopefully I'll be done with this by tomorrow and create a PR, now that I have a little more time to spend on the project :)

How is this going?

#1131 is going to fix this problem, without any additional interior mutability :)

@Razican
Copy link
Member

Razican commented May 20, 2021

Now that #1131 has been merged, I think that this only needs a rebase to use the latest changes, @tofpie.

@Razican Razican modified the milestones: v0.12.0, v0.13.0 Jun 7, 2021
@HalidOdat
Copy link
Member

This has too many merge conflicts to try rebase this, it would be better to start over, besides most of the internal methods have already been implemented. So closing this PR.

@HalidOdat HalidOdat closed this Jul 22, 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 execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Internal Object Methods
4 participants