Wrap for-using bindings in VariableDeclaratorNode#1999
Open
marcoww6 wants to merge 1 commit into
Open
Conversation
Summary: #### From Human The fix makes sense to me especially looking at the test change in xplat/static_h/test/Parser/for-using.js from a bare identifier to a normal declaration block. Should be able to fix problems in facebook#1981 #### From AI Hermes' parser builds a malformed AST for `for (using x of y)` and `for [await] (await using x of y)`: it pushes a bare `IdentifierNode` into `VariableDeclaration._declarations` instead of wrapping it in a `VariableDeclaratorNode`. Compare to `for (var x of y)`, which correctly produces `[ VariableDeclarator { init: null, id: Identifier } ]`. Downstream Sema code (`SemanticResolver::extractIdentsFromDecl`, `ScopedFunctionPromoter::extractDeclaredIdents`) calls `cast<VariableDeclaratorNode>(&decl)` over the list. With an `IdentifierNode` in the slot, the cast reads `_init`/`_id` at offsets that actually contain `_name` (a `UniqueString*`) and `_typeAnnotation` (often null), then dereferences the `UniqueString*` as a `Node*`. In native debug this fires a `Casting.h` assertion; in release WASM it traps non-deterministically depending on heap layout — the bug reported in facebook#1981, which Prettier tripped over while adding `using` support in prettier/prettier#18961. It surfaces only via hermes-parser (not regular Hermes execution) because `resolveASTForParser` runs Sema with `compile=false`, bypassing the explicit `using declarations are not yet supported` gate at `SemanticResolver.cpp:326`. Tools using hermes-parser (Prettier, ESLint, Babel) walk past the gate into the bad cast. **Fix:** in the two `using` for-loop branches in `JSParserImpl.cpp` (`:1912` and `:1932`), wrap the binding identifier in a `VariableDeclaratorNode` (with `init = nullptr`) before pushing into the declarations list. Mirrors what `parseVariableDeclaration` already does at `:1257` for the no-initializer case. The non-for-loop `parseUsingDeclaration` path was already correct because it routes through `parseVariableDeclarationList`. **Tests:** - New `test/Parser/for-using-declarator.js`: asserts the AST contains `VariableDeclarator { init: null, id: Identifier }` for `for (using x of y)` and `for await (await using x of y)`. - New `test/Sema/for-using-not-supported.js`: asserts the compile-path Sema rejects `for (using ...)` and `for await (await using ...)` with the clean `"using declarations are not yet supported"` diagnostic instead of crashing in `cast<VariableDeclaratorNode>`. Covers the regression at the layer where it manifested. - Update stale CHECK lines in pre-existing `test/Parser/for-using.js`, which encoded the buggy bare-Identifier shape and would have masked any future re-introduction of the bug. **Caveat:** verified end-to-end against the native debug binary (assertion no longer fires; AST shape matches `for (var x of y)`). The original WASM trap was diagnosed by code-reading — the cast site, the `compile=false` codepath in `hermes-parser-wasm.cpp:104`, and the field-offset overlap between `IdentifierNode` and `VariableDeclaratorNode` all line up — but the WASM was not rebuilt to empirically confirm the 1-in-10000 trap drop. Reviewed By: avp Differential Revision: D102691940
|
@marcoww6 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D102691940. |
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
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.
Summary:
From Human
The fix makes sense to me especially looking at the test change in xplat/static_h/test/Parser/for-using.js from a bare identifier to a normal declaration block. Should be able to fix problems in #1981
From AI
Hermes' parser builds a malformed AST for
for (using x of y)andfor [await] (await using x of y): it pushes a bareIdentifierNodeintoVariableDeclaration._declarationsinstead of wrapping it in aVariableDeclaratorNode. Compare tofor (var x of y), which correctly produces[ VariableDeclarator { init: null, id: Identifier } ].Downstream Sema code (
SemanticResolver::extractIdentsFromDecl,ScopedFunctionPromoter::extractDeclaredIdents) callscast<VariableDeclaratorNode>(&decl)over the list. With anIdentifierNodein the slot, the cast reads_init/_idat offsets that actually contain_name(aUniqueString*) and_typeAnnotation(often null), then dereferences theUniqueString*as aNode*. In native debug this fires aCasting.hassertion; in release WASM it traps non-deterministically depending on heap layout — the bug reported in #1981, which Prettier tripped over while addingusingsupport in prettier/prettier#18961.It surfaces only via hermes-parser (not regular Hermes execution) because
resolveASTForParserruns Sema withcompile=false, bypassing the explicitusing declarations are not yet supportedgate atSemanticResolver.cpp:326. Tools using hermes-parser (Prettier, ESLint, Babel) walk past the gate into the bad cast.Fix: in the two
usingfor-loop branches inJSParserImpl.cpp(:1912and:1932), wrap the binding identifier in aVariableDeclaratorNode(withinit = nullptr) before pushing into the declarations list. Mirrors whatparseVariableDeclarationalready does at:1257for the no-initializer case. The non-for-loopparseUsingDeclarationpath was already correct because it routes throughparseVariableDeclarationList.Tests:
test/Parser/for-using-declarator.js: asserts the AST containsVariableDeclarator { init: null, id: Identifier }forfor (using x of y)andfor await (await using x of y).test/Sema/for-using-not-supported.js: asserts the compile-path Sema rejectsfor (using ...)andfor await (await using ...)with the clean"using declarations are not yet supported"diagnostic instead of crashing incast<VariableDeclaratorNode>. Covers the regression at the layer where it manifested.test/Parser/for-using.js, which encoded the buggy bare-Identifier shape and would have masked any future re-introduction of the bug.Caveat: verified end-to-end against the native debug binary (assertion no longer fires; AST shape matches
for (var x of y)). The original WASM trap was diagnosed by code-reading — the cast site, thecompile=falsecodepath inhermes-parser-wasm.cpp:104, and the field-offset overlap betweenIdentifierNodeandVariableDeclaratorNodeall line up — but the WASM was not rebuilt to empirically confirm the 1-in-10000 trap drop.Reviewed By: avp
Differential Revision: D102691940