-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Infer string literals at comparison locations #6196
Changes from 8 commits
0d2fb26
639d9bf
13ec5d7
e109b17
de9789a
d0a8573
e452955
ced65ac
881b52d
8365410
58580ed
069ff33
2874156
e2ddb29
f7e9135
e6bd7ad
16fe01b
bc34ebb
4eced90
01cc2f1
259a3cf
cc2ab55
5b9e5d1
fdd7fde
09d1762
dec70f1
1dd43fa
ab25584
9673152
6b8bac3
740792c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7334,24 +7334,37 @@ namespace ts { | |
return undefined; | ||
} | ||
|
||
function getContextualTypeForBinaryOperand(node: Expression): Type { | ||
function getContextualTypeForBinaryOperand(node: Expression, literalNode: StringLiteral): Type { | ||
const binaryExpression = <BinaryExpression>node.parent; | ||
const operator = binaryExpression.operatorToken.kind; | ||
if (operator >= SyntaxKind.FirstAssignment && operator <= SyntaxKind.LastAssignment) { | ||
|
||
if (SyntaxKind.FirstAssignment <= operator && operator <= SyntaxKind.LastAssignment) { | ||
// In an assignment expression, the right operand is contextually typed by the type of the left operand. | ||
if (node === binaryExpression.right) { | ||
return checkExpression(binaryExpression.left); | ||
} | ||
return undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably don't need this |
||
} | ||
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); | ||
if (!type && node === binaryExpression.right) { | ||
type = checkExpression(binaryExpression.left); | ||
} | ||
return type; | ||
|
||
switch (operator) { | ||
case 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 = getContextualTypeWorker(binaryExpression, literalNode); | ||
if (!type && node === binaryExpression.right) { | ||
type = checkExpression(binaryExpression.left); | ||
} | ||
return type; | ||
case SyntaxKind.EqualsEqualsEqualsToken: | ||
case SyntaxKind.ExclamationEqualsEqualsToken: | ||
case SyntaxKind.EqualsEqualsToken: | ||
case SyntaxKind.ExclamationEqualsToken: | ||
if (literalNode) { | ||
return getStringLiteralTypeForText(literalNode.text); | ||
} | ||
break; | ||
} | ||
|
||
return undefined; | ||
} | ||
|
||
|
@@ -7459,9 +7472,9 @@ namespace ts { | |
} | ||
|
||
// In a contextually typed conditional expression, the true/false expressions are contextually typed by the same type. | ||
function getContextualTypeForConditionalOperand(node: Expression): Type { | ||
function getContextualTypeForConditionalOperand(node: Expression, literalNode: StringLiteral): Type { | ||
const conditional = <ConditionalExpression>node.parent; | ||
return node === conditional.whenTrue || node === conditional.whenFalse ? getContextualType(conditional) : undefined; | ||
return node === conditional.whenTrue || node === conditional.whenFalse ? getContextualTypeWorker(conditional, literalNode) : undefined; | ||
} | ||
|
||
function getContextualTypeForJsxExpression(expr: JsxExpression | JsxSpreadAttribute): Type { | ||
|
@@ -7509,6 +7522,14 @@ namespace ts { | |
* @returns the contextual type of an expression. | ||
*/ | ||
function getContextualType(node: Expression): Type { | ||
return getContextualTypeWorker(node, /*literalNode*/ undefined); | ||
} | ||
|
||
function getLiteralContextualType(literalNode: StringLiteral) { | ||
return getContextualTypeWorker(literalNode, literalNode); | ||
} | ||
|
||
function getContextualTypeWorker(node: Expression, literalNode: StringLiteral): Type { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just make a separate function that has separate rules for the contextual string literal types. Right now, it is just being packed along in the calls to getContextualTypeWorker, but whenever it's needed, it results in a completely different code path. To me that's a sign that it really has nothing to do with contextual types. In other words, I think it's more straightforward to implement it as its own walk up the AST. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about it on the bus and I agree. |
||
if (isInsideWithStatementBody(node)) { | ||
// We cannot answer semantic questions within a with block, do not proceed any further | ||
return undefined; | ||
|
@@ -7536,21 +7557,27 @@ namespace ts { | |
case SyntaxKind.AsExpression: | ||
return getTypeFromTypeNode((<AssertionExpression>parent).type); | ||
case SyntaxKind.BinaryExpression: | ||
return getContextualTypeForBinaryOperand(node); | ||
return getContextualTypeForBinaryOperand(node, literalNode); | ||
case SyntaxKind.PropertyAssignment: | ||
return getContextualTypeForObjectLiteralElement(<ObjectLiteralElement>parent); | ||
case SyntaxKind.ArrayLiteralExpression: | ||
return getContextualTypeForElementExpression(node); | ||
case SyntaxKind.ConditionalExpression: | ||
return getContextualTypeForConditionalOperand(node); | ||
return getContextualTypeForConditionalOperand(node, literalNode); | ||
case SyntaxKind.TemplateSpan: | ||
Debug.assert(parent.parent.kind === SyntaxKind.TemplateExpression); | ||
return getContextualTypeForSubstitutionExpression(<TemplateExpression>parent.parent, node); | ||
case SyntaxKind.ParenthesizedExpression: | ||
return getContextualType(<ParenthesizedExpression>parent); | ||
return getContextualTypeWorker(<ParenthesizedExpression>parent, literalNode); | ||
case SyntaxKind.JsxExpression: | ||
case SyntaxKind.JsxSpreadAttribute: | ||
return getContextualTypeForJsxExpression(<JsxExpression>parent); | ||
case SyntaxKind.SwitchStatement: | ||
case SyntaxKind.CaseClause: | ||
if (literalNode) { | ||
return getStringLiteralTypeForText(literalNode.text); | ||
} | ||
break; | ||
} | ||
return undefined; | ||
} | ||
|
@@ -9869,11 +9896,7 @@ namespace ts { | |
if (produceDiagnostics && targetType !== unknownType) { | ||
const widenedType = getWidenedType(exprType); | ||
|
||
// Permit 'number[] | "foo"' to be asserted to 'string'. | ||
const bothAreStringLike = | ||
someConstituentTypeHasKind(targetType, TypeFlags.StringLike) && | ||
someConstituentTypeHasKind(widenedType, TypeFlags.StringLike); | ||
if (!bothAreStringLike && !(isTypeAssignableTo(targetType, widenedType))) { | ||
if (!isTypeAssignableTo(targetType, widenedType)) { | ||
checkTypeAssignableTo(exprType, targetType, node, Diagnostics.Neither_type_0_nor_type_1_is_assignable_to_the_other); | ||
} | ||
} | ||
|
@@ -10723,10 +10746,6 @@ namespace ts { | |
case SyntaxKind.ExclamationEqualsToken: | ||
case SyntaxKind.EqualsEqualsEqualsToken: | ||
case SyntaxKind.ExclamationEqualsEqualsToken: | ||
// Permit 'number[] | "foo"' to be asserted to 'string'. | ||
if (someConstituentTypeHasKind(leftType, TypeFlags.StringLike) && someConstituentTypeHasKind(rightType, TypeFlags.StringLike)) { | ||
return booleanType; | ||
} | ||
if (!isTypeAssignableTo(leftType, rightType) && !isTypeAssignableTo(rightType, leftType)) { | ||
reportOperatorError(); | ||
} | ||
|
@@ -10866,7 +10885,7 @@ namespace ts { | |
} | ||
|
||
function checkStringLiteralExpression(node: StringLiteral): Type { | ||
const contextualType = getContextualType(node); | ||
const contextualType = getLiteralContextualType(node); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this even need to be a type? I mean, do you actually use the type for anything, or just check if it exists? I feel like a boolean is more appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That actually might be a better way to go about it. |
||
if (contextualType && contextualTypeIsStringLiteralType(contextualType)) { | ||
return getStringLiteralTypeForText(node.text); | ||
} | ||
|
@@ -13249,7 +13268,6 @@ namespace ts { | |
let hasDuplicateDefaultClause = false; | ||
|
||
const expressionType = checkExpression(node.expression); | ||
const expressionTypeIsStringLike = someConstituentTypeHasKind(expressionType, TypeFlags.StringLike); | ||
forEach(node.caseBlock.clauses, clause => { | ||
// Grammar check for duplicate default clauses, skip if we already report duplicate default clause | ||
if (clause.kind === SyntaxKind.DefaultClause && !hasDuplicateDefaultClause) { | ||
|
@@ -13271,12 +13289,7 @@ namespace ts { | |
// In a 'switch' statement, each 'case' expression must be of a type that is assignable to or from the type of the 'switch' expression. | ||
const caseType = checkExpression(caseClause.expression); | ||
|
||
const expressionTypeIsAssignableToCaseType = | ||
// Permit 'number[] | "foo"' to be asserted to 'string'. | ||
(expressionTypeIsStringLike && someConstituentTypeHasKind(caseType, TypeFlags.StringLike)) || | ||
isTypeAssignableTo(expressionType, caseType); | ||
|
||
if (!expressionTypeIsAssignableToCaseType) { | ||
if (!isTypeAssignableTo(expressionType, caseType)) { | ||
// 'expressionType is not assignable to caseType', try the reversed check and report errors if it fails | ||
checkTypeAssignableTo(caseType, expressionType, caseClause.expression, /*headMessage*/ undefined); | ||
} | ||
|
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.
Purely stylistic, but everywhere else in the compiler we always put the constant on the right hand side (as the code was originally written). I'd prefer to keep things consistent.