String literal types #5185
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,7 +81,7 @@ namespace ts { | |
| symbolToString, | ||
| getAugmentedPropertiesOfType, | ||
| getRootSymbols, | ||
| getContextualType: getApparentTypeOfContextualType, | ||
| getFullyQualifiedName, | ||
| getResolvedSignature, | ||
| getConstantValue, | ||
|
|
@@ -1623,7 +1623,7 @@ namespace ts { | |
| writeAnonymousType(<ObjectType>type, flags); | ||
| } | ||
| else if (type.flags & TypeFlags.StringLiteral) { | ||
| writer.writeStringLiteral(`"${escapeString((<StringLiteralType>type).text)}"`); | ||
| } | ||
| else { | ||
| // Should never get here | ||
|
|
@@ -4353,12 +4353,13 @@ namespace ts { | |
| } | ||
|
|
||
| function getStringLiteralType(node: StringLiteral): StringLiteralType { | ||
| const text = node.text; | ||
| if (hasProperty(stringLiteralTypes, text)) { | ||
| return stringLiteralTypes[text]; | ||
| } | ||
|
|
||
| let type = stringLiteralTypes[text] = <StringLiteralType>createType(TypeFlags.StringLiteral); | ||
| type.text = text; | ||
| return type; | ||
| } | ||
|
|
||
|
|
@@ -6877,7 +6878,7 @@ namespace ts { | |
| else if (operator === SyntaxKind.BarBarToken) { | ||
| // When an || expression has a contextual type, the operands are contextually typed by that type. When an || | ||
| // expression has no contextual type, the right operand is contextually typed by the type of the left operand. | ||
| let type = getApparentTypeOfContextualType(binaryExpression); | ||
| if (!type && node === binaryExpression.right) { | ||
| type = checkExpression(binaryExpression.left); | ||
| } | ||
|
|
@@ -6949,7 +6950,7 @@ namespace ts { | |
|
|
||
| function getContextualTypeForObjectLiteralElement(element: ObjectLiteralElement) { | ||
| let objectLiteral = <ObjectLiteralExpression>element.parent; | ||
| let type = getApparentTypeOfContextualType(objectLiteral); | ||
| if (type) { | ||
| if (!hasDynamicName(element)) { | ||
| // For a (non-symbol) computed property, there is no reason to look up the name | ||
|
|
@@ -6975,7 +6976,7 @@ namespace ts { | |
| // type of T. | ||
| function getContextualTypeForElementExpression(node: Expression): Type { | ||
| let arrayLiteral = <ArrayLiteralExpression>node.parent; | ||
| let type = getApparentTypeOfContextualType(arrayLiteral); | ||
| if (type) { | ||
| let index = indexOf(arrayLiteral.elements, node); | ||
| return getTypeOfPropertyOfContextualType(type, "" + index) | ||
|
|
@@ -6988,7 +6989,7 @@ namespace ts { | |
| // In a contextually typed conditional expression, the true/false expressions are contextually typed by the same type. | ||
| function getContextualTypeForConditionalOperand(node: Expression): Type { | ||
| let conditional = <ConditionalExpression>node.parent; | ||
| return node === conditional.whenTrue || node === conditional.whenFalse ? getApparentTypeOfContextualType(conditional) : undefined; | ||
| } | ||
|
|
||
| function getContextualTypeForJsxExpression(expr: JsxExpression|JsxSpreadAttribute): Type { | ||
|
|
@@ -7013,12 +7014,22 @@ namespace ts { | |
|
|
||
| // Return the contextual type for a given expression node. During overload resolution, a contextual type may temporarily | ||
| // be "pushed" onto a node using the contextualType property. | ||
| function getApparentTypeOfContextualType(node: Expression): Type { | ||
| let type = getContextualType(node); | ||
| return type && getApparentType(type); | ||
| } | ||
|
|
||
| /** | ||
| * Woah! Do you really want to use this function? | ||
| * | ||
| * Unless you're trying to get the *non-apparent* type for a value-literal type, | ||
| * you probably meant to use 'getApparentTypeOfContextualType'. | ||
| * Otherwise this is slightly less useful. | ||
| * | ||
| * @param node the expression whose contextual type will be returned. | ||
| * @returns the contextual type of an expression. | ||
| */ | ||
| function getContextualType(node: Expression): Type { | ||
| if (isInsideWithStatementBody(node)) { | ||
| // We cannot answer semantic questions within a with block, do not proceed any further | ||
| return undefined; | ||
|
|
@@ -7057,7 +7068,7 @@ namespace ts { | |
| Debug.assert(parent.parent.kind === SyntaxKind.TemplateExpression); | ||
| return getContextualTypeForSubstitutionExpression(<TemplateExpression>parent.parent, node); | ||
| case SyntaxKind.ParenthesizedExpression: | ||
| return getApparentTypeOfContextualType(<ParenthesizedExpression>parent); | ||
| case SyntaxKind.JsxExpression: | ||
| case SyntaxKind.JsxSpreadAttribute: | ||
| return getContextualTypeForJsxExpression(<JsxExpression>parent); | ||
|
|
@@ -7097,7 +7108,7 @@ namespace ts { | |
| Debug.assert(node.kind !== SyntaxKind.MethodDeclaration || isObjectLiteralMethod(node)); | ||
| let type = isObjectLiteralMethod(node) | ||
| ? getContextualTypeForObjectLiteralMethod(node) | ||
| : getApparentTypeOfContextualType(node); | ||
| if (!type) { | ||
| return undefined; | ||
| } | ||
|
|
@@ -7227,7 +7238,7 @@ namespace ts { | |
| type.pattern = node; | ||
| return type; | ||
| } | ||
| let contextualType = getApparentTypeOfContextualType(node); | ||
| if (contextualType && contextualTypeIsTupleLikeType(contextualType)) { | ||
| let pattern = contextualType.pattern; | ||
| // If array literal is contextually typed by a binding pattern or an assignment pattern, pad the resulting | ||
|
|
@@ -7318,7 +7329,7 @@ namespace ts { | |
|
|
||
| let propertiesTable: SymbolTable = {}; | ||
| let propertiesArray: Symbol[] = []; | ||
| let contextualType = getApparentTypeOfContextualType(node); | ||
| let contextualTypeHasPattern = contextualType && contextualType.pattern && | ||
| (contextualType.pattern.kind === SyntaxKind.ObjectBindingPattern || contextualType.pattern.kind === SyntaxKind.ObjectLiteralExpression); | ||
| let inDestructuringPattern = isAssignmentTarget(node); | ||
|
|
@@ -10273,6 +10284,25 @@ namespace ts { | |
| return getUnionType([type1, type2]); | ||
| } | ||
|
|
||
| function checkStringLiteralExpression(node: LiteralExpression) { | ||
| // TODO (drosen): Do we want to apply the same approach to no-sub template literals? | ||
|
There was a problem hiding this comment. I'm choosing not to apply the same approach because we currently don't treat NoSubTemplates the same as strings in places like string indexing and overload resolution. I think it would be weird to treat them the same way here but not elsewhere. For the record, I would prefer if we treated them the same elsewhere, but I don't want to conflate that with this PR. |
||
|
|
||
| let contextualType = getContextualType(node); | ||
| if (contextualType) { | ||
| if (contextualType.flags & TypeFlags.Union) { | ||
| for (const type of (<UnionType>contextualType).types) { | ||
| if (type.flags & TypeFlags.StringLiteral && (<StringLiteralType>type).text === node.text) { | ||
| return type; | ||
| } | ||
| } | ||
| } | ||
| else if (contextualType.flags & TypeFlags.StringLiteral && (<StringLiteralType>contextualType).text === node.text) { | ||
| return contextualType; | ||
| } | ||
| } | ||
| return stringType; | ||
| } | ||
|
|
||
| function checkTemplateExpression(node: TemplateExpression): Type { | ||
| // We just want to check each expressions, but we are unconcerned with | ||
| // the type of each expression, as any value may be coerced into a string. | ||
|
|
@@ -10332,7 +10362,7 @@ namespace ts { | |
| if (isInferentialContext(contextualMapper)) { | ||
| let signature = getSingleCallSignature(type); | ||
| if (signature && signature.typeParameters) { | ||
| let contextualType = getApparentTypeOfContextualType(<Expression>node); | ||
| if (contextualType) { | ||
| let contextualSignature = getSingleCallSignature(contextualType); | ||
| if (contextualSignature && !contextualSignature.typeParameters) { | ||
|
|
@@ -10403,6 +10433,7 @@ namespace ts { | |
| case SyntaxKind.TemplateExpression: | ||
| return checkTemplateExpression(<TemplateExpression>node); | ||
| case SyntaxKind.StringLiteral: | ||
| return checkStringLiteralExpression(<LiteralExpression>node); | ||
| case SyntaxKind.NoSubstitutionTemplateLiteral: | ||
|
There was a problem hiding this comment. Not that I want to sound negative but for every string literal, this gets called. It's a huge slowdown. There was a problem hiding this comment. What kind of numbers are you seeing (and on what codebase)? There was a problem hiding this comment.
I think the VM already optimize for this? There was a problem hiding this comment. It's useless to speculate about what's fast and what's not. Measure it. There was a problem hiding this comment. Meh. Measure it in 1 version of Chrome, it changes in the next. You measure it, I've said what I had to say. There was a problem hiding this comment. @jbondc 👍 for the benchmark test. There was a problem hiding this comment. I don't think the original issue was so terrible. You only ran into this case when you were contextually typed by a type with a string literal constituent to begin with. In practice, this is not frequently encountered. However, @jbondc, I think you'll like the the current implementation a lot better. We now only create a string literal type if the constituent types have a string literal type, not just if their content is equal. Furthermore, the types are cached in a map of strings to string literal types. Whenever testing the assignability of two string literal types, reference equality kicks in (which is fast). Additionally, error messages are slightly better because you have a specific literal type to report (i.e. There was a problem hiding this comment. @jbondc at worst that's probably much faster than trying to resolve the 100 different overloads. Though, I'm going to try to avoid speculating. I wasn't seeing a real perf hit in our benchmarks even with the initial change. |
||
| return stringType; | ||
| case SyntaxKind.RegularExpressionLiteral: | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1977,9 +1977,7 @@ namespace ts { | |
|
|
||
| function parseParameterType(): TypeNode { | ||
| if (parseOptional(SyntaxKind.ColonToken)) { | ||
| return parseType(); | ||
| } | ||
|
|
||
| return undefined; | ||
|
|
@@ -2367,6 +2365,8 @@ namespace ts { | |
| // If these are followed by a dot, then parse these out as a dotted type reference instead. | ||
| let node = tryParse(parseKeywordAndNoDot); | ||
| return node || parseTypeReferenceOrTypePredicate(); | ||
| case SyntaxKind.StringLiteral: | ||
| return <StringLiteral>parseLiteralNode(/*internName*/ true); | ||
| case SyntaxKind.VoidKeyword: | ||
| case SyntaxKind.ThisKeyword: | ||
| return parseTokenNode<TypeNode>(); | ||
|
|
@@ -2397,6 +2397,7 @@ namespace ts { | |
| case SyntaxKind.OpenBracketToken: | ||
| case SyntaxKind.LessThanToken: | ||
| case SyntaxKind.NewKeyword: | ||
| case SyntaxKind.StringLiteral: | ||
| return true; | ||
| case SyntaxKind.OpenParenToken: | ||
| // Only consider '(' the start of a type if followed by ')', '...', an identifier, a modifier, | ||
|
|
@@ -5457,6 +5458,7 @@ namespace ts { | |
| return parseTokenNode<JSDocType>(); | ||
| } | ||
|
|
||
| // TODO (drosen): Parse string literal types in JSDoc as well. | ||
|
There was a problem hiding this comment. Not sure we'd need to do this - strings types aren't directly valid jsdoc types. They'd have to be wrapped up inside an enum, 'a la: /**
* @enum
*/
var MyEnum = {
Member: "Member"
}
/**
* @param {MyEnum} param
*/
function foo(param) {
}So literal string types should never appear in JSDoc comments, since nothing really supports them. |
||
| return parseJSDocTypeReference(); | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,33 +1,33 @@ | ||
| === tests/cases/compiler/callSignatureFunctionOverload.ts === | ||
| var foo: { | ||
| >foo : { (name: string): string; (name: "order"): string; (name: "content"): string; (name: "done"): string; } | ||
|
|
||
| (name: string): string; | ||
| >name : string | ||
|
|
||
| (name: 'order'): string; | ||
| >name : "order" | ||
|
|
||
| (name: 'content'): string; | ||
| >name : "content" | ||
|
|
||
| (name: 'done'): string; | ||
| >name : "done" | ||
| } | ||
|
|
||
| var foo2: { | ||
| >foo2 : { (name: string): string; (name: "order"): string; (name: "order"): string; (name: "done"): string; } | ||
|
|
||
| (name: string): string; | ||
| >name : string | ||
|
|
||
| (name: 'order'): string; | ||
| >name : "order" | ||
|
|
||
| (name: 'order'): string; | ||
| >name : "order" | ||
|
|
||
| (name: 'done'): string; | ||
| >name : "done" | ||
| } | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Yeah... that was why I named it Worker...
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.
Ah, that makes sense, though it feels strange to get the actual contextual type when its name is worker. If you think it'd be better to rename it back to worker, I can do that.
I also suspect that we could shave off some time by using this function (instead of
getApparentTypeOfContextualType) for array literals as well. I'll probably do it as part of another change.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 idea (however it is achieved) is to make the apparent type the default behavior when you don't specify anything. Getting the contextual type without its apparent type is quite an odd thing to do, so I prefer that to be the marked form. You can also call them getContextualType and getContextualTypeWithoutApparentType.