fix(decorator): hoist consts referenced by emitted Ivy definitions#302
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483e24edaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Closes #287. When `@Component` metadata such as `providers` references a top-level `const` declared *after* the class, the emitted `static ɵcmp` field evaluates `ɵɵProvidersFeature([…])` eagerly at class-init time. With the binding still in TDZ, this threw `ReferenceError` at module load. Match Angular's official compiler by hoisting referenced VariableDeclaration statements above the earliest class that needs them. Identifier collection walks decorator metadata but stops at function/arrow/class bodies, so lazy references like `useFactory: () => DEP` don't trigger unnecessary moves. Function declarations are JS-hoisted already, and class declarations are intentionally skipped to avoid clobbering the transform pipeline's existing edits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brooooooklyn
force-pushed
the
fix/hoist-tdz-consts-issue-287
branch
from
483e24e to
0961ef6
Compare
After #299 turned `emit_class_metadata` on by default, the raw `${UNRESOLVED}-tag` template literal is intentionally preserved verbatim inside `ɵsetClassMetadata(...)` to mirror ngc — that's runtime metadata, not the compiled selector. The original test asserted on the full output and now panics on every CI run since d83445f landed on main. Narrow the assertion to the `ɵcmp` definition itself and additionally verify the compiled selector falls back to `ng-component`, matching the test's stated intent ("unresolved interpolation must not produce a partial/garbage selector"). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ceae120b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two PR-review bugs in the hoist pass:
1. **Transitive deps were dropped.** When `@Component({ providers: PROVIDERS
})` was followed by `const PROVIDERS = [{ provide: TOKEN, ... }]; const
TOKEN = ...;`, only `PROVIDERS` got hoisted. The hoisted `const PROVIDERS
= [...]` then evaluated above the class and TDZ-threw on `TOKEN` — just
moving the same `ReferenceError` one frame deeper.
2. **Multi-declarator dedup was nondeterministic.** The plan was keyed by
binding *name*, so `const A = 1, B = 2;` referenced by two different
classes produced two `HoistEntry` values sharing `stmt_start` but
carrying different `insert_at` targets. The `emitted_stmts` dedup kept
whichever HashMap iteration visited first — often the *later* class —
leaving the earlier class in the TDZ.
Fix:
* Key the plan by `stmt_start` and merge collisions by taking MIN
`insert_at` (no more nondeterministic dedup; multi-declarators collapse
to one entry by construction).
* Per-statement, collect the union of identifier references across every
declarator initializer and feed them back into a BFS worklist — so
hoisting `PROVIDERS` also schedules `TOKEN` (transitive closure).
* Topologically sort the planned statements before emission so
dependencies land *before* their dependents in the hoisted prelude
(e.g. `const TOKEN` precedes `const PROVIDERS = [{ provide: TOKEN, ...
}]`). DFS is iterative to avoid stack overflow on deep chains; cycles
are broken silently because they can't yield a valid evaluation order
anyway.
Adds two integration tests that lock in both fixes:
* `component_provider_aggregate_const_pulls_in_transitive_tdz_dep`
* `component_shared_multideclarator_const_hoists_above_earliest_referencer`
(the latter was flaky against the old code — 7/20 failure rate locally
before this change, 30/30 passes after).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96f8c9afbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two PR #302 review fixes for the const-hoist: 1. Off-by-one at the class-body boundary (Cursor): the check `stmt_start <= class_body_end` skipped a const declared at exactly `class.body.span.end` (no whitespace between `}` and `const`). `class.body.span.end` is the exclusive end of the body, so a stmt starting there is the very next byte after the class and still needs hoisting. Switched to `<`. To keep the new delete from chewing into the `decls_after_class` insert at the same offset, the hoist delete now runs with a negative priority so it applies before that insert. 2. Transitive deps through function calls (Codex): a hoisted initializer that calls a top-level function (`const PROVIDERS = makeProviders()`) evaluates that function's body at module load, so any later-declared binding the body reads still TDZ-throws. Indexed top-level function declarations via `oxc_ast_visit::Visit` (skipping nested function / arrow / class expression bodies for the same lazy-evaluation reason as the existing expression walker), and chase identifiers through them in the BFS and in the topological-sort edge expansion. Added two regression tests: - `component_provider_const_immediately_after_class_brace_is_hoisted` - `component_provider_const_via_function_call_pulls_in_transitive_tdz_dep` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2893ba7b07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replace the hand-rolled name-string indexing in `collect_hoist_edits` with SymbolId-based resolution backed by `oxc_semantic`. The AOT path in `transform_angular_file` now builds a `Semantic` and threads it into the hoister so every `IdentifierReference` resolves through the symbol table to the actual declaring binding. Previously, `binding_to_stmt: HashMap<&str, u32>` and `fn_body_idents: HashMap<&str, HashSet<&str>>` keyed everything by the identifier's spelling. If a nested scope shadowed a top-level binding with the same name, the walker couldn't tell them apart and might count a non-top-level reference as a TDZ-relevant hit on the top-level binding. After this refactor, every reference is resolved to a `SymbolId` and matched against the top-level binding set, so shadows are impossible. No observable behavior change is intended: all 344 integration tests (including the two regression tests from PR #302 `component_provider_const_immediately_after_class_brace_is_hoisted` and `component_provider_const_via_function_call_pulls_in_transitive_tdz_dep`) continue to pass. The hoist priorities, off-by-one boundary check, topological sort, and lazy-body skipping are all preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 616588614a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three Codex review findings on PR #302: 1. Lazy fn-value over-hoisting: when a top-level function is referenced from decorator metadata as a *value* (e.g. `useFactory: makeFactory`) the BFS still chased its body refs and hoisted them above the class. Since Angular invokes such factories lazily, this could introduce a fresh TDZ that didn't exist in the source. Introduce an `eagerly_called` closure (direct callees in any top-level initializer or decorator metadata, transitively expanded through `fn_body_called_symbols`) and gate the BFS body-chase branch plus `expand_through_functions` on it. Regression test: `component_provider_useFactory_function_value_does_not_hoist_body_deps`. 2. `Expression::ChainExpression` swallowed by catch-all: optional chaining like `{ provide: TOKEN?.id, useValue: 1 }` recorded no reference to `TOKEN`. Add an explicit arm that dispatches each `ChainElement` variant (`CallExpression`, member variants, `TSNonNullExpression`) to the matching collection logic, also recording the inner call's direct callee for `f?.()`. Regression test: `component_provider_optional_chain_token_is_hoisted`. 3. Destructured top-level bindings not indexed: `collect_top_level_bindings` only handled `BindingPattern::BindingIdentifier`, so `const { TOKEN } = TOKENS;` never made it into `symbol_to_stmt`. Add `for_each_binding_identifier` recursive walker covering `ObjectPattern`/`ArrayPattern`/`AssignmentPattern` (plus rest elements and nested patterns). Regression test: `component_provider_destructured_top_level_token_is_hoisted`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d4a3fc9f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… guard
Three bot-review findings on the post-refactor hoist:
1. Multi-declarator over-hoist (Codex). When a `const TOKEN = 'tok',
BACKREF = TestComponent;` declarator list is hoisted because TOKEN is
referenced in decorator metadata, the peer `BACKREF = TestComponent`
moves above the class and introduces a new TDZ. Added a safe-skip:
refuse to hoist a statement when any of its initializer symbols
resolves to a top-level class declared at position >=
`effective_start`. Indexed via a new `collect_top_level_class_positions`
helper. (Split-hoist is intentionally out of scope; the safe-skip
preserves the "no regressions" invariant — the user's existing TDZ on
the directly-referenced symbol stays, but we don't introduce a new one.)
2. IIFE bodies in decorator metadata missed (Codex). `providers: (() =>
[{ provide: TOKEN }])()` runs the arrow body eagerly at class init,
but the lazy-bodies rule was treating every arrow/function expression
body as opaque. New `walk_iife_callee_body` detects an immediately-
invoked function/arrow callee (peeling parens + TS wrappers) and
walks the body via `FunctionBodyIdentVisitor`; the wider lazy rule
still applies elsewhere. Symmetric handling for `NewExpression` and
`ChainElement::CallExpression`.
3. Global `eagerly_called` bleeds across classes (Cursor). Previously
seeded from every top-level initializer's call sites + every
decorator's call sites, so `const X = foo()` in one part of the
module would mark `foo` as eagerly-called for every other class
too — even classes that only reference `foo` as a value
(`useFactory: foo`). Replaced the global precomputation with a
per-class closure inside the BFS: seeded only from THIS class's
`decorator_called`, extended incrementally as the BFS plans
bindings (adding each planned init's `init_called_symbols` and
re-closing through `fn_body_called_symbols`). Functions popped
before they became eagerly_called are deferred and belatedly
chased when promotion happens. Topological sort uses the union
of per-class sets so dependency edges still see every fn whose
body fires at module load for some hoisted statement.
Regression tests for each finding:
- component_provider_multi_declarator_with_class_self_ref_skips_hoist
- component_provider_iife_metadata_hoists_inner_token
- component_provider_useFactory_value_does_not_chase_global_eager_caller
All 350 integration tests pass; fmt clean.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62de526973
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Parameter default expressions (`function f(x = TOKEN)` /
`function f({ a = X } = {})`) evaluate at call time, before the body
runs. When a hoisted initializer eagerly calls a top-level function,
any later-declared binding read by a parameter default is just as
TDZ-relevant as a body ref. The previous scan only walked the function
body, so such defaults left their referenced bindings below the class
and the hoisted call-site threw `ReferenceError: Cannot access ... before
initialization`.
Add a `for_each_pattern_default` helper that yields every nested
`AssignmentPattern::right` inside a `BindingPattern`. Use it (alongside
`FormalParameter::initializer` for top-level defaults) in
`collect_top_level_bindings` for top-level function declarations and in
`walk_iife_callee_body` for IIFE callee arrow/function expressions. The
collected refs and direct callees join the same `fn_body_symbol_refs` /
`fn_body_called_symbols` sets the body walk populates, so the existing
BFS and eagerly-called closure pick them up transparently.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 311a41a046
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…-called fns
`collect_top_level_bindings` now walks `AssignmentPattern.right` defaults
nested in each declarator's binding pattern, so `const { TOKEN = FALLBACK }
= {}` records `FALLBACK` as an init-time dep and keeps the dependency
graph TDZ-correct. `FunctionBodyIdentVisitor` now detects IIFE callees in
`visit_call_expression` / `visit_new_expression` and walks their bodies
eagerly via `walk_iife_callee_body`, so IIFEs inside an eagerly-called
top-level function no longer drop their identifier reads. Addresses Codex
P2 #3311274924 and Cursor #3311313158 on PR #302.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 437d4da4c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolves the two new inline bot comments on PR #302 and the follow-on adversarial findings the fix surfaced. * Codex P2 #3311493528 — the `stmt_references_later_class` guard now closes `init_called_symbols` through `fn_body_called_symbols` and checks each function's `fn_body_symbol_refs` for class refs, so an initializer that eagerly calls a function whose body reads a later class is no longer hoisted above that class. * Cursor Low #3311551145 — `collect_expr_symbols` gains explicit `AssignmentExpression` / `UpdateExpression` arms plus `collect_assignment_target_symbols` and siblings, so identifier refs on assignment-target lvalues (including pattern targets with defaults and member targets) flow into the dependency graph. * Cascade un-planning ("Step 2c"): when a planned statement's full dep closure reaches a top-level binding that isn't planned at an `insert_at` ≤ the dependent's `insert_at`, the dependent is dropped and the loop iterates to a fixed point — prevents leaving caller hoists stranded when a transitive dep was guard-skipped or planned only for a later class. * Function-valued bindings indexed: each per-declarator `const f = () => …` / `function() { … }` populates `fn_body_*` maps so the guard chases through them like a function declaration. Covers single- and multi-declarator forms. * Indirect call shapes: `fn.call(...)`, `fn.apply(...)`, and `fn.bind(...)()` recorded as eager invocations of `fn` at every call/new visit site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 375382728c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolves the new inline bot comments on PR #302 and the follow-on adversarial findings the fix surfaced. * Codex P2 #3311913006 — when a popped symbol both has `symbol_to_stmt` and `fn_body_symbol_refs`, the BFS branch now plans the statement AND chases body refs (or defers them via `deferred_fns` when the binding isn't yet eagerly called). * Cursor Low #3311962888 — `topological_order` no longer takes the global `combined_eagerly_called`; per-S `stmt_eager_sets` and a shared `stmt_fn_valued_bindings` map drive the dep-edge construction, matching the cascade's reasoning. Adversarial follow-up: the safe-skip guard and cascade un-planning were blind to fn-valued bindings whose arrow body reads a class — for `@Component({providers: make()}) class C {} const make = () => C;` the planner hoisted `make` above `C` and Ivy's eager `make()` then hit TDZ on `C`. The fix lifts `stmt_fn_valued_bindings` to right after `collect_top_level_bindings` and folds each statement's eagerly-called fn-valued binding bodies into both the per-class safe-skip closure (via the BFS's `eagerly_called`) and the cascade closure (via `combined_eagerly_called`), keeping all three passes symmetric. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3c38d26a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* Codex P2 #3312108552 — top-level class declarations are now indexed into `fn_body_symbol_refs` / `fn_body_called_symbols` (with `include_constructor: true`) via a new `walk_class_eager_parts` helper. The helper covers the union of class-definition-time eager refs (super_class, computed keys, static field / accessor initializers, static blocks) and `new`-time eager refs (constructor body + parameter defaults, instance field / accessor initializers). The BFS body-chase branch then chases through `new ClassName()` callers in hoisted initializers — `const PROVIDERS = [new S()]` no longer hoists past `class TestComponent` when `S`'s constructor reads a later const. * Codex P2 #3312108558 — `collect_expr_symbols`'s previously-opaque `E::ClassExpression` arm now calls `walk_class_eager_parts` with `include_constructor: false`, so the eager parts of an inline class expression (super_class, computed keys, static initializers, static blocks) feed the dependency graph. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de5f1cc83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…te tags
* Codex P2 #3314767088 — `FunctionBodyIdentVisitor::visit_class` was a
no-op, dropping inline `class extends TOKEN {}` defined inside an
eagerly-called function body. It now calls `walk_class_eager_parts`
with `include_constructor: true` so super_class, computed keys,
static initializers/blocks, and (conservatively) constructor body
refs feed `fn_body_symbol_refs` / `fn_body_called_symbols`.
* Codex P2 #3314767091 — `record_direct_callee` only recognised bare
identifiers under parens / TS wrappers. Restructured as an iterative
worklist that also descends into both branches of
`ConditionalExpression` / `LogicalExpression` and the last expression
of a `SequenceExpression`, so callees like `(cond ? makeA : makeB)()`
and `(makeA || makeB)()` enter the eager-call set.
* Cursor Low #3314770575 — `FunctionBodyIdentVisitor` gained
`visit_tagged_template_expression`, recording the tag via the same
direct/indirect/bind callee helpers as `visit_call_expression` so
`tag\`…\`` inside a top-level function body chases the tag's body
refs.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 174c796c1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`collect_expr_symbols::E::TaggedTemplateExpression` only called `record_direct_callee`, while the call/new arms and the body visitor's override already invoke all three callee helpers. Decorator metadata like `make.bind(null)\`...\`` or `make.call(this)\`...\`` therefore recorded `make` as a value reference but never as an eager callee, so its body wasn't chased and a later-declared binding it reads stayed below the class. Now mirrors call/new: direct + indirect + bind. Closes Cursor Low #3314809112 and Codex P2 #3314810080. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 730983c3c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The BFS branch for a `symbol_to_stmt` hit ran the fn-valued binding
body-chase only AFTER the `stmt_start < class_body_end` early-continue,
so a `const make = () => [{ provide: TOKEN }];` declared *before* the
decorated class slipped past the chase entirely. `make`'s binding is
indeed initialized at class-eval time, but the arrow body still fires
when the decorator calls `make()` — and its later-declared reads stay
in TDZ. Moved the body-chase block to BEFORE the early-continue so it
runs regardless of stmt position; removed the now-duplicate post-plan
copy. Closes Codex P2 #3314836115.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4934906fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Removes trailing `See PR #302 ...`, `Regression for/test for ... review #...`, `Round N follow-on review`, `Codex P2/P3 review Finding N`, etc. — the references that belong in PR descriptions and rot as the codebase moves. Substantive technical justifications kept; only the bookkeeping tails removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73482730d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7348273. Configure here.
* `record_indirect_callee` / `record_bind_callee` previously required the static-member receiver to be a bare identifier, missing shapes like `(cond ? makeA : makeB).call(null)` and `(cond ? makeA : makeB).bind(null)()`. Both now delegate the receiver descent to `record_direct_callee`, which already peels parens / TS wrappers and walks conditional / logical / sequence branches. * The topo-precompute eager set was closed under `fn_body_called_symbols` BEFORE folding in fn-valued binding symbols, while the cascade did the opposite order. Reordered the precompute to match the cascade so both passes expand through the same transitive callees and dependency edges through fn-valued bindings stay visible to the topological sort. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9575f2cb19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`FunctionBodyIdentVisitor::visit_function` no-opped on every nested
function, so a local declaration like
`function outer() { function inner() { return TOKEN; } return inner(); }`
called from an eagerly-evaluated body never contributed `TOKEN` to the
eager surface — `inner` is a local symbol that's not indexed in any
`fn_body_*` map, so `close_eagerly_called` can't chase through it. The
visitor now walks the body and parameter defaults of any nested
`Function` with `id: Some(_)`. Anonymous functions and arrow expressions
remain lazy.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e5074478f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`FunctionBodyIdentVisitor` skipped arrows entirely, so a local binding like `const inner = () => TOKEN;` inside an eagerly-called body left `inner` recorded as a callee but with no body to chase — `TOKEN` stayed below the class. Added a per-visitor `local_fn_bodies` map populated by a new `visit_variable_declarator` override that indexes arrow/function inits via a scratch walk. At each `CallExpression` / `NewExpression` / `TaggedTemplateExpression` we resolve every reachable callee identifier (through parens, TS wrappers, conditional / logical / sequence branches, `.call` / `.apply` receivers, and the inner call of `.bind(...)()`) and fold the indexed body's refs / callees into the eager surface, with a `folded` guard for self- and mutually-recursive arrows. Value-passed arrows that are never invoked inside the body (`useFactory: lazy`) stay lazy because the fold only fires at call sites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7df59fff65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The prior round made `FunctionBodyIdentVisitor::visit_function` walk
named nested function bodies unconditionally, which broke laziness — an
uncalled `function unused() { return TOKEN; }` declared inside an
eagerly-called helper folded `TOKEN` into the eager surface and could
hoist it above the class for no reason. Mirror the local-arrow path:
`visit_function` now indexes the function into `local_fn_bodies` and
defers the fold to call sites. A new `visit_function_body` /
`visit_block_statement` pre-pass indexes hoisted function declarations
before source-order walking so a call that textually precedes its
declaration still resolves.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Summary
const/let/vardeclarations referenced by Angular decorator metadata when they're declared after the decorated class, matching Angular's official compiler.ReferenceError: Cannot access 'TOKEN' before initializationthrown at module load when@Component({ providers: [{ provide: TOKEN, useValue: 1 }] })is followed byconst TOKEN = …;(the reported repro from No TDZ-safe hoisting of consts referenced by emitted Ivy definitions #287).crates/oxc_angular_compiler/src/component/hoist.rsbuilds a(delete original, insert before class)edit pair per referenced binding. Edits are merged into the existingtransform_angular_fileedit pipeline; multiple hoists targeting the same class are bundled into one insert to preserve source order.Closes #287.
What changes
component/hoist.rs(new):collect_hoist_editswalks each Angular-decorated class's decorator argument for bare identifier references, stops at function/arrow/class bodies (lazy refs likeuseFactory: () => DEPdon't trigger a hoist), looks up the referencing identifier in a top-levelVariableDeclarationmap, and schedules a hoist if the declaration starts after the class body end. Shared consts hoist once, ahead of the earliest referencing class.component/transform.rs: appends the hoist edits to the wider edit set right beforeapply_edits. Hoist inserts usepriority = 5so they land above the compiler's own pre-class declarations at the same offset.component/mod.rs: registers the new module.What is intentionally NOT hoisted
const { a } = x;) — moving them could change observable side-effect order on the RHS.Test plan
cargo test -p oxc_angular_compiler— 2,461 tests pass (340 integration including 8 new tests for the bug).cargo run -p oxc_angular_conformance— 1,252/1,252 (100%).pnpm test(NAPI vitest) — 189/189 pass.cargo fmt --all -- --check— clean.New tests (
tests/integration_test.rs)component_providers_const_after_class_is_hoisted— exact repro from the issue.component_view_providers_const_after_class_is_hoisted—viewProvidersparity.component_multiple_provider_consts_after_class_are_hoisted_in_order— relative-order preservation.component_provider_const_before_class_is_not_hoisted— guards against pointless rewrites.component_use_factory_dependency_const_is_hoisted_when_referenced_at_top_level—deps:array entries hoist.component_shared_provider_const_is_hoisted_once_for_multiple_classes— dedup ahead of earliest user.component_const_referenced_only_inside_factory_body_is_not_hoisted— guards against over-hoisting.🤖 Generated with Claude Code
Note
Medium Risk
Changes module evaluation order for real Angular components via a large, conservative hoist pass; incorrect edge-case handling could skip needed hoists or leave TDZ, but behavior is gated and aligned with Angular’s compiler.
Overview
Adds TDZ-safe hoisting so top-level
const/let/varbindings referenced from Angular decorator metadata (e.g.providers: [{ provide: TOKEN }]) are moved above the decorated class when they appear later in the file—matching the official Angular compiler and fixing module-loadReferenceError(#287).The new
component/hoist.rsbuilds delete/insertEdits viaoxc_semanticsymbol resolution: it walks decorator arguments (not lazy factory/arrow bodies), plans hoists per decorated class with transitive initializer dependencies, topologically sorts hoisted statements, and applies safe-skip / cascade un-plan rules so hoisting does not introduce new TDZ on classes or unreachable deps.transform_angular_fileappends those edits beforeapply_edits, with priority skew so hoists sit above the class without clobbering compiler insertions; a cheapprogram_has_angular_decorated_classgate skips the semantic pass when no Angular-decorated classes exist.Workspace deps add
oxc_ast_visitandoxc_syntaxto the compiler crate.Reviewed by Cursor Bugbot for commit 342d625. Bugbot is set up for automated code reviews on this repo. Configure here.