Repository navigation
fix(e2e-compare): actually compare setClassMetadata between compilers - #562
Conversation
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: 2816c2ed13
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0e52ea4a3
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98873950b2
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12ddeed3f6
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6072510a1
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7620855cf7
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1c90ba95d
ℹ️ 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".
`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.
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.
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.
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.
…nces 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.
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.
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`.
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.
e051291 to
fa999bc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa999bcb55
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45dbfdd300
ℹ️ 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 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25a3ff86cc
ℹ️ 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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 442f1b9535
ℹ️ 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 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c84ac9e6f
ℹ️ 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".
`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.
Closes #561. Stacked on #560 (contains its fixture + known-differences entry).
extractClassMetadataCallsmatchedi\d+\.ɵɵsetClassMetadata— both compilers emiti0.ɵsetClassMetadata(singleɵ), soclassMetadataDiffswas always empty and the comparator has been dead code.Changes
src/compare.ts: fix the regex (also coversɵsetClassMetadataAsync), and compare each argument as a normalized AST viaparseSync+normalizeAstinstead of whitespace-collapsed strings — quote style, indentation, and trailing commas no longer produce diffs.fixtures/runner.ts+known-differences.ts:KnownDifferencegainsmetadataDiffs(Class.fieldallowlist, e.g.AliasedService.setClassMetadata) andtestFixturefails on undocumented metadata diffs — implementing the allowlist mechanism the test(e2e): record the aliased-@Injectable emit difference #560 review suggested.Triage after enabling
Enabling extraction alone surfaced 79 mismatches; with AST normalization only 2 real diffs remain, both in pre-existing entries:
edge-cases/single-custom-decoratorandedge-cases/multiple-custom-decorators— ngtsc leaves those classes uncompiled so Oxc'ssetClassMetadatais a genuine extra, now declared viametadataDiffs.edge-cases/aliased-injectablealso declares its extra metadata explicitly.Verified
pnpm test(napi/angular-compiler): 401/401;oxfmtclean.