Eliminate well known symbols as a concept in the checker and rely on unique symbols #42543
Conversation
…ded with compat code in place, but this makes goto-def make more sense)
| return append(concatenate(concatenate([container], additionalContainers), reexportContainers), objectLiteralContainer); // This order expresses a preference for the real container if it is in scope | ||
| } | ||
| // we potentially a symbol which is a member of the instance side of something - look for a variable in scope with the container's type | ||
| // which may be acting like a namespace | ||
| const firstVariableMatch = !(container.flags & getQualifiedLeftMeaning(meaning)) |
There was a problem hiding this comment.
I'll call this out since it's a new bit compared to the older PR. This bit and the following modify our symbol container lookup to prefer, eg, Symbol.iterator to SymbolConstructor.iterator when doing printback for where the specified SymbolFlags say the first is allowable. Now, this isn't specific to symbols or Symbol in any way - other dynamic names from namespace-ish things should work similarly, too! So ArrayConstructor, Int8ArrayConsturctor.... anything with the split namespace pattern the DOM uses a lot.
src/compiler/checker.ts
Outdated
| @@ -5104,8 +5119,9 @@ namespace ts { | |||
| } | |||
| } | |||
| } | |||
| context.enclosingDeclaration = saveEnclosingDeclaration; | |||
| context.enclosingDeclaration = propertySymbol.valueDeclaration || propertySymbol.declarations?.[0] || saveEnclosingDeclaration; | |||
There was a problem hiding this comment.
If we don't set a specific enclosingDeclaration when serializing the name, in cases where we don't pass a contextual one (eg, error reporting), we'd print a name like [iterator] instead of [Symbol.iterator] (because symbol chain lookup bails entirely if there's no enclosing declaration provided). I think we much prefer the later, so now we actually specify an enclosing declaration for the name whenever we can.
There was a problem hiding this comment.
What's an example where propertySymbol.declarations?.[0] is useful?
There was a problem hiding this comment.
valueDeclaration is unset in two cases - one: when the symbol doesn't have a value meaning (as when it comes from an interface), two: when two value symbols merge and have differing value declarations. In both cases, we'd like a context node for the name here.
src/compiler/checker.ts
Outdated
| if (prop.escapedName === InternalSymbolName.Default) { | ||
| type = getLiteralType("default"); | ||
| } | ||
| else { | ||
| const name = prop.valueDeclaration && getNameOfDeclaration(prop.valueDeclaration) as PropertyName; | ||
| type = name && getLiteralTypeFromPropertyName(name) || getLiteralType(symbolName(prop)); | ||
| type = name && getLiteralTypeFromPropertyName(name) || (!isKnownSymbol(prop) ? getLiteralType(symbolName(prop)) : undefined); |
There was a problem hiding this comment.
Perhaps confusingly, isKnownSymbol has almost nothing to do with well-known symbols, and actually just checks if the checker Symbol has a symbol-y name (eg, it's escapedName starts with __@). It's mostly gone, since we were, intent-wise, actually using it to proxy for well-known-symboly-ness in places; however a handful of select locations remain where we actually want to check for normal symbol-ness.
There was a problem hiding this comment.
It won't be confusing once well-known symbols are gone as a concept, right? Seems fine to me.
src/compiler/checker.ts
Outdated
| @@ -25321,7 +25342,10 @@ namespace ts { | |||
| } | |||
|
|
|||
| if (computedNameType && !(computedNameType.flags & TypeFlags.StringOrNumberLiteralOrUnique)) { | |||
| if (isTypeAssignableTo(computedNameType, stringNumberSymbolType)) { | |||
| if (isTypeAny(computedNameType)) { | |||
There was a problem hiding this comment.
Technically, I can push this change through without this change, if we'd prefer. However, that means when you write, eg
const obj = {
[Symbol.whatever]: 0
};obj would get a number index signature (since Symbol.whatever is going to be any from the error of it not existing), whereas with this it gets a string index signature (which, IMO, makes much more sense for an any computed name). It mostly only matters, in the context of unique symbols, for follow-on errors to nonexistant symbols which were previously "well known" and thus entirely unchecked (which now get an error, become any and thus produce an index signature of some kind).
There was a problem hiding this comment.
From the comment here, it sounds like this mostly to cater to the "degenerate" case of Symbol.thingThatDoesNotExist which can happen in a loosely configured JavaScript project - is that the use-case you have in mind? And
That does loosen the behavior of this by quite a bit. I think I would be willing to go the opposite way and take a break on this behavior.
There was a problem hiding this comment.
You definitely don't wanna go the opposite way, because the numeric index signature we otherwise make makes very little sense, IMO. I actually think any producing a number index signature just feels a bit like a bug that we aught to fix.
There was a problem hiding this comment.
I get that, but I think the any behavior is already pretty undesirable, and is exacerbated if you're not using --noUncheckedIndexedAccess.
There was a problem hiding this comment.
???? That flag is about accessing keys and the types accessed keys are assigned, where here the issue is solely weather or not we make a key at all, and what slot that key should go in; that seems wholly unrelated to that flag.
There was a problem hiding this comment.
But
const s = {["str" as any]: "str"}can now be indexed by a wider variety of nonsense that would never work either, like s["uh oh"]. You're allowing a much more optimistic type to be formed which you can't take back after the fact unless you add a new flag.
There was a problem hiding this comment.
And in fact, I think that's why the numeric index signature was chosen - because at the time we also felt the behavior it supported was more desirable even if it wasn't good.
The point of the numeric index signature isn't to support indexing with numbers - it's to support indexing with any and still getting the appropriate type out. It's a hack, but it doesn't widen the net to every possible non-numeric property.
There was a problem hiding this comment.
But the point of any is to "widen the net".
There was a problem hiding this comment.
At best this isn't something we need to change with this feature. I don't find the "well known symbol that doesn't exist" thing to be a use-case that should fundamentally change what happens when you use any as a computed property key.
src/compiler/checker.ts
Outdated
| function getPropertyNameForKnownSymbolName(symbolName: string): __String { | ||
| const ctorType = getGlobalESSymbolConstructorSymbol(/*reportErrors*/ false); | ||
| const uniqueType = ctorType && getTypeOfPropertyOfType(getTypeOfSymbol(ctorType), escapeLeadingUnderscores(symbolName)); | ||
| return uniqueType && isTypeUsableAsPropertyName(uniqueType) ? getPropertyNameFromType(uniqueType) : `__@${symbolName}` as __String; |
There was a problem hiding this comment.
The __@${symbolName} as __String; case is reachable in scenarios where our internals request a name for a symbol which doesn't exist in the lib. Eg, requesting Symbol.asyncIterator in a lib target where it doesn't exist. No construct can make a property symbol name in that format anymore (unique symbol names have the form __@desc@###), so it's guaranteed to result in an undefined result from getPropertyOfType, resulting in a noIterationTypes result (as you may expect when the core iteration protocol types are missing!).
There was a problem hiding this comment.
Can you give a little more context on the significance? Are you saying this is handled well in the existing code, that other code needs to be careful, that there's a fundamental architectural issue, or that this now has a user-facing impact? (or all?)
There was a problem hiding this comment.
I'm saying that properties with a name like that can no longer be constructed, so when the proper name cannot be found, returning a name like that will always make a follow-up property lookup fail, which has the desired outcome.
| @@ -269,8 +269,7 @@ namespace ts { | |||
| * any such locations | |||
| */ | |||
| export function isSimpleInlineableExpression(expression: Expression) { | |||
| return !isIdentifier(expression) && isSimpleCopiableExpression(expression) || | |||
| isWellKnownSymbolSyntactically(expression); | |||
| return !isIdentifier(expression) && isSimpleCopiableExpression(expression); | |||
There was a problem hiding this comment.
This change is pretty much single-handedly responsible for the .js emit changes in this PR. Symbol.iterator and it's ilk aren't "simple copyable expressions" since, technically, accessing it could produce side effects (like any other property access). Now... at some point, we added Identifiers to isSimpleCopiableExpression's domain (which isn't technically safe, since the identifier may get reassigned between the copied references), so maybe property accesses which only consist of Identifiers wouldn't be terribly unreasonable, either. 🤷
There was a problem hiding this comment.
@rbuckton is probably the best judge of that.
| @@ -5,7 +5,7 @@ interface SymbolConstructor { | |||
| * A method that returns the default iterator for an object. Called by the semantics of the | |||
| * for-of statement. | |||
| */ | |||
| readonly iterator: symbol; | |||
| readonly iterator: unique symbol; | |||
There was a problem hiding this comment.
Fun fact! I didn't actually need to make any of these lib changes to make the PR work, what with the compat code in place! But I've updated them anyway, so they serve as examples of what unique symbols really are to anyone reading the definitions.
There was a problem hiding this comment.
Hm - and that won't conflict with a separate "polyfill" definition right? In other words, if someone separately declares it as
interface SymbolConstructor {
readonly iterator: symbol;
}and these merge, they won't end up conflicting, right?
There was a problem hiding this comment.
Nope - and there's already a test to that effect on the diff.
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 4c808c4. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 4c808c4. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 4c808c4. You can monitor the build here. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 4c808c4. You can monitor the build here. |
|
@weswigham Here they are:Comparison Report - master..42543
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I'm not sure if it will affect this PR or not, but I'm investigating changes to |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
Nah, I don't think it'd affect this. Freshness is for types from "literals" (so, |
|
Perf and user runs look fine. DT has two packages that look like some improvements, but I'll take a deeper look later. RWC has one project (a sample) with new errors because we reference es6+ symbols in the sample, but don't include the es6 lib (and prior to this we never checked that). So all in all tests look good. |
src/compiler/checker.ts
Outdated
| @@ -5104,8 +5119,9 @@ namespace ts { | |||
| } | |||
| } | |||
| } | |||
| context.enclosingDeclaration = saveEnclosingDeclaration; | |||
| context.enclosingDeclaration = propertySymbol.valueDeclaration || propertySymbol.declarations?.[0] || saveEnclosingDeclaration; | |||
There was a problem hiding this comment.
What's an example where propertySymbol.declarations?.[0] is useful?
src/compiler/checker.ts
Outdated
| @@ -25321,7 +25342,10 @@ namespace ts { | |||
| } | |||
|
|
|||
| if (computedNameType && !(computedNameType.flags & TypeFlags.StringOrNumberLiteralOrUnique)) { | |||
| if (isTypeAssignableTo(computedNameType, stringNumberSymbolType)) { | |||
| if (isTypeAny(computedNameType)) { | |||
There was a problem hiding this comment.
From the comment here, it sounds like this mostly to cater to the "degenerate" case of Symbol.thingThatDoesNotExist which can happen in a loosely configured JavaScript project - is that the use-case you have in mind? And
That does loosen the behavior of this by quite a bit. I think I would be willing to go the opposite way and take a break on this behavior.
src/compiler/checker.ts
Outdated
| function getPropertyNameForKnownSymbolName(symbolName: string): __String { | ||
| const ctorType = getGlobalESSymbolConstructorSymbol(/*reportErrors*/ false); | ||
| const uniqueType = ctorType && getTypeOfPropertyOfType(getTypeOfSymbol(ctorType), escapeLeadingUnderscores(symbolName)); | ||
| return uniqueType && isTypeUsableAsPropertyName(uniqueType) ? getPropertyNameFromType(uniqueType) : `__@${symbolName}` as __String; |
There was a problem hiding this comment.
Can you give a little more context on the significance? Are you saying this is handled well in the existing code, that other code needs to be careful, that there's a fundamental architectural issue, or that this now has a user-facing impact? (or all?)
| @@ -269,8 +269,7 @@ namespace ts { | |||
| * any such locations | |||
| */ | |||
| export function isSimpleInlineableExpression(expression: Expression) { | |||
| return !isIdentifier(expression) && isSimpleCopiableExpression(expression) || | |||
| isWellKnownSymbolSyntactically(expression); | |||
| return !isIdentifier(expression) && isSimpleCopiableExpression(expression); | |||
There was a problem hiding this comment.
@rbuckton is probably the best judge of that.
| [Symbol.iterator]() { } | ||
| ~~~~~~~~~~~~~~~ | ||
| !!! error TS2471: A computed property name of the form 'Symbol.iterator' must be of type 'symbol'. |
There was a problem hiding this comment.
This used to be close to giving a good error. Do we have any strictness flag that disallows this?
There was a problem hiding this comment.
Nope? Pretty sure we allow computed named with completely arbitrary (key compatible) types and just infer an index signature.
There was a problem hiding this comment.
Yeah, but what do you want to do when you have a key of type any? I can't imagine why you'd want that behavior under --strict. But it sounds like a separate issue.
There was a problem hiding this comment.
I mean, when Symbol isn't typed any, as in this test, we issue a lookup error...
| !!! error TS2495: Type 'StringIterator' is not an array type or a string type. |
There was a problem hiding this comment.
why did this change? Does the special-case code that generates this error need to be updated?
There was a problem hiding this comment.
!!! error TS2585: 'Symbol' only refers to a type, but is being used as a value here. Do you need to change your target library? Try changing the
libcompiler option to es2015 or later.
The Symbol.iterator member no longer exists, because Symbol.iterator doesn't exist (this is an ES5 target project).
| @@ -14,8 +14,8 @@ class C { | |||
| } | |||
|
|
|||
| (new C)[Symbol.iterator](0) // Should error | |||
There was a problem hiding this comment.
This now gives an error type but doesn't show an error. That seems wrong to me, and seems similar to a complaint that @DanielRosenwasser had about an earlier test, but I can't tell if your explanation there applies to this test or not.
Here's what I see:
- Symbol.iterator exists, and has type symbol. Fine to use as a computed property name.
- That computed property name should, ideally, look up the property in the call expression and give the expected error.
There was a problem hiding this comment.
Symbol.iterator in this case comes from an anonymous type on a Symbol variable declaration, so the interop symbol -> unique symbol code path doesn't take effect. So the Symbol.iterator in the class body makes an index signature, and that index signature doesn't handle symbols because they never do, hence the failed lookup here. It's not a problem that arises in the real world, since in the real world, the lib defines the global Symbol type using the SymbolConstructor type which we apply interop logic to.
There was a problem hiding this comment.
So....what is this test testing? Anything useful? Maybe it could be removed?
There was a problem hiding this comment.
If/when we get proper symbol index signature support, its behavior should change, so I don't think it should be removed.
src/compiler/checker.ts
Outdated
| if (prop.escapedName === InternalSymbolName.Default) { | ||
| type = getLiteralType("default"); | ||
| } | ||
| else { | ||
| const name = prop.valueDeclaration && getNameOfDeclaration(prop.valueDeclaration) as PropertyName; | ||
| type = name && getLiteralTypeFromPropertyName(name) || getLiteralType(symbolName(prop)); | ||
| type = name && getLiteralTypeFromPropertyName(name) || (!isKnownSymbol(prop) ? getLiteralType(symbolName(prop)) : undefined); |
There was a problem hiding this comment.
It won't be confusing once well-known symbols are gone as a concept, right? Seems fine to me.
|
TypeScript/src/lib/es2015.symbol.d.ts Line 5 in 616e6e6
Edit: Sorry! I just realized I got |
|
Yeah... only after posting my comment I realized that I had gotten it mixed up with |
using `Object.freeze` on Quaternion makes the quaternion type imcompatible because of iterator It seems to be a bug of TypeScript which is already fixed in the latest version See: microsoft/TypeScript#42543
using `Object.freeze` on Quaternion makes the quaternion type imcompatible because of iterator It seems to be a bug of TypeScript which is already fixed in the latest version See: microsoft/TypeScript#42543
#24738, but for 2021 and not 2018.
The parts from the original PR description that still hold:
Fixes #24622
Fixes #27525
Fixes #31253
Fixes #21603
Fixes #37182
Fixes half of #36468 (indirect calls to
Symbol()still do not produce freshunique symbols like direct calls do - but that should be a separate change, IMO)