String literal types #5185
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10286,7 +10286,7 @@ namespace ts { | |
|
|
||
| function checkStringLiteralExpression(node: LiteralExpression) { | ||
| // TODO (drosen): Do we want to apply the same approach to no-sub template literals? | ||
|
|
||
| let contextualType = getContextualType(node); | ||
| if (contextualType) { | ||
| if (contextualType.flags & TypeFlags.Union) { | ||
|
|
@@ -10435,7 +10435,7 @@ namespace ts { | |
| 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: | ||
| return globalRegExpType; | ||
| case SyntaxKind.ArrayLiteralExpression: | ||
|
|
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.
Should we resolve this
TODO?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 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.
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.
Alright.