Add --stableTypeOrdering for TS7 ordering compat#63084
Add --stableTypeOrdering for TS7 ordering compat#63084jakebailey wants to merge 6 commits intomicrosoft:mainfrom
Conversation
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
The flag is disabled; checking perf to verify that it isn't harmful. @typescript-bot perf test this faster |
There was a problem hiding this comment.
Pull request overview
Adds a new --stableTypeOrdering compiler flag intended to make type/symbol/node ordering deterministic (TS7 ordering compatibility) to support concurrent checking in the tsgo port, at a performance cost.
Changes:
- Introduces the
stableTypeOrderingcompiler option (CLI +CompilerOptions) and its help/diagnostic text. - Reorders
TypeFlags(and adjusts related flag groupings) to support a deterministic sort strategy. - Implements stable ordering logic in the checker (type/symbol/node comparators and stable sorting in several flows).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/baselines/reference/tsc/commandLine/help-all.js | Updates --help --all baseline to include --stableTypeOrdering. |
| tests/baselines/reference/api/typescript.d.ts | Updates public API baselines for reordered TypeFlags and new CompilerOptions.stableTypeOrdering. |
| src/compiler/types.ts | Reorders TypeFlags, tweaks ObjectFlags.ObjectTypeKindMask, adds stableTypeOrdering to CompilerOptions. |
| src/compiler/diagnosticMessages.json | Adds the option description message used by the CLI help output. |
| src/compiler/core.ts | Makes binarySearchKey tolerant of non-Comparison comparer returns; exports compareComparableValues. |
| src/compiler/commandLineParser.ts | Registers the new stableTypeOrdering option for CLI/config parsing. |
| src/compiler/checker.ts | Adds stable ordering machinery and wires it behind compilerOptions.stableTypeOrdering. |
| switch (kind1) { | ||
| case TypeMapKind.Simple: { | ||
| const c = compareTypes(m1.source, (m2 as typeof m1).source); | ||
| if (c !== 0) { | ||
| return c; |
There was a problem hiding this comment.
TypeMapKind includes Deferred, Function, and Composite (see src/compiler/types.ts:7076+), but compareTypeMappers doesn’t handle them. For those kinds, the switch falls through and returns 0, causing many distinct mappers to compare equal and forcing later fallback to unstable IDs. Add deterministic comparisons for the missing kinds (and consider handling Composite similarly to Merged).
| return stableTypeOrdering ? binarySearch(types, type, identity, compareTypes) >= 0 : binarySearch(types, type, getTypeId, compareValues) >= 0; | ||
| } | ||
|
|
||
| function insertType(types: Type[], type: Type): boolean { | ||
| const index = binarySearch(types, type, getTypeId, compareValues); | ||
| const index = stableTypeOrdering ? binarySearch(types, type, identity, compareTypes) : binarySearch(types, type, getTypeId, compareValues); |
There was a problem hiding this comment.
binarySearch expects a Comparer returning Comparison (-1/0/1), but compareTypes returns arbitrary numbers (e.g. ID differences). This is a type error and also violates the Comparer contract. Wrap with Math.sign/toComparison at the call site (or change compareTypes to return Comparison by using compareValues/compareComparableValues and converting numeric deltas).
There was a problem hiding this comment.
This is true, but I updated binarySearch to math.Sign, which is theoretically sufficient, although I could define a compareTypes helper that does that
| for (let i = 0; i < (t1.labeledElementDeclarations?.length ?? 0); i++) { | ||
| const c = compareElementLabels(t1.labeledElementDeclarations![i], t2.labeledElementDeclarations![i]); | ||
| if (c !== 0) { | ||
| return c; | ||
| } |
There was a problem hiding this comment.
Tuple label comparison only iterates up to t1.labeledElementDeclarations?.length and doesn’t account for t2 having more (or fewer) labels, so tuples can compare equal despite differing label metadata and then fall back to unstable IDs. Compare the label-declaration lengths first (or iterate up to max(len1, len2) and treat missing as undefined).
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| affectsBuildInfo: true, | ||
| category: Diagnostics.Type_Checking, | ||
| description: Diagnostics.Ensure_types_are_ordered_stably_and_deterministically_across_compilations, | ||
| defaultValueDescription: false, |
There was a problem hiding this comment.
I should be able to make this a hidden flag, too.
|
@typescript-bot pack this |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
Yoinked out of #62556 / tsgo-port.
The Go port of the compiler does concurrent checking with multiple checkers. But because they can visit nodes/types/symbols in a different order, their results are not stable when compared to each other due to lazy ID assignment.
The way the port addresses this is to enforce a strict ordering of types, symbols, and nodes, that does not rely on IDs. This causes changes in behavior, however, since the ordering is not a defined part of the language.
This PR adds a flag that enables the same algorithm that TS7 has, at the cost of perf.
On main, this flag is defaulted to false, of course, but it'd be flipped on in tsgo-port (which is more convenient than maintaining a patch).
This requires
TypeFlagsto change order, but this is not a part of the API per se; nobody should depend on the exact values of these as they are not declaredconstin the published.d.ts.