-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow dynamic names in types #15473
Allow dynamic names in types #15473
Conversation
tests/cases/compiler/dynamicNames.ts
Outdated
|
||
interface T16 { | ||
[c5]: number; | ||
[c6]: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be an error? I also tested with duplicate string literals with incompatible types. I expected a compile time error, however a union may also be possible.
const k1 = "literal name";
const k2 = "literal name";
interface T17 {
[k1]: string;
[k2]: 123; // error expected
}
type T18 = {
[k1]: string;
[k2]: 123; // error expected
}
// strange IntelliSense:
// type T18 = { literal name: string }
interface T19 {
['literal name']: string;
['literal name']: 123; // error as expected
}
interface T20 {
['literal name']: string; // error here
[k1]: 123; // and also here
}
let t17: T17;
t17['literal name'] // strange IntelliSense: `T17.literal name: string`
let t19: T19;
t19['literal name'] // strange IntelliSense: `T19[['literal name']]: string`
Can you give an example of the symbol literal type scenario? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks solid enough, but I don't really get why this change is needed right now. It seems like a lot of change without much payoff. Maybe this should wait until we have symbol literal types?
Also a few nitpicks in the comments.
tests/cases/compiler/dynamicNames.ts
Outdated
@@ -0,0 +1,102 @@ | |||
// @target: esnext | |||
// @module: commonjs | |||
// @filename: module.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add // @declaration: true
here to test the declarationEmitter code?
src/compiler/checker.ts
Outdated
if (name) { | ||
// TODO(rbuckton): ESSymbolLiteral | ||
const nameType = checkComputedPropertyName(name); | ||
return (nameType.flags & TypeFlags.StringOrNumberLiteral) !== 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find !!
easier to read than !== 0
, but I'm not sure how common that is..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it has a major impact on performance, but !!
requires coercion from SMI to boolean, while !== 0
does not.
src/compiler/checker.ts
Outdated
} | ||
|
||
function resolveDynamicMembersOfClassOrInterfaceOrTypeLiteralNode(node: ClassLikeDeclaration | InterfaceDeclaration | TypeLiteralNode, symbolTable: SymbolTable) { | ||
for (const member of node.members) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be simpler to have a third parameter symbol
and only one resolveDynamicMembersOfNode(members: NodeArray<ClassElement | TypeElement | ObjectLiteralElementLike>, symbols: Symbol[], symbolTable: SymbolTable)
. Then resolveDynamicMembersOfSymbol
could pull off the properties of node
in each case
.
The PR has been updated to include unique symbol types. |
Updated with support for unique symbol types. Also updated the description, above. |
At some point we may choose to simplify the binder with respect to well-known symbols and let late-binding take care of it. |
@sandersn any other comments? |
@ahejlsberg, @mhegazy: In ae11ae5 I've made some changes to how we handle widening in |
can I use without use "// @ts-ignore" instruction to make the compiler ignore that "mistake"? for example:
|
@DanielRosenwasser Can we add it to |
@HerringtonDarkholme we are updating the docs currently. we should have them all up-to-date before the final 2.7 goes out next week. |
This changes allows the use of an Identifier or PropertyAccessExpression as part of a computed property name in an interface, class, or type literal as long as the type of the expression is a string or numeric literal type, or is a unique symbol type.
Unique Symbol Types
A unique symbol type is created in specific cases when calling the global
Symbol
function or the globalSymbol.for
function, or when you use theunique symbol
type.Unique symbol types have several rules and restrictions:
unique symbol
type may only be used on aconst
variable orreadonly
property declaration.unique symbol
type widens tosymbol
.unique symbol
types are not assignable to each other.unique symbol
type is bound to the symbol of the declaration where it was defined.unique symbol
, as the names for these declarations merge into the same symbol.Symbol.for(x)
would return the same ES Symbol at runtime if called multiple times for the samex
, we currently treat separate calls toSymbol.for(x)
as unique symbols. We may choose to revisit this in the future.Examples
Late Binding
Dynamic member names are resolved and "bound" in the checker on-demand whenever the members of a symbol are requested, allowing members with dynamic names to participate in type relationships.
Since dynamic members are bound later than syntactically recognizable member names, we disallow defining a member both syntactically and via a dynamic name so as not to introduce inconsistencies with overload resolution as the declarations might end up in the wrong order.
Examples
Fixes #2012
Fixes #5579
Fixes #7436 (via
typeof SAYHELLO
)Fixes #11736 (via
typeof opAdd
)Partially Fixes #13031 (via
unique symbol
type, though there are other issues that still block this)