Graft .gts SourceFiles onto every virtual twin, once per program - #235
Closed
NullVoxPopuli-ai-agent wants to merge 2 commits into
Closed
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for looking into the issue! I tried the changes of this PR (the one in |
NullVoxPopuli-ai-agent
force-pushed
the
fix/virtual-twin-type-identity
branch
2 times, most recently
from
July 6, 2026 14:14
522d4bc to
8aa2b59
Compare
This comment was marked as outdated.
This comment was marked as outdated.
NullVoxPopuli-ai-agent
force-pushed
the
fix/virtual-twin-type-identity
branch
from
July 6, 2026 14:57
d114ce8 to
6568520
Compare
This comment was marked as outdated.
This comment was marked as outdated.
NullVoxPopuli-ai-agent
force-pushed
the
fix/virtual-twin-type-identity
branch
from
July 6, 2026 15:53
6568520 to
cb279ae
Compare
Contributor
🏎️ Benchmark ComparisonParse
Full mitata output
Full mitata output |
A project program can hold one templated file under up to three paths: the .gts root, a .ts virtual created when another file resolves `import './x'` extensionless through the patched fileExists, and (on newer TS/typescript-eslint versions) a .mts twin injected as a root via the patched readDirectory. syncMtsGtsSourceFiles only grafted the first twin it found, preferring .mts. On setups where the .mts root exists, the .ts virtual — the copy every extensionless import actually resolves to — kept its own AST and therefore its own class symbols. A class with a private member is not assignable to itself across two declarations, unions of the "same" type don't dedupe, and typed rules (no-unnecessary-type-assertion, no-redundant-type-constituents) report order-dependent false positives (ember-tooling#229). Graft all twins so every import path reaches the same nodes and one type identity; sharing statements also drops the duplicate ASTs. The graft also ran after every parse, walking the whole program file list each time and re-copying binder state (symbol/locals) onto twins under a live type checker, leaving stale symbols in checker caches — the remaining non-determinism vector. Run it once per program instead. Regression tests are split out to a follow-up PR (they fail without this change by design). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NullVoxPopuli-ai-agent
force-pushed
the
fix/virtual-twin-type-identity
branch
from
July 6, 2026 16:54
cb279ae to
eb6de6d
Compare
Three assertions, each under both project and projectService (feature-detected; v7's experimental service can't hold .gts files in-project, so the mode is registered only when a probe parse yields a program that contains virtual twins — 3 tests on tsee 7, 6 on tsee 8): - every virtual twin present shares statements with the .gts root, - a private-member class has one type identity across import paths, - an already-synced program is not re-grafted on later parses. Against the unfixed parser these fail — demonstrated on CI in ember-tooling#240: the re-graft canaries fail on TS 5.7, and the twin-graft/type-identity tests additionally fail on TS 6 cells where the .mts twin shadows the old single-candidate fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
I think something is wrong with the benchmark, or the benchmark isn't testing the right things. on the 4 million line project at work, this PR is actually a pretty serious reduction in speed. 
|
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
3 participants
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.
Follow-up to #233 for the remaining problems in #229 (non-deterministic typed-rule false positives, memory).
The bug
A project-mode program can hold one templated file under up to three paths:
.gtsroot itself (the linted file),.tsvirtual, created when another file resolves an extensionlessimport './x'through the patchedfileExists,.mtstwin injected as a root via the patchedreadDirectory(the targetreplaceExtensionsrewrites explicitimport './x.gts'specifiers to).syncMtsGtsSourceFilesgrafted the real SourceFile onto only the first twin it found, preferring.mts. On older TS/typescript-eslint versions the.mtstwin doesn't end up in the program, the fallback fires, and the.tsvirtual gets grafted — everything works. On current versions (verified with typescript-eslint 8.62.1 + TypeScript 6.0.3) the.mtsroot does exist, so the graft lands on a file nothing imports, while the.tsvirtual — the copy every extensionless import actually resolves to — keeps its own independent AST.Two ASTs for the same class means two class symbols. TypeScript compares private members nominally, so the class becomes not assignable to itself across import paths ("Type 'Foo' is not assignable to type 'Foo'"), unions of the "same" type don't dedupe, and typed rules like
no-unnecessary-type-assertion/no-redundant-type-constituentsproduce false positives that depend on which path a cached type was first reached through — i.e. on lint order, which varies run-to-run with concurrency. Verified in an isolated repro:checker.isTypeAssignableTo(gtsType, tsVirtualType)isfalseboth ways for a class with a private member on 0.14.3,true(identical type objects) with this patch.The second bug
The graft also ran after every parse, re-walking the entire program file list (O(program files) per linted file) and re-copying binder state (
symbol,locals) onto twins while a live type checker holds references to the previous symbols — stale symbols stay cached, fresh ones appear for new queries, and results become lint-order dependent. The graft is only needed once per program, so it now runs behind aWeakSetguard (a rebuilt program is a new object and gets re-grafted).Measurements (400 synthetic gts + 800 ts files, typed queries on every identifier)
Sharing statements across twins also drops the duplicate ASTs (~11% heap on tiny synthetic files; real components should save proportionally more).
Tests
tests/virtual-twin-type-identity.test.js, run under bothprojectandprojectService(feature-detected; 3 tests on tsee 7, 6 on tsee 8). #240 demonstrated them failing against the unfixed parser on CI:statementswith the.gtsroot,What this should fix for #229
The order-dependent false positives from typed rules, plus a slice of time/memory. The inherent cost — one full TS program per concurrent worker — remains; that part is expected for typed linting.
🤖 Generated with Claude Code