fix destructuring property reference target order#4336
Open
dotcarmen wants to merge 2 commits intoboa-dev:mainfrom
Open
fix destructuring property reference target order#4336dotcarmen wants to merge 2 commits intoboa-dev:mainfrom
dotcarmen wants to merge 2 commits intoboa-dev:mainfrom
Conversation
Member
|
Seems like this causes other tests to fail: Test262 test suite
Fixed tests (1):Broken tests (996):New panics (546): |
Author
|
yeah, i saw that when i saw ci fail. i'm looking into this test which seems like a good starting point for isolating the problem but i think i found a bug? when trying to run the first statement using repl outputi captured this from the build on main so it's not isolated to my change. the bug doesn't happen when |
Member
Yeah, that's a good starting point :)
The --dump-ast option doesn't execute the code; it only performs lexing and parsing, then outputs the resulting AST. This can be particularly helpful when diagnosing parser-related issues. |
Member
|
@HalidOdat They're talking about the final message after the console prints the ast:
|
Member
ah, sorry about that, yeah... that's interesting, that definitely shouldn't happen, seems like a bug with the scope analyzer, nice find! @dotcarmen I'll create an issue for it 😄 |
Member
|
Actually found the bug, It's not a bug with the scope analyzer, the bug is in the CLI, will create a PR for it 😄 EDIT: #4337 -- May have been a false positive lint introduced in https://github.com/boa-dev/boa/pull/4315/files#diff-37970e7d817d4316a093bb967d74ff7e26d9c6c01d93f5a79d3d195ba9f6155bL401-R404 |
dotcarmen
force-pushed
the
fix-destructuring-eval-order
branch
from
83fd4f7 to
479320b
Compare
Author
|
probably shouldn't have squashed and merged in the same push 😅 sorry here's the diff since the last reviewdiff --git a/core/engine/src/bytecompiler/declaration/declaration_pattern.rs b/core/engine/src/bytecompiler/declaration/declaration_pattern.rs
index 91c8b7b486..c9eade71cb 100644
--- a/core/engine/src/bytecompiler/declaration/declaration_pattern.rs
+++ b/core/engine/src/bytecompiler/declaration/declaration_pattern.rs
@@ -39,61 +39,59 @@
let dst = self.register_allocator.alloc();
match name {
- PropertyName::Literal(ident) => {
+ PropertyName::Literal(field_ident) => {
self.emit_get_property_by_name(
&dst,
object,
object,
- ident.sym(),
+ field_ident.sym(),
);
let key = self.register_allocator.alloc();
self.emit_push_literal(
Literal::String(
self.interner()
- .resolve_expect(ident.sym())
+ .resolve_expect(field_ident.sym())
.into_common(false),
),
&key,
);
+ excluded_keys_registers.push(key);
self.emit_binding(
def,
ident.to_js_string(self.interner()),
&dst,
);
- excluded_keys_registers.push(key);
}
PropertyName::Computed(node) => {
let key = self.register_allocator.alloc();
self.compile_expr(node, &key);
- let object_key = self.register_allocator.alloc();
+ let property_key = self.register_allocator.alloc();
self.bytecode.emit_to_property_key(
key.variable(),
- object_key.variable(),
+ property_key.variable(),
);
self.register_allocator.dealloc(key);
-
self.emit_binding(
def,
ident.to_js_string(self.interner()),
&dst,
);
-
if rest_exits {
self.bytecode.emit_get_property_by_value_push(
dst.variable(),
- object_key.variable(),
+ property_key.variable(),
object.variable(),
object.variable(),
);
- excluded_keys_registers.push(object_key);
+ excluded_keys_registers.push(property_key);
} else {
self.bytecode.emit_get_property_by_value(
dst.variable(),
- object_key.variable(),
+ property_key.variable(),
object.variable(),
object.variable(),
);
- self.register_allocator.dealloc(object_key);
+ self.register_allocator.dealloc(property_key);
}
}
} |
Author
|
looks like i'm still failing +798 test262 tests with this pr, it's either gotta be handling defaults or handling rests (my money's on the first one) but i'll get into it tomorrow |
Author
|
still getting +674 failures, but now i'm second guessing myself? using the debug object to debug: here's the bytecode on mainhere's the bytecode on my last commitso now i have 2 instead of 3 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
==========================================
+ Coverage 47.24% 50.24% +2.99%
==========================================
Files 476 499 +23
Lines 46892 50158 +3266
==========================================
+ Hits 22154 25200 +3046
- Misses 24738 24958 +220 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jedel1043
added
bug
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 PR fixes failing test language/destructuring/binding/keyed-destructuring-property-reference-target-evaluation-order-with-bindings
It changes the following:
GetPropertyByValue{,Push}instr to convert the property key value to the property key, use the property key directly