Skip to content
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

String literal types #5185

Merged
merged 44 commits into from
Nov 10, 2015
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
311a0cf
Added tests.
DanielRosenwasser Sep 11, 2015
911e907
Added test for string literal types in type arguments.
DanielRosenwasser Oct 1, 2015
f04cc39
Accepted baselines.
DanielRosenwasser Oct 1, 2015
191be4f
Make string literals valid constituent types nodes in the parser.
DanielRosenwasser Oct 1, 2015
84786d8
Accepted baselines.
DanielRosenwasser Oct 1, 2015
dc0e368
Make string literals valid types in type lists.
DanielRosenwasser Oct 1, 2015
8891fba
Accepted baselines.
DanielRosenwasser Oct 1, 2015
82545ce
Added test for string types in tuples.
DanielRosenwasser Oct 1, 2015
ed927d8
Accepted baselines.
DanielRosenwasser Oct 1, 2015
a3e7ccb
Use normalized text for text on string literal types.
DanielRosenwasser Oct 2, 2015
20c2c4e
Amended fourslash tests to expect double quotes.
DanielRosenwasser Oct 2, 2015
87f2957
Accepted baselines.
DanielRosenwasser Oct 2, 2015
f721971
Capture compatible contextual types for unions containing string lite…
DanielRosenwasser Oct 2, 2015
7b4e94d
Accepted baselines.
DanielRosenwasser Oct 2, 2015
d8d72aa
Separated the concept of apparent types from contextual types for str…
DanielRosenwasser Oct 2, 2015
315b06d
Accepted baselines.
DanielRosenwasser Oct 2, 2015
4b736da
Fixed issue in test.
DanielRosenwasser Oct 2, 2015
fd5dec4
Accepted baselines.
DanielRosenwasser Oct 2, 2015
f7a6ac7
Added more tests.
DanielRosenwasser Oct 7, 2015
a440f06
Accepted baselines.
DanielRosenwasser Oct 8, 2015
d2e2a55
Added fourslash test.
DanielRosenwasser Oct 8, 2015
6e3343c
Merge branch 'master' into stringLiteralTypes
DanielRosenwasser Oct 8, 2015
74ac57d
Accepted post-merge baselines.
DanielRosenwasser Oct 8, 2015
61ece76
Return the string literal type itself instead of the union type.
DanielRosenwasser Oct 8, 2015
84b64c4
Accepted baselines.
DanielRosenwasser Oct 8, 2015
3788254
Semicolon.
DanielRosenwasser Oct 8, 2015
ebc47d5
Linting.
DanielRosenwasser Oct 8, 2015
725bda8
Merge branch 'master' into stringLiteralTypes
DanielRosenwasser Oct 15, 2015
ec0d49a
Always use a string literal type if contextually typed by any string …
DanielRosenwasser Oct 15, 2015
1dbd8d1
Accepted baselines.
DanielRosenwasser Oct 15, 2015
307d73e
Merge branch 'master' into stringLiteralTypes
DanielRosenwasser Oct 22, 2015
049d02f
Merge branch 'master' into stringLiteralTypes
DanielRosenwasser Oct 23, 2015
6618fd2
Added tests for operations that use assignable to/from.
DanielRosenwasser Oct 26, 2015
bf4880a
Merge branch 'master' into stringLiteralTypes
DanielRosenwasser Oct 26, 2015
5e23143
Accepted baselines.
DanielRosenwasser Oct 27, 2015
8dbfe1c
Added specific checks for comparing stringlike types.
DanielRosenwasser Nov 6, 2015
a1fcfaf
Accepted baselines.
DanielRosenwasser Nov 6, 2015
bb232f7
Merge remote-tracking branch 'origin/master' into stringLiteralTypes
DanielRosenwasser Nov 6, 2015
f939ff2
Fixed unreachable code in tests.
DanielRosenwasser Nov 6, 2015
d234b8d
Accepted baselines.
DanielRosenwasser Nov 6, 2015
c011ed4
Const.
DanielRosenwasser Nov 6, 2015
38090c6
Added tests for template strings with string literal types.
DanielRosenwasser Nov 9, 2015
d294524
Accepted baselines.
DanielRosenwasser Nov 9, 2015
ea4e21d
Fixed comments.
DanielRosenwasser Nov 9, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 49 additions & 18 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ namespace ts {
symbolToString,
getAugmentedPropertiesOfType,
getRootSymbols,
getContextualType,
getContextualType: getApparentTypeOfContextualType,
getFullyQualifiedName,
getResolvedSignature,
getConstantValue,
Expand Down Expand Up @@ -1623,7 +1623,7 @@ namespace ts {
writeAnonymousType(<ObjectType>type, flags);
}
else if (type.flags & TypeFlags.StringLiteral) {
writer.writeStringLiteral((<StringLiteralType>type).text);
writer.writeStringLiteral(`"${escapeString((<StringLiteralType>type).text)}"`);
}
else {
// Should never get here
Expand Down Expand Up @@ -4353,12 +4353,13 @@ namespace ts {
}

function getStringLiteralType(node: StringLiteral): StringLiteralType {
if (hasProperty(stringLiteralTypes, node.text)) {
return stringLiteralTypes[node.text];
const text = node.text;
if (hasProperty(stringLiteralTypes, text)) {
return stringLiteralTypes[text];
}

let type = stringLiteralTypes[node.text] = <StringLiteralType>createType(TypeFlags.StringLiteral);
type.text = getTextOfNode(node);
let type = stringLiteralTypes[text] = <StringLiteralType>createType(TypeFlags.StringLiteral);
type.text = text;
return type;
}

Expand Down Expand Up @@ -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 = getContextualType(binaryExpression);
let type = getApparentTypeOfContextualType(binaryExpression);
if (!type && node === binaryExpression.right) {
type = checkExpression(binaryExpression.left);
}
Expand Down Expand Up @@ -6949,7 +6950,7 @@ namespace ts {

function getContextualTypeForObjectLiteralElement(element: ObjectLiteralElement) {
let objectLiteral = <ObjectLiteralExpression>element.parent;
let type = getContextualType(objectLiteral);
let type = getApparentTypeOfContextualType(objectLiteral);
if (type) {
if (!hasDynamicName(element)) {
// For a (non-symbol) computed property, there is no reason to look up the name
Expand All @@ -6975,7 +6976,7 @@ namespace ts {
// type of T.
function getContextualTypeForElementExpression(node: Expression): Type {
let arrayLiteral = <ArrayLiteralExpression>node.parent;
let type = getContextualType(arrayLiteral);
let type = getApparentTypeOfContextualType(arrayLiteral);
if (type) {
let index = indexOf(arrayLiteral.elements, node);
return getTypeOfPropertyOfContextualType(type, "" + index)
Expand All @@ -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 ? getContextualType(conditional) : undefined;
return node === conditional.whenTrue || node === conditional.whenFalse ? getApparentTypeOfContextualType(conditional) : undefined;
}

function getContextualTypeForJsxExpression(expr: JsxExpression|JsxSpreadAttribute): Type {
Expand All @@ -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 getContextualType(node: Expression): Type {
let type = getContextualTypeWorker(node);
function getApparentTypeOfContextualType(node: Expression): Type {
let type = getContextualType(node);
return type && getApparentType(type);
}

function getContextualTypeWorker(node: Expression): Type {
/**
* Woah! Do you really want to use this function?
Copy link
Contributor

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...

Copy link
Member Author

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.

Copy link
Contributor

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.

*
* 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;
Expand Down Expand Up @@ -7057,7 +7068,7 @@ namespace ts {
Debug.assert(parent.parent.kind === SyntaxKind.TemplateExpression);
return getContextualTypeForSubstitutionExpression(<TemplateExpression>parent.parent, node);
case SyntaxKind.ParenthesizedExpression:
return getContextualType(<ParenthesizedExpression>parent);
return getApparentTypeOfContextualType(<ParenthesizedExpression>parent);
case SyntaxKind.JsxExpression:
case SyntaxKind.JsxSpreadAttribute:
return getContextualTypeForJsxExpression(<JsxExpression>parent);
Expand Down Expand Up @@ -7097,7 +7108,7 @@ namespace ts {
Debug.assert(node.kind !== SyntaxKind.MethodDeclaration || isObjectLiteralMethod(node));
let type = isObjectLiteralMethod(node)
? getContextualTypeForObjectLiteralMethod(node)
: getContextualType(node);
: getApparentTypeOfContextualType(node);
if (!type) {
return undefined;
}
Expand Down Expand Up @@ -7227,7 +7238,7 @@ namespace ts {
type.pattern = node;
return type;
}
let contextualType = getContextualType(node);
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
Expand Down Expand Up @@ -7318,7 +7329,7 @@ namespace ts {

let propertiesTable: SymbolTable = {};
let propertiesArray: Symbol[] = [];
let contextualType = getContextualType(node);
let contextualType = getApparentTypeOfContextualType(node);
let contextualTypeHasPattern = contextualType && contextualType.pattern &&
(contextualType.pattern.kind === SyntaxKind.ObjectBindingPattern || contextualType.pattern.kind === SyntaxKind.ObjectLiteralExpression);
let inDestructuringPattern = isAssignmentTarget(node);
Expand Down Expand Up @@ -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?
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.


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.
Expand Down Expand Up @@ -10332,7 +10362,7 @@ namespace ts {
if (isInferentialContext(contextualMapper)) {
let signature = getSingleCallSignature(type);
if (signature && signature.typeParameters) {
let contextualType = getContextualType(<Expression>node);
let contextualType = getApparentTypeOfContextualType(<Expression>node);
if (contextualType) {
let contextualSignature = getSingleCallSignature(contextualType);
if (contextualSignature && !contextualSignature.typeParameters) {
Expand Down Expand Up @@ -10403,6 +10433,7 @@ namespace ts {
case SyntaxKind.TemplateExpression:
return checkTemplateExpression(<TemplateExpression>node);
case SyntaxKind.StringLiteral:
return checkStringLiteralExpression(<LiteralExpression>node);
case SyntaxKind.NoSubstitutionTemplateLiteral:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I want to sound negative but for every string literal, this gets called.

It's a huge slowdown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of numbers are you seeing (and on what codebase)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you consider that every string has on average 5 bytes, you're making 1-5 integer checks for every comparison:

I think the VM already optimize for this?
http://jsperf.com/string-vs-int-comparison-1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's useless to speculate about what's fast and what's not. Measure it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh. Measure it in 1 version of Chrome, it changes in the next. You measure it, I've said what I had to say.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jbondc 👍 for the benchmark test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Type '"Foo"' is not assignable to '"Bar"'. instead of Type 'string' is not assignable to '"Bar"'.).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:
Expand Down
8 changes: 5 additions & 3 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1977,9 +1977,7 @@ namespace ts {

function parseParameterType(): TypeNode {
if (parseOptional(SyntaxKind.ColonToken)) {
return token === SyntaxKind.StringLiteral
? <StringLiteral>parseLiteralNode(/*internName*/ true)
: parseType();
return parseType();
}

return undefined;
Expand Down Expand Up @@ -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>();
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -5457,6 +5458,7 @@ namespace ts {
return parseTokenNode<JSDocType>();
}

// TODO (drosen): Parse string literal types in JSDoc as well.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/callSignatureFunctionOverload.types
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; }
>foo : { (name: string): string; (name: "order"): string; (name: "content"): string; (name: "done"): string; }

(name: string): string;
>name : string

(name: 'order'): string;
>name : 'order'
>name : "order"

(name: 'content'): string;
>name : 'content'
>name : "content"

(name: 'done'): string;
>name : 'done'
>name : "done"
}

var foo2: {
>foo2 : { (name: string): string; (name: 'order'): string; (name: 'order'): string; (name: 'done'): string; }
>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"

(name: 'order'): string;
>name : 'order'
>name : "order"

(name: 'done'): string;
>name : 'done'
>name : "done"
}

16 changes: 8 additions & 8 deletions tests/baselines/reference/constantOverloadFunction.types
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,27 @@ class Derived3 extends Base { biz() { } }
>biz : () => void

function foo(tagName: 'canvas'): Derived1;
>foo : { (tagName: 'canvas'): Derived1; (tagName: 'div'): Derived2; (tagName: 'span'): Derived3; (tagName: string): Base; }
>tagName : 'canvas'
>foo : { (tagName: "canvas"): Derived1; (tagName: "div"): Derived2; (tagName: "span"): Derived3; (tagName: string): Base; }
>tagName : "canvas"
>Derived1 : Derived1

function foo(tagName: 'div'): Derived2;
>foo : { (tagName: 'canvas'): Derived1; (tagName: 'div'): Derived2; (tagName: 'span'): Derived3; (tagName: string): Base; }
>tagName : 'div'
>foo : { (tagName: "canvas"): Derived1; (tagName: "div"): Derived2; (tagName: "span"): Derived3; (tagName: string): Base; }
>tagName : "div"
>Derived2 : Derived2

function foo(tagName: 'span'): Derived3;
>foo : { (tagName: 'canvas'): Derived1; (tagName: 'div'): Derived2; (tagName: 'span'): Derived3; (tagName: string): Base; }
>tagName : 'span'
>foo : { (tagName: "canvas"): Derived1; (tagName: "div"): Derived2; (tagName: "span"): Derived3; (tagName: string): Base; }
>tagName : "span"
>Derived3 : Derived3

function foo(tagName: string): Base;
>foo : { (tagName: 'canvas'): Derived1; (tagName: 'div'): Derived2; (tagName: 'span'): Derived3; (tagName: string): Base; }
>foo : { (tagName: "canvas"): Derived1; (tagName: "div"): Derived2; (tagName: "span"): Derived3; (tagName: string): Base; }
>tagName : string
>Base : Base

function foo(tagName: any): Base {
>foo : { (tagName: 'canvas'): Derived1; (tagName: 'div'): Derived2; (tagName: 'span'): Derived3; (tagName: string): Base; }
>foo : { (tagName: "canvas"): Derived1; (tagName: "div"): Derived2; (tagName: "span"): Derived3; (tagName: string): Base; }
>tagName : any
>Base : Base

Expand Down
Loading