Repository navigation
test(e2e): record the aliased-@Injectable emit difference - #560
Merged
Merged
Conversation
`import { Injectable as X }` + `@X(...)` compiles to a working `ɵfac`
and setClassMetadata under Oxc; ngtsc's `needsFactory` check compares
the written decorator name, so it emits only `ɵprov` pointing at a
factory it never generated — an upstream bug (issue #507). Oxc keeps
its output; this adds a fixture covering the case and documents the
difference in known-differences.ts.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: f55828e4fb
ℹ️ 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".
Brooooooklyn
added a commit
that referenced
this pull request
Oct 8, 2026
`extractClassMetadataCalls` matched `i\d+\.ɵɵsetClassMetadata` but both compilers emit `i0.ɵsetClassMetadata` (single ɵ), so classMetadataDiffs was always empty — the comparator has been dead code. Fixes: - the regex (also covering `ɵsetClassMetadataAsync`) - `normalizeMetadataString` now parses each argument and compares normalized ASTs, so quote style, indentation and trailing commas no longer produce noise (this turned out to be the bulk of the 79 diffs enabling extraction surfaced) - `KnownDifference` gains `metadataDiffs` (Class.field allowlist) and `testFixture` fails on undocumented metadata diffs, per the Codex review comment on #560 Two pre-existing custom-decorator entries now declare their extra setClassMetadata (Oxc compiles the class where ngtsc leaves it uncompiled), and `edge-cases/aliased-injectable` declares its AliasedService.setClassMetadata diff instead of passing implicitly. Closes #561.
Brooooooklyn
added a commit
that referenced
this pull request
Oct 8, 2026
…#562) * fix(e2e-compare): actually compare setClassMetadata between compilers `extractClassMetadataCalls` matched `i\d+\.ɵɵsetClassMetadata` but both compilers emit `i0.ɵsetClassMetadata` (single ɵ), so classMetadataDiffs was always empty — the comparator has been dead code. Fixes: - the regex (also covering `ɵsetClassMetadataAsync`) - `normalizeMetadataString` now parses each argument and compares normalized ASTs, so quote style, indentation and trailing commas no longer produce noise (this turned out to be the bulk of the 79 diffs enabling extraction surfaced) - `KnownDifference` gains `metadataDiffs` (Class.field allowlist) and `testFixture` fails on undocumented metadata diffs, per the Codex review comment on #560 Two pre-existing custom-decorator entries now declare their extra setClassMetadata (Oxc compiles the class where ngtsc leaves it uncompiled), and `edge-cases/aliased-injectable` declares its AliasedService.setClassMetadata diff instead of passing implicitly. Closes #561. * fix(e2e-compare): parse full setClassMetadata call arguments Two Codex findings on the revived comparator: - ctorParams/propDecorators were sniffed by first character, so an arrow `() => [{ type: Service }]` desynced the parse and both args read as null. The extractor now finds the call's matching paren and splits arguments on top-level commas — any argument shape works. - the callee regex required `i\d+`, missing calls under a reused namespace import (`ng.ɵsetClassMetadata`); it accepts any identifier now. Enabling ctor-param comparison surfaces a real Oxc emit difference: `type:` names the annotation even for type-only symbols where ngtsc emits `undefined` (issue #563) — recorded in three fixtures' metadataDiffs until that is fixed. * fix(e2e-compare): harden setClassMetadata extraction edge cases Three more Codex findings on the comparator: - `ɵsetClassMetadataAsync` (deferred imports) was extracted as a sync entry and overwrote the inner call in the class-keyed map. Async calls now carry `isAsync` + `resolver` and are keyed separately, so a sync-vs-async flip surfaces as missing/extra and the resolver thunk is compared via normalized AST. - `\w+` couldn't capture `$` in class names (`Foo$`) — now the full identifier grammar. - the callee gate now requires the namespace to be an `@angular/core` binding (`import * as X`/`const X = require(...)`) in the emitted file, so a user-side `debug.ɵsetClassMetadata` can't shadow real compiler metadata. * fix(e2e-compare): extract setClassMetadata calls from the AST Two more Codex findings, both rooted in the text scanner: - `findMatchingBracket` stopped at `)` inside regex literals or comments, truncating the argument list and hiding ctorParams/propDecorators diffs. Extraction now walks the parsed AST (`walkAst`) — `CallExpression`s whose callee is a member of an `@angular/core` namespace binding (`import * as X` or `const X = require('@angular/core')` read from `ImportDeclaration`s). Spans are character offsets, so argument text still slices cleanly. - The async wrapper `(B, A) => {...}` versus `(A, B) => {...}` was invisible: only the resolver was compared. Wrappers are now compared after alpha-renaming their parameters (`p0`, `p1`, ...), so param order changes surface while name-only differences don't. The scanner helpers (`findMatchingBracket`, `splitTopLevelArguments`) are gone with no remaining callers. * fix(e2e-compare): preserve absent-arg and multi-call metadata differences Two Codex findings on the setClassMetadata comparator: - `null` and `undefined`/`void 0`/a missing argument are not interchangeable: the runtime guards each field with `!== null`, so an absent argument assigns the field instead of skipping it. The extractor now records the literal absent spelling (`'null'` vs `'undefined'`) instead of collapsing them. - A source file can contain its own `ng.ɵsetClassMetadata(...)` call via the Angular namespace binding; keying the compare maps by class kept only the last call, letting an identical user call mask a divergent generated one. Keys now map to sorted call lists compared pairwise as a multiset. * fix(e2e-compare): keep metadata call order, rename only bound references Two more Codex findings on the setClassMetadata comparator: - Repeated setClassMetadata calls overwrite class fields, so call order is semantically significant. Grouped calls are now compared in execution order instead of being sorted by normalized signature. - The async wrapper alpha-rename renamed every identifier matching a parameter name, including non-reference positions — member properties (`registry.X`), non-computed object keys (`{X: ...}`, the key of a shorthand `{X}`), labels, and `import.meta`. Those positions keep their names now, so wrappers differing only in observable key/property names no longer normalize identically. Also exports `extractClassMetadataCalls` and `compareClassMetadata` for direct probing. * fix(e2e-compare): scope-aware wrapper rename, keep void side effects Two more Codex findings on the setClassMetadata comparator: - The wrapper alpha-rename matched by name only, so a nested binding shadowing a wrapper parameter (inner function param, let/const/class/ function declaration, hoisted var, catch param, class name) had its declaration and references renamed as though bound to the parameter. `renameBoundIdentifiers` now tracks lexical scopes and only renames references actually bound to the wrapper's parameters. - `metadataArgOrAbsent` collapsed every `void expr` to `undefined`. Only the inert spellings (`void 0`, `void(0)`) map to `undefined` now; an operand with side effects (`void recordMetadata()`) keeps its text so it diffs against `undefined`. * fix(e2e-compare): switch discriminant scope, verify metadata allowlist Two more Codex findings: - `renameBoundIdentifiers` removed case-level `let`/`const` bindings for the whole SwitchStatement, including the discriminant — but the discriminant is evaluated in the enclosing scope before the case-block environment exists. The discriminant is now traversed with the original mapping and only `cases` see the shadowed names. - `undocumentedMetadata` checked observed ⊆ declared only, so a fixed metadata bug left its declared `metadataDiffs` entry silently able to re-allow a regression. The check is now bidirectional: declared entries with no observed diff mark the fixture `mismatch` with `staleMetadataFields`, reported alongside stale known differences. * fix(e2e-compare): use collision-proof canonical wrapper parameter names Renaming declarations to p0/p1 before traversing made a canonical name collide with an original parameter named p1, corrupting the shadow set and comparing equivalent wrappers as different. References are now renamed first while scope tracking still sees the original names, the parameter declarations are renamed afterwards, and the canonical spellings are `#p0`, `#p1`, ... — never legal identifiers — so a free `p0` in the body cannot collide either. * fix(e2e-compare): exclude nested sync calls covered by async wrappers The AST walk extracted `ɵsetClassMetadata` calls nested inside a `ɵsetClassMetadataAsync` wrapper argument as independent entries, so equivalent wrappers differing only in parameter spelling (`(A)` vs `(B)`) produced a spurious `setClassMetadata.decorators` diff even though `wrapperNormalized` already covers the nested call under the wrapper bindings. Nested calls inside the wrapper argument are now marked during the async pass and skipped. * fix(e2e-compare): compare metadata call order across classes Per-class grouping made each bucket compare equal even when the two outputs emitted the same calls in a different sequence, and call order is observable when decorator arguments run evaluated expressions. When both outputs hold the same multiset of calls (keyed by class/kind) in a different order, a `setClassMetadata.order` diff is now reported at the first divergent position; differing multisets keep producing only the existing missing/extra diffs. * fix(e2e-compare): keep body bindings out of parameter initializer scope Parameter default initializers evaluate in the parameter scope, but the function branch collected body bindings (hoisted vars and top-level block decls) into the shadowed set before traversing the whole node, so a `let A` in the body hid the outer mapping for `(x = A)` and made a free `A` compare equal to a bound one. Params are now traversed with only the function params/id removed, and body bindings apply to the body traversal only. * fix(e2e-compare): scope static-block vars to the static block `varDeclNames` recursed into class static blocks, hoisting `var` names there into the enclosing function's shadow set, while the StaticBlock branch never collected vars for its own scope. Static blocks are now treated as their own var scope: `varDeclNames` skips them (and class nodes), and the StaticBlock branch collects its hoisted vars alongside its lexical declarations.
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.
Closes #507.
Keeps OXC's output for
import { Injectable as X }+@X(...)— a workingɵfac+setClassMetadata— instead of copying ngtsc's emission, which produces aɵprovthat references aS.ɵfacit never generated (upstream'sneedsFactorycomparescurrent.name === 'Injectable', the written name, inannotations/src/injectable.ts).Changes
edge-cases/aliased-injectablecovering the repro.known-differences.tsentry documentingAliasedService.ɵfacas the deliberate difference.Verified
pnpm compare --fixtures --category edge-cases— 44/44 pass, fixture reported as known-difference.