-
-
Notifications
You must be signed in to change notification settings - Fork 407
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 cross-realm construction bugs #2786
Conversation
Drafting because I want to see the impact this change makes in the test suite first. |
Test262 conformance changes
Fixed tests (13):
Broken tests (20):
|
45dbf6e
to
30572fd
Compare
Codecov Report
@@ Coverage Diff @@
## main #2786 +/- ##
==========================================
- Coverage 50.66% 50.57% -0.09%
==========================================
Files 415 415
Lines 41063 41207 +144
==========================================
+ Hits 20803 20841 +38
- Misses 20260 20366 +106
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Ok, this PR fixes half of the problem. The other half is that functions should also store the There is a problem with this though: the realm is storing the full environment stack instead of only the global bindings. This means the vm needs to access the realm to access any binding at execution, which would be a disaster perf-wise if we wrapped the Considering this, it'll be better to merge this as it is to reduce the size of the next PR, which will solve the second part of the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some small suggestions :)
Also If the added TODO
s are not going to be addressed in this PR, maybe we should create issues about them?
Ah, I'll probably solve those on the follow-up PR. I just added those to be easily searchable when I start working on it. |
I'll try to use the new I went through most filled and it looked good. If I have the time I'll finish the review today :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! LGTM
ac47b5f
to
8e302f6
Compare
There was a problem hiding this 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! :)
bors r+ |
This Pull Request fixes test [`assert-throws-same-realm.js`](https://github.com/tc39/test262/blob/eb44f67274bf3896fbec8814f81dd2ae02236686/test/harness/assert-throws-same-realm.js). It changes the following: - Handles global variables through the global object, instead of the `context`. - Adds an `active_function` field to the vm, which is used as the `NewTarget` when certain builtins aren't called with `new`. - Adds a `realm_intrinsics` field to `Function`.
Pull request successfully merged into main. Build succeeded: |
This Pull Request fixes test
assert-throws-same-realm.js
.It changes the following:
context
.active_function
field to the vm, which is used as theNewTarget
when certain builtins aren't called withnew
.realm_intrinsics
field toFunction
.