Cherry-pick: Fix class variable caching bugs when class is in finally block#2046
Open
ramonclaudio wants to merge 1 commit into
Open
Conversation
Summary: Finally blocks cause code to be emitted twice: once for normal flow and once for exception handling. This exposed a class of bugs where internal Variables created during class compilation were not cached. On recompilation, new Variables were created, but cached functions (via findCompiledEntity) still referenced the original Variables, causing crashes or incorrect behavior. Fixed issues in genLegacyClassLike: - classVar, clsPrototypeVar: new Variables created each compilation, but cached initializer functions referenced old ones. Caused assertion failure: "PutOwn requires object operand". - instElemInitFuncVar: same issue for instance element initializers. - Computed field key variables: same issue for computed properties. Fixed issues in emitPrivateNameDeclarations: - staticBrand, instanceBrand: Variables for private method brands were local variables, not cached. Additionally, brand stores were only emitted when creating the Variable (inside getPrivateBrand), not on recompilation. This caused "undefined is not a function" errors when calling private methods in classes defined in finally blocks. The fix introduces LegacyClassInternalVars to cache all internal Variables created during legacy class compilation, keyed by the class AST node. When the same class is compiled multiple times, existing Variables are reused. For private brands, the stores are now emitted at the end of emitPrivateNameDeclarations unconditionally, ensuring they exist in all code paths. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Reviewed By: fbmal7 Differential Revision: D90690547 fbshipit-source-id: abe039ecb6913fe8534faf4a7544a84745991182 (cherry picked from commit 1e94fbe)
meta-cla
Bot
added
the
CLA Signed
This was referenced Jun 4, 2026
Closed
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
1 participant
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
Cherry-pick of
1e94fbe0ebb46d676ff656287e17c58839765e73fromstatic_hto the250829098.0.0-stablebranch.A
finallyblock emits its body twice, once for normal flow and once for the exception path. On the second emission, legacy class compilation created fresh internal Variables (class and prototype vars, the instance-element init function, computed field keys, private brands) while cached functions reached viafindCompiledEntitystill referenced the originals, miscompiling classes declared infinallyblocks. The fix caches these in aLegacyClassVarsstruct keyed by the class AST node (legacyClassVars_) and emits private-brand stores unconditionally.This landed on
static_hafter the branch was cut (2025-08-29), so it is missing from the V1 line React Native ships (needed by react/react-native#56816). The stacked typed-class sibling639e5d6afis intentionally not included: it fixes the Static Hermes typed class path, which standard React Native JS does not exercise. Authored by @tmikov, reviewed internally by @fbmal7.Test plan
test/IRGen/class-static-in-finally.jsandtest/hermes/class-in-finally.js, a CHECK update totest/IRGen/regress-class-recompiled.jsthat drops the duplicate?C1#1,?C1.prototype#1,<instElemInitFunc:C1>#1Variables the bug produced, and a brand-store ordering update totest/IRGen/private-properties.js.hermesandhermescfrom250829098.0.0-stable(517edaff8): the baseline failsregress-class-recompiled.jsand the runtimeclass-in-finally.js. With this cherry-pick all four bundled tests pass.LIT_FILTER="class-in-finally|regress-class-recompiled|class-static-in-finally" cmake --build build --target check-hermespasses.