[Merged by Bors] - Refactor some class features #2513
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test262 conformance changes
Fixed tests (265): |
raskad
requested review from
nekevss,
jedel1043,
HalidOdat,
jasonwilliams,
RageKnify and
Razican
Codecov Report
@@ Coverage Diff @@
## main #2513 +/- ##
==========================================
+ Coverage 50.38% 50.98% +0.59%
==========================================
Files 377 373 -4
Lines 36917 36812 -105
==========================================
+ Hits 18600 18767 +167
+ Misses 18317 18045 -272
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jedel1043
requested changes
Jan 12, 2023
boa_engine/src/object/mod.rs
Outdated
| @@ -1773,58 +1772,119 @@ impl Object { | |||
| self.properties.remove(key) | |||
| } | |||
|
|
|||
| /// Check if a private name exists. | |||
There was a problem hiding this comment.
I'm seeing this resembles the PrivateElementFind operation and friends. Could this be moved to boa_engine/src/object/operations.rs and adapted to somewhat resemble the spec operations? Probably also initialize_instance_elements.
I say this because there's a host hook HostEnsureCanAddPrivateElement that needs to be called on the PrivateFieldAdd and PrivateMethodOrAccessorAdd operations to restrict the addition of new private fields for an object.
There was a problem hiding this comment.
Good call. I've refactored those methods and they are mostly equivalent to the spec now.
| description: Sym, | ||
|
|
||
| /// The indices of the private name are included to ensure that private names are unique. | ||
| pub(crate) indices: (usize, usize), |
There was a problem hiding this comment.
I was reading the spec about private names and it specifies that PrivateNames should be globally unique, but indices are only unique in the context of a single Context, right? I was thinking that the global uniqueness property is probably to restrict the access of private names from objects that share the same indices but don't belong to the same Context, but to me it seems like this doesn't prevent that? I ask this because I don't know if you've already considered that.
There was a problem hiding this comment.
There is still some uniqueness missing with this implementation. We indeed have to add some runtime unique identifier to separate PrivateName of multiple objects that are created from the same class. I think this will probably be some kind of Context wide index. As far as different Contexts go, I think that would need some further work as our Context is currently also a Realm. If we have figured out how we can separate those, the Context should be the outermost api to the engine and imo should the the uniqueness-border.
I left the runtime uniqueness out of the PR to limit the scope somewhat.
There was a problem hiding this comment.
It's okay! Can you add a TODO describing this then? It would be good to track this for when we implement proper Realms.
raskad
force-pushed
the
fix-class-bugs
branch
from
7f922eb
to
bd01017
Compare
jedel1043
requested changes
Jan 13, 2023
raskad
force-pushed
the
fix-class-bugs
branch
from
bd24008
to
85abc6e
Compare
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Jan 15, 2023
This Pull Request fixes various bugs related to classes. The biggest changes are: - Changed private names to be unique across multiple classes. - Changed private name resolution to work via a visitor after a class is parsed. The way class early errors are defined makes it impossible to perform private name resolution while parsing. - Added function names to class methods. - Added class name binding to method function environments. - Separated opcodes for `static` and non-`static` class method definitions to make the above operations possible. There are still some bugs and further issues with classes but this is already a lot.
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes various bugs related to classes.
The biggest changes are:
staticand non-staticclass method definitions to make the above operations possible.There are still some bugs and further issues with classes but this is already a lot.