compat-eslint: --debug-estree + JSX attribute parent fix + shape-table refactor#88
Merged
Conversation
`tsslint --debug-estree` (or env TSSLINT_DEBUG_ESTREE=1 for callers
outside the CLI) prints, after lint completes, the actual ESTree
node types lazy-estree materialised this session and their counts —
sorted by count desc, then alphabetical:
estree node types (58 kinds, 1,611 nodes)
Identifier 296
MemberExpression 124
BlockStatement 118
CallExpression 117
VariableDeclaration 97
...
Useful for: spotting which kinds dominate conversion volume on a
real codebase (informs where to focus lazy-estree perf), confirming
that lazy materialisation is actually skipping unused subtrees
(missing kinds aren't in the table because no rule visited them),
and seeing at a glance whether a rule's selector pulled in a
surprising amount of TS-only kinds.
Counter lives on globalThis under Symbol.for('@tsslint/compat-eslint:
node-type-counts'). Reason: a user's project typically resolves
@tsslint/compat-eslint against ITS node_modules, while a CLI-side
require() lands on the CLI's neighbour copy — different module
instances, different Map references. globalThis closes the gap so
the CLI sees what the user's instance populated.
Cost when off: one boolean check per LazyNode construction, no
allocation. Cost when on: one queueMicrotask per construction (need
to defer because `readonly type = '...'` field initialisers run
AFTER super(), so reading this.type from the base ctor synchronously
sees undefined).
Surfaced by --debug-estree on Dify web/ (5867 files,
react-x/no-leaked-conditional-rendering): the converter table showed
9,659 TSJsxAttributes nodes — a SyntaxKind that has no real ESTree
counterpart. typescript-estree elides the JsxAttributes container
and exposes attributes directly via JSXOpeningElement.attributes.
In lazy-estree, materialise's bottom-up parent walk wasn't aware of
this shape. Walking up from a JsxExpression inside a JSX attribute
(`<X prop={x} />`) hit JsxAttributes on the way to JsxOpeningElement,
and lazy-estree fell through to GenericTSNode → 'TSJsxAttributes'.
The skip list at the top of the parent walk already had SyntaxList,
CaseBlock, NamedImports, ImportClause, etc. — JsxAttributes was
just missing.
Adding the skip:
- JSXAttribute.parent now reads as JSXOpeningElement directly
(matches typescript-estree shape; nothing produces TSJsxAttributes)
- 9,659 fewer GenericTSNode allocations on Dify cold (-4.8% of
total materialised node count)
- All compat-eslint test suites still pass — predicate coverage,
selector analysis, lazy-estree, ts-ast-scan, scope-compat,
compat-pipeline (152/152 ESTree types covered).
Wall-clock impact in the noise (allocation savings small per node);
memory peak drops by ~1.5MB.
The previous commit (7d3c25e) skipped JsxAttributes in materialise's parent walk to eliminate phantom 'TSJsxAttributes' nodes — a type that doesn't exist in typescript-estree's shape (attributes go directly on JSXOpeningElement.attributes). The skip alone fixed the parent for non-self-closing tags but introduced a regression for self-closing: bottom-up materialise of an attribute under `<Foo prop={x} />` landed `JSXAttribute.parent = JSXElement`, because JsxSelfClosingElement materialises as JSXElement (JSXOpeningElement is synthetic and only exists as a getter). Fix: add a wrapper-route in findWrapperRoute for JsxAttribute / JsxSpreadAttribute when the JsxAttributes container's parent is an opening element (self-closing or not). The route triggers the owner's `attributes` getter (or `openingElement.attributes` for self-closing), which builds the synthetic JSXOpeningElement + attribute children and registers them in the cache. After the trigger fires, materialise's normal cache lookup finds the correctly-parented attribute and returns it. Net materialised node count goes UP slightly on JSX-heavy codebases (Dify: 191k → 203k after both fixes vs 201k baseline) because we now build the synthetic JSXOpeningElement + the JSXAttribute children for self-closing tags. The previous shape was wrong; the new count reflects what eager (typescript-estree) already produces. No more 'TSJsxAttributes' in any output. Tests (7 new bottom-up cases + 1 catch-all walk): - bottom-up from {x} attr value lands JSXAttribute.parent on JSXOpeningElement for both self-closing and non-self-closing - sibling attrs share the same JSXOpeningElement parent (cache) - spread attribute walks correctly under both shapes - JSX child {x} (NOT in attribute) unaffected by the wrapper — pinning that the route doesn't over-fire - nested JSX in attribute value walks each level correctly - whole-tree visitor-keys walk over a comprehensive fixture sees no node with type 'TSJsxAttributes'
The counter shipped in 1a9c5a3 had no direct tests — the visible output was only inspected by hand against tsslint dogfood + Dify. Pin the four invariants via spawned child processes (env var is read at module load, so toggling in-process isn't workable): - off-path: TSSLINT_DEBUG_ESTREE unset → counter stays empty (boolean-check-only fast path; no allocations) - on-path: counter populates with the expected node types (Program / JSXOpeningElement / JSXAttribute) and notably does NOT contain 'TSJsxAttributes' — pins the parent-skip invariant from 5a9e582 from the counter side too - globalThis sharing: a second require() (separate module instance, mimicking project vs CLI module-resolution split) sees the same populated map via Symbol.for(...) global key - resetNodeTypeCounts(): clears the map between sessions Each child driver triggers a visitor-keys walk over the produced ESTree so getters fire (otherwise convertLazy alone only counts the eager Program node). Counts are read on the next microtask so deferred queueMicrotask bumps in LazyNode's ctor have landed.
Hardens the test suite against the bug class fixed earlier in this
PR (JSXAttribute.parent corruption via phantom TSJsxAttributes).
Master's tests had three blind spots:
1. lazy-estree.test's `compare()` walks tree TOP-DOWN through child
getters, where parent is set correctly even on master. Bottom-up
`materialise(tsNode)` (what tsScanTraverse does on selector match)
was never exercised. The function also explicitly skips the
`parent` field — so even top-down parent-chain corruption would
not have failed parity.
2. compat-pipeline's JSXAttribute listener pushed `attr:${n.name}`
events but never read `n.parent`, leaving the listener-API parent
contract unverified.
3. Nothing checked the invariant "no node has a 'TS<KindName>' type
for kinds typescript-estree elides" — the property the
TSJsxAttributes regression directly violated.
Three new test blocks address each:
Bottom-up parity sweep (lazy-estree.test): 9 JSX-attribute fixtures
(self-closing, non-self-closing, spread, sibling attrs, multi-attr
mixed, fragment, nested attr value, deeply nested), each walks every
TS node, runs `lazy.materialise(tsNode, ctx)`, and compares the
resulting type / parent.type / parent.parent.type against eager's
`astMaps.tsNodeToESTreeNodeMap` lookup. Eager parents are stitched
ourselves (typescript-estree's astConverter doesn't set parent —
ESLint's SourceCode does that downstream). 117 type asserts + 108
parent asserts on the fix branch; 7 unique mismatches on master.
Phantom-types invariant (lazy-estree.test): 5 comprehensive fixtures
(jsx-attrs-everything, ts-everything, imports-everything, patterns-
everything, jsx-fragment-mix), each walks via visitor-keys and ALSO
bottom-up materialises every TS node that has an eager counterpart.
Asserts no produced type starts with 'TS' AND isn't in typescript-
estree's published TS-* type list. The published list is hard-coded
inline (~75 entries) so the test fails immediately if a future
PR-added GenericTSNode fallback emits a name not in eager's spec.
JSX listener parent assertion (compat-pipeline): test #7's
JSXAttribute / JSXSpreadAttribute listeners now also assert
`parents[0] === 'JSXOpeningElement'` — the contract any
jsx-a11y / react/jsx-* rule relies on. The contract is correct in
spec; whether each listener path independently fails on master
depends on whether sibling listeners pre-warm the cache top-down,
but the assert still locks the contract from inside the actual
ESLint listener API surface.
Out of scope (real but unrelated impedance with eager that the broad
sweep also surfaces — left as separate follow-ups):
- export wrapper identity (lazy maps the TS node to ExportNamed/
DefaultWrapper, eager maps to the inner declaration)
- chain-expression wrapping for `a?.b?.c`
- destructuring with defaults
- CatchClause param lifted from ts.VariableDeclaration shim
- TSStringKeyword / TSVoidKeyword direct-on-Signature
Last commit's bottom-up parity sweep was deliberately scoped to JSX
attribute fixtures because the broader fixture set surfaced 5 real
impedance mismatches against typescript-estree that needed their own
fixes. This commit closes them all out and expands the sweep to
exercise each:
3 lib changes (real bugs):
- PropertySignature/MethodSignature/CallSignature/ConstructSignature/
IndexSignature/FunctionType/ConstructorType added to
WRAPPER_ROUTE_PARENT_BITMAP, with corresponding findWrapperRoute
cases that route through `typeAnnotation` / `returnType` getters.
Fixes TSStringKeyword/TSVoidKeyword/etc. landing directly on the
Signature parent instead of the synthetic TSTypeAnnotation
wrapper that typescript-estree always emits.
- VariableDeclaration-inside-CatchClause added to materialise's
walk-up skip list (mirrors existing tsScanTraverse skip). TS
wraps the catch param in a VariableDeclaration shim that ESTree
elides — `CatchClause.param` is the Identifier directly. Without
the skip, bottom-up materialise of the param landed `parent` on
a phantom VariableDeclarator → VariableDeclaration chain.
- The shorthand-with-default destructure drill (existing logic for
`{ x = 1 } = o` lifting Identifier.parent to AssignmentPattern)
now also fires when the innermost child is the BindingElement's
initializer (the default value), not just the name. Without this,
the default-value Literal landed on Property directly, skipping
the AssignmentPattern wrapper between.
2 test-side equivalences (eager quirks, not lazy bugs):
- Export wrappers: lazy maps `ts.<X>Statement` (when exported) to
ExportNamed/DefaultDeclaration; eager maps to the inner
declaration. The user-visible parent chain (inner.parent ===
wrapper, wrapper.parent === Program) matches on both sides.
Drill into `lazyNode.declaration` to align comparison.
- Optional-chain scaffolding: typescript-estree emits one
ChainExpression per optional level, but only the outermost is
structurally in the tree (setEagerParents only stitches reachable
nodes). Skip rows where eager.parent is null AND eager.type !==
'Program' — those are off-tree scaffolding mappings with no
position to compare against. Captures the same idea for
shorthand-with-default Identifier (one TS token, two ESTree
positions: Property.key vs AssignmentPattern.left).
Sweep grew from 9 → 15 fixtures (export-named-const, export-default-fn,
optional-chain-call, destructure-defaults, try-catch-param, ts-interface-
keyword-signature added). 169 type asserts + 154 parent asserts, all
clean. Removed the "out of scope" comment since the items it deferred
are now covered.
Other compat-eslint suites still pass: 152/152 predicate coverage,
24/24 scope-compat fixtures clean, all ts-ast-scan / selector-analysis /
compat-pipeline cases. self-lint clean across all 6 packages.
… tables
The bug class fixed earlier in this branch (top-down getters and
bottom-up materialise drifting on shape rules) had a root cause:
shape knowledge was scattered across multiple if/else cascades. Each
new typescript-estree shape divergence needed a new if-block in
several places — easy to forget one and ship a parity break.
This commit collapses the bottom-up shape rules into two declarative
tables, the single source of truth for each category:
SKIP_AS_PARENT (10 entries):
Structural-only TS kinds with no ESTree counterpart in their
usual position. The walker skips past so the child's parent
resolves to the next-level real ESTree ancestor. Replaces the
7-condition if-cascade in materialise's walk-up.
Adding a new structural-skip TS kind = one new line.
TYPE_SLOT_TRIGGERS (12 entries):
Per-parent-kind callback that drills the parent's getter chain
to materialise the synthetic TSTypeAnnotation wrapper around a
`.type` slot. Replaces 5 separate if-blocks in findWrapperRoute
(one per parent kind family). Adding a new TS parent with a
.type slot wrapped in TSTypeAnnotation = one new line.
WRAPPER_ROUTE_PARENT_BITMAP is now derived from
TYPE_SLOT_TRIGGERS' keys plus a small list of pattern-position
parents — the bitmap can never drift from the table.
Net -56 lines. All 6 compat-eslint test suites still pass:
predicate-coverage 152/152, scope-compat 24/24, plus
lazy-estree.test's full 15-fixture bottom-up parity sweep + phantom-
types invariant. self-lint clean.
Out of scope (different mechanism, less amenable to a flat table —
left as separate follow-ups):
- findWrapperRoute's pattern-literal / type-arg / JSX tag-name
cases (each has its own match logic + drill path)
- Wrapper-drill cases in materialise's cachedAnc branch (Class /
Interface / Enum members → body, MethodDef → value, etc.)
…iven Continues the bottom-up shape consolidation. Two more cascades collapse into declarative tables: TYPE_ARG_HOSTS (8 entries) — TS parent kinds that expose `typeArguments`. Replaces findTypeArgRoute's 30-line switch with an O(1) lookup + a single trigger that drills `typeArguments.params` (or `openingElement.typeArguments.params` for self-closing JSX). Adding a new TS host kind with typeArguments = one new line. WRAPPER_DRILLS (7 entries) — when materialise's walk-up hits a CACHED ancestor whose ESTree shape WRAPS the actual parent (synthetic ClassBody / TSInterfaceBody / TSEnumBody / AssignmentPattern / FunctionExpression-via-MethodDefinition.value etc.), drill into the right slot. Replaces an 80-line if/else cascade with a 25-line table walk. Net -24 lines on top of the previous -56. Total -80 lines from the two-phase refactor. All bottom-up shape knowledge — skip rules, type-slot wrappers, type-arg routes, wrapper drills — now lives in one file region (~150 lines of declarative tables) instead of being scattered across ~400 lines of cascading conditionals. Adding a new typescript-estree shape divergence to the bottom-up walk is now a one-line table edit per category. The class of bugs where wrapper-route logic forgot to handle a new kind (TSJsxAttributes, self-closing JSXOpeningElement) is structurally prevented because the same table feeds both the bitmap (fast-path filter) and the dispatch (slow-path lookup). All compat-eslint suites still pass: 152/152 predicate coverage, 24/24 scope-compat, lazy-estree's full bottom-up parity sweep across 15 fixtures + phantom-types invariant.
Phase 3 of the shape-knowledge consolidation: hand-written LazyNode
subclasses for purely mechanical shapes (those whose getters were
all `get x() { return this._x ??= convertChild(this._ts.field, this); }`)
are replaced by entries in a SHAPES registry. A factory function
`defineShape` builds the class + lazy memoised getters from a
declarative `{ type, slots }` definition.
This closes the bug class: the same SHAPES table feeds both
top-down getter materialisation (via the factory's generated
getters) AND bottom-up materialise dispatch (via SHAPE_CLASSES.get
in convertChildInner). They cannot drift on these kinds.
28 shapes migrated in this batch — IfStatement, ReturnStatement,
TSUnionType, TSIntersectionType, TSArrayType, TSTypeLiteral,
TSIndexedAccessType, TSQualifiedName, TSTypeAssertion,
TSSatisfiesExpression, TSConditionalType, TSInferType, TSModuleBlock,
Decorator, ThrowStatement, TryStatement, WhileStatement,
DoWhileStatement, ForStatement, LabeledStatement, AwaitExpression,
TSTupleType, TSOptionalType, TSRestType, ConditionalExpression,
TSNonNullExpression, TSExternalModuleReference, ImportAttribute.
Each replaces a 5-7-line subclass + a switch case in convertChildInner
with one declarative table entry.
Net -140 lines. The SHAPES section grew by ~190 lines (one entry per
shape); 360 lines of subclass + switch boilerplate disappeared.
NOT migrated (intentionally — would need factory extensions):
- SK.LiteralType: convertLiteralType has a `null`-keyword special
case that synthesises a bare TSNullKeyword instead of TSLiteralType.
- SK.ObjectLiteralExpression / ArrayLiteralExpression: the dispatch
picks ObjectPattern/ArrayPattern vs ObjectExpression/ArrayExpression
based on `allowPattern` flag — pattern-context dispatch, not pure
kind dispatch. Stays in convertChildInner's switch.
- Subclasses with custom constructors that compute primitive
fields from modifiers/tokens (VariableDeclaration's `kind`,
ClassDeclaration's `abstract`, etc.) — would need the factory
to support `consts` callbacks. Future migration batch.
All compat-eslint suites still pass (152/152 predicate coverage,
24/24 scope-compat, lazy-estree's bottom-up parity sweep + phantom-
types invariant, full real-source parity 33/33). self-lint clean.
ShapeDef gains an optional `consts` callback that derives readonly
fields from the TS node in the constructor (after super()). Lets
classes whose only non-mechanical part was computing fields from
TS modifier tokens / asterisks / etc. join the table.
Migrates 2 more shapes:
- TSTypeParameter (`const`, `in`, `out` from modifier kinds)
- YieldExpression (`delegate` from asteriskToken)
30 shape entries total now (28 mechanical + 2 with consts).
Net -16 lines on top of the previous batch.
Stopping point for the migration sweep — the remaining 123
LazyNode subclasses each have one or more of:
- custom converter functions (convertJSXTagName,
convertTypeAnnotation, convertNamedTupleMember, etc.)
- conditional getter logic (Block.body's directive handling,
LiteralType's null special case)
- synthetic intermediate nodes (TSTypeAnnotation wrappers,
JSXOpeningElement for self-closing)
- multi-field constructor logic that doesn't fit a flat consts
callback (computing via combined modifier scans + flag bits)
Each adds non-trivial factory complexity for a single-class win;
the table-driven payoff is in the bulk mechanical cases that this
PR has already covered. Future migrations extend the factory or
stay hand-written, depending on the cost/benefit per case.
johnsoncodehk
changed the title
johnsoncodehk
added a commit
that referenced
this pull request
May 2, 2026
Merged
3 tasks
johnsoncodehk
added a commit
that referenced
this pull request
May 2, 2026
Addresses review on PR #89: defineShape<TsT extends ts.Node>(...): each call site now binds the TS node type via generic. tsField is constrained to keys of TsT, so typos like `tsField: 'thenSttement'` fail at compile time. consts callback parameter is auto-inferred (no per-shape `(tn: ts.X) =>` annotation needed). 46 defineShape sites updated with their concrete TS type parameter. All build + tests still pass. Two focused factory unit tests in lazy-estree.test: - TSTypeQuery.typeArguments returns undefined (not null) for absent slot, AND is an own-property (eager parity), AND memoises on second read. - ReturnStatement.argument returns null (not undefined) for bare `return;` — the default whenAbsent='null' is honoured. Also confirmed via Dify bench that there's no perf regression — cold ~11s (improved vs PR #88's 13s baseline; the earlier 14s reading was disk warmup), warm ~6s. The factory's hidden-class shape transitions land on V8's monomorphic happy path.
This was referenced May 2, 2026
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.
Why
lazy-estree carried shape knowledge (skip rules, wrapper-routes, drill-throughs encoding typescript-estree's deviations from raw TS-AST) in two separate code paths: top-down getters in 152 LazyNode subclasses, and bottom-up
materialise(the parent walk used when a TS selector matches deep inside). New typescript-estree shape divergences had to be added to BOTH places — easy to forget one and ship a silent parity break.Concrete instance found while building
--debug-estree: on Dify (5867 files, one type-aware rule) the converter emitted 9,659 nodes of type'TSJsxAttributes'— a type that doesn't exist in typescript-estree's spec. Any rule registering aJSXAttributeselector + readingn.parent.type(jsx-a11y/*,react/jsx-no-bind, etc.) silently mis-fired becausen.parent.type === 'JSXOpeningElement'checks evaluated false.This PR ships the debug flag, fixes the JSX bug, locks the bug class out via tests, and consolidates shape knowledge into a single declarative source of truth.
What's here
--debug-estreeflagPrints the actual ESTree node types lazy-estree materialised this session, sorted by count desc. Counter lives on
globalThis[Symbol.for('@tsslint/compat-eslint:node-type-counts')]so the CLI reads what the user-project's compat-eslint instance populated (different module instances per-project under pnpm). Off-path cost: one boolean check per LazyNode construction; on-path: one queueMicrotask (deferred because the subclass'sreadonly type = '...'field initialiser runs after super()).Lazy-estree shape now lives in declarative tables
All bottom-up shape rules collapsed into a small set of typed tables — single source of truth per category. Adding a new typescript-estree shape divergence means one new table line, not edits in multiple if-cascades.
SKIP_AS_PARENT(10 entries)TYPE_SLOT_TRIGGERS(12 entries).typeslot wraps in synthetic TSTypeAnnotation (PropertySignature / Parameter / FunctionDeclaration return / etc.)TYPE_ARG_HOSTS(8 entries)typeArguments(TypeReference / NewExpression / CallExpression / JsxOpeningElement / etc.)WRAPPER_DRILLS(7 entries)WRAPPER_ROUTE_PARENT_BITMAPTYPE_SLOT_TRIGGERS.keys()+ pattern-position parents — cannot driftSHAPES(30 entries)The
defineShape({ type, slots, consts? })factory builds a LazyNode subclass with memoised getters from a declarative spec. 30 mechanical kinds migrated (28 withslotsonly, 2 withconstscallback for modifier-derived fields likeTSTypeParameter.const/in/out).Remaining 123 hand-written subclasses each carry custom converter logic, conditional getters, or synthetic intermediates — migrating them adds non-trivial factory complexity for a single-class win. They stay hand-written; the parity test below catches drift.
JSX attribute parent — fixed
JsxAttributesadded toSKIP_AS_PARENT, plus a wrapper-route that drills throughattributes(oropeningElement.attributesfor self-closing) soJSXAttribute.parentlands onJSXOpeningElementfor both shapes. typescript-estree parity restored.Tests pin the invariants
lazy-estree.test): 15 fixtures × every TS node ×lazy.materialise× compare type / parent.type / parent.parent.type against eager'sastMaps.tsNodeToESTreeNodeMap. 169 type asserts + 154 parent asserts. Eager parents stitched in-test (typescript-estree's astConverter doesn't set parent — ESLint's SourceCode does that downstream). Catches any drift between top-down and bottom-up.'TS<KindName>'invariant: 5 comprehensive fixtures × top-down + bottom-up materialisation × asserts no produced type starts with'TS'AND isn't in typescript-estree's spec list (~75 entries inline). Locks out the bug class that started this PR.--debug-estreecounter behaviour: 13 checks via spawned child processes — env unset = empty, env set = populates, globalThis sharing across module instances,resetNodeTypeCounts()clears.parents[0] === 'JSXOpeningElement'for bothJSXAttributeandJSXSpreadAttributeselectors, exercising the actual ESLint API surface.Numbers (Dify web/, 5867 files,
react-x/no-leaked-conditional-rendering)TSJsxAttributes)TSJsxAttributescountJSXOpeningElementcountJSXAttributecountNet node count UP by 1,751: synthetic JSXOpeningElement + attribute children for self-closing tags now built (previously skipped). Wall-clock unchanged (cold ~13s, warm ~6.5–7.5s).
Test plan
pnpm buildclean--debug-estree --forcematches expected counts (no phantom types, JSXOpeningElement count = total tags with{x}attrs)