GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — ES-conformant symbol typing by michaelfig · Pull Request #64451 · microsoft/TypeScript Conversation
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Non-literal and union registry keys currently receive unsound single unique-symbol identities.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: None
Adds registered-symbol typing and preserves symbol identities through inference, property access, and declaration emit.
Changes:
RegisteredSymbol<Key> and generic Symbol.for.| File | Description |
|---|---|
tsc/internal/checker/checker.go |
Implements registered-symbol types and unique-symbol preservation. |
tsc/internal/checker/nodebuilderimpl.go |
Emits registered symbol types and computed property names. |
tsc/internal/bundled/libs/lib.es2015.symbol.d.ts |
Declares the new intrinsic and Symbol.for signature. |
tsc/testdata/tests/cases/compiler/registeredSymbolIntrinsic.ts |
Tests intrinsic inference. |
tsc/testdata/tests/cases/compiler/registeredSymbolPropertyAccess.ts |
Tests symbol-keyed property access and emit. |
tsc/testdata/tests/cases/compiler/uniqueSymbolConstLikeWidening.ts |
Tests const-like unique-symbol widening. |
tsc/testdata/baselines/reference/compiler/registeredSymbolIntrinsic.types |
Records inferred types. |
tsc/testdata/baselines/reference/compiler/registeredSymbolIntrinsic.symbols |
Records resolved symbols. |
tsc/testdata/baselines/reference/compiler/registeredSymbolPropertyAccess.types |
Records property-access types. |
tsc/testdata/baselines/reference/compiler/registeredSymbolPropertyAccess.symbols |
Records property symbols. |
tsc/testdata/baselines/reference/compiler/registeredSymbolPropertyAccess.js |
Records declaration emit. |
tsc/testdata/baselines/reference/compiler/uniqueSymbolConstLikeWidening.types |
Records widening types. |
tsc/testdata/baselines/reference/compiler/uniqueSymbolConstLikeWidening.symbols |
Records widening symbols. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Resolve broad registered-symbol keys to symbol and distribute literal union keys over their possible registered symbol identities. Add compiler coverage for both cases. Co-authored-by: Codex <[email protected]>
|
The TypeScript team hasn't accepted the linked issue #27524. If you can get it accepted, this PR will have a better chance of being reviewed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Keep a type parameter deferred so instantiating Symbol.for's generic | ||
| // return type can resolve the actual key supplied at the call site. | ||
| if keyType.flags&(TypeFlagsStringOrNumberLiteral|TypeFlagsTypeParameter) == 0 { | ||
| return c.esSymbolType | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
How does this keep the type parameter deferred? Doesn't this just return symbol?
I think you not only need a new intrinsic type, you need a new kind of type (e.g. see newStringMappingType). Similar to how a type with TypeFlagsStringMapping doesn't have TypeFlagsString, it wouldn't have TypeFlagsSymbol - maybe because it could be instantiated to never? I'd have to consult with others.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I guess we just handle that in assignability separately and keeping the bitflags disjoint on this avoids other issues.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I agree that a separate type flag is a better way to represent a registered symbol whose key is still a type parameter. I’ve updated the PR to use that representation and added a regression test where two Symbol.for calls with generic keys previously shared an unsound unique-symbol identity.
My main hesitation was the limited remaining space in the TypeFlags uint32. It seems that all the bits up to bit 31 are already in use, so this uses the former Reserved3 bit at 31 and keeps TypeFlags as uint32. That consumes the remaining reserved bit, but seems justified for a distinct type kind.
I’d welcome your assessment of that tradeoff.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Avoid assigning a unique symbol identity to Symbol.for calls with unresolved keys. Add a regression case for indexed access through two generic registry keys. Co-authored-by: Codex <[email protected]>
|
@microsoft-github-policy-service agree |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Review effort: Lite
Findings: 1
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return r.isRelatedTo(source.AsStringMappingType().target, target.AsStringMappingType().target, RecursionFlagsBoth, false /*reportErrors*/) | ||
| } | ||
| case source.flags&TypeFlagsRegisteredESSymbol != 0: | ||
| return r.isRelatedTo(source.AsRegisteredESSymbolType().target, target.AsRegisteredESSymbolType().target, RecursionFlagsBoth, false /*reportErrors*/) |
| TypeFlagsBooleanLike = TypeFlagsBoolean | TypeFlagsBooleanLiteral | ||
| TypeFlagsEnumLike = TypeFlagsEnum | TypeFlagsEnumLiteral | ||
| TypeFlagsESSymbolLike = TypeFlagsESSymbol | TypeFlagsUniqueESSymbol | ||
| TypeFlagsESSymbolLike = TypeFlagsESSymbol | TypeFlagsUniqueESSymbol | TypeFlagsRegisteredESSymbol |
| @@ -505,7 +505,7 @@ const ( | |||
| // This *should* be every type other than null, undefined, void, and never | |||
| TypeFlagsNarrowable = TypeFlagsAny | TypeFlagsUnknown | TypeFlagsStructuredOrInstantiable | TypeFlagsStringLike | TypeFlagsNumberLike | TypeFlagsBigIntLike | TypeFlagsBooleanLike | TypeFlagsESSymbol | TypeFlagsUniqueESSymbol | TypeFlagsNonPrimitive | |||
Add regression coverage for generic key relations and symbol union and intersection reduction. Co-authored-by: Codex <[email protected]>
Preserve const alias identities in declaration output and refresh the unique-symbol declaration baselines. Co-authored-by: Codex <[email protected]>
Refresh 19 type, symbol, and diagnostic baselines while leaving the computed registered-symbol type baseline for further investigation. Co-authored-by: Codex <[email protected]>
Serialize computed expressions through their known registered-symbol types, preserving ordinary and readonly properties. Add declaration emit coverage and accept the corrected symbol-name baseline. Co-authored-by: Codex <[email protected]>
Use the existing Instantiable aggregate for base constraints and omit a redundant IncludesMask bit in both enum definitions. Co-authored-by: Codex <[email protected]>
|
|
||
| declare var Symbol: SymbolConstructor; | ||
|
|
||
| type RegisteredSymbol<Key extends string | number> = intrinsic; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Numeric keys appear to get separate identities from their string equivalents, but the registry stringifies keys. This should accept only strings or canonicalize numeric keys.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 44adab7 and baa2e42. The ECMAScript Symbol.for algorithm applies ToString(key) before looking in the registry; ToString throws a TypeError for a primitive Symbol (Symbol.for, ToString). The declaration now constrains Key to NonSymbol, rejecting symbol-typed arguments while accepting non-symbol primitives and objects. Runtime calls that bypass the type check still retain the specified throw behavior.
For statically known primitive keys, the RegisteredSymbol intrinsic emulates that string conversion before caching the symbol identity. Thus 1, 1n, and "1" share an identity, as do -0 and 0; distinct resulting strings remain distinct. Finite unions are handled member by member. Broad string, number, and bigint keys, and objects whose conversion result cannot be known statically, widen to symbol rather than claiming a particular registry identity. The new registeredSymbolPrimitiveKeys.ts regression test covers these cases, and the full compiler baseline suite passes.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Accept primitive Symbol.for keys and share registered symbol identities by their stringified registry key. Cover collisions, unions, broad keys, and invalid inputs. Co-authored-by: Codex <[email protected]>
Use a NonSymbol constraint so object and function values are accepted while unresolved keys remain plain symbol types. Update compiler coverage and baselines. Co-authored-by: Codex <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I’d prefer if RegisteredSymbol and Symbol.for retained the string type restriction:
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| * @param key key to search for. | ||
| */ | ||
| for(key: string): symbol; | ||
| for<Key extends NonSymbol>(key: Key): RegisteredSymbol<Key>; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| for<Key extends NonSymbol>(key: Key): RegisteredSymbol<Key>; | |
| for<Key extends string>(key: Key): RegisteredSymbol<Key>; |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| type NonSymbol = string | number | bigint | boolean | null | undefined | object; | ||
|
|
||
| type RegisteredSymbol<Key extends NonSymbol> = intrinsic; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| type NonSymbol = string | number | bigint | boolean | null | undefined | object; | |
| type RegisteredSymbol<Key extends NonSymbol> = intrinsic; | |
| type RegisteredSymbol<Key extends string> = intrinsic; |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I was trying to be as lax with the types as the ES standard allows, but if TS has a history of providing stricter types than ES APIs handle, I can certainly restore that behaviour. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
unique symbol lost on assignment to const despite type assertion Can't evaluate equality of symbols made by Symbol.for Assign a known symbol to a constant should retain type information
There was an error while loading. Please reload this page.