ES-conformant symbol typing - #64451
Michael FIG (michaelfig) wants to merge 6 commits into
Conversation
Move the new test cases and accepted baselines into tsc/testdata so the compiler test runner executes them. Regenerate baselines with the repository's CRLF convention. Co-authored-by: Codex <codex@openai.com>
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
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
What changed in this PR
Adds registered-symbol typing and preserves symbol identities through inference, property access, and declaration emit.
Changes:
- Introduces
RegisteredSymbol<Key>and genericSymbol.for. - Preserves unique-symbol types in const-like contexts.
- Adds compiler tests and baselines for symbol-keyed properties.
| 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.
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 <codex@openai.com>
|
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. |
| // 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.
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.
There was a problem hiding this comment.
I guess we just handle that in assignability separately and keeping the bitflags disjoint on this avoids other issues.
There was a problem hiding this comment.
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.
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 <codex@openai.com>
|
Michael FIG (@michaelfig) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
AI-generated from my test cases that showed discrepancies between TypeScript typing and ES standard behaviour.
Summary
RegisteredSymbol<Key>intrinsic and preserve literal registry keys in the return type ofSymbol.for.unique symboltypes in const-like declarations and allow const assertions on them.Tests
go -C ./tsc test -run='TestLocal/(registeredSymbolDeferredKey|registeredSymbolIntrinsic|registeredSymbolPropertyAccess|uniqueSymbolConstLikeWidening)\.ts$' ./internal/testrunner -count=1go -C ./tsc test ./internal/checker ./internal/apiRefs: #35562, #37469, #54778, #60052
Closes: #35909
Closes: #27524
Closes: #55901
Co-authored-by: Codex codex@openai.com