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

Infer string literals at comparison locations #6196

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0d2fb26
Added tests.
DanielRosenwasser Dec 21, 2015
639d9bf
Accepted baselines.
DanielRosenwasser Dec 22, 2015
13ec5d7
Infer string literal types at comparison locations.
DanielRosenwasser Dec 22, 2015
e109b17
Accepted baselines.
DanielRosenwasser Dec 22, 2015
de9789a
Removed ad-hoc string-like checking for type assertions, switch/cases…
DanielRosenwasser Dec 22, 2015
d0a8573
Accepted baselines.
DanielRosenwasser Dec 22, 2015
e452955
Amended tests.
DanielRosenwasser Dec 22, 2015
ced65ac
Accepted baselines.
DanielRosenwasser Dec 22, 2015
881b52d
Separated logic out to 'shouldAcquireLiteralType'.
DanielRosenwasser Dec 22, 2015
8365410
Amended comment.
DanielRosenwasser Dec 22, 2015
58580ed
Rewrote loop, amended comment.
DanielRosenwasser Dec 22, 2015
069ff33
Add conditional expressions, be more conservative in checks for '||'.
DanielRosenwasser Dec 23, 2015
2874156
Added more to test.
DanielRosenwasser Dec 23, 2015
e2ddb29
Accepted baselines.
DanielRosenwasser Dec 23, 2015
f7e9135
Merge branch 'master' into literalTypeLocations
DanielRosenwasser Jan 5, 2016
e6bd7ad
Added test.
DanielRosenwasser Jan 5, 2016
16fe01b
Accepted baselines.
DanielRosenwasser Jan 5, 2016
bc34ebb
Added RHS operands of '&&' and ',' as match locations.
DanielRosenwasser Jan 5, 2016
4eced90
Accepted baselines.
DanielRosenwasser Jan 5, 2016
01cc2f1
Added tests for type assertions in match locations.
DanielRosenwasser Jan 8, 2016
259a3cf
Accepted baselines.
DanielRosenwasser Jan 8, 2016
cc2ab55
Factored out operand detection logic and made it recursive.
DanielRosenwasser Jan 8, 2016
5b9e5d1
Included type assertions in operand detection logic.
DanielRosenwasser Jan 8, 2016
fdd7fde
Accepted baselines.
DanielRosenwasser Jan 8, 2016
09d1762
Fix lint issues.
DanielRosenwasser Jan 8, 2016
dec70f1
Added scenario to tests.
DanielRosenwasser Jan 8, 2016
1dd43fa
Accepted baselines.
DanielRosenwasser Jan 8, 2016
ab25584
Merge branch 'master' into literalTypeLocations
DanielRosenwasser Jan 8, 2016
9673152
Merge branch 'master' into literalTypeLocations
DanielRosenwasser Apr 5, 2016
6b8bac3
Accepted other baselines.
DanielRosenwasser Apr 5, 2016
740792c
Merge branch 'master' into literalTypeLocations
DanielRosenwasser Jun 10, 2016
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
83 changes: 64 additions & 19 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7337,11 +7337,13 @@ namespace ts {
function getContextualTypeForBinaryOperand(node: Expression): 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) {
Copy link
Member

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.

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ||
Expand All @@ -7352,6 +7354,7 @@ namespace ts {
}
return type;
}

return undefined;
}

Expand Down Expand Up @@ -7509,6 +7512,7 @@ namespace ts {
* @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 @@ -7555,6 +7559,62 @@ namespace ts {
return undefined;
}

function shouldAcquireLiteralType(literalNode: StringLiteral) {
Copy link
Member

Choose a reason for hiding this comment

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

An alternative way to structure this is to have an isEqualityComparisonOperand function that returns true if the node is the operand of an equality comparison (including switch and case). You could then add a call to this function to the existing code in checkStringLiteralExpression. As it is now, this function mixes two concerns (equality comparison operand and presence of string literal contextual type) that really are separate.


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove whitespace

Copy link
Member

Choose a reason for hiding this comment

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

And elsewhere in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

The whitespace here and below is intentional and it's for readability so that everything doesn't look like one big clump of code.

let current: Node = literalNode;
while (true) {
const { parent } = current;
switch (parent.kind) {
// The operand of a 'switch' should get a literal type.
case SyntaxKind.SwitchStatement:
return current === (parent as SwitchStatement).expression;

// The tested expression of a 'case' clause should get a literal type.
case SyntaxKind.CaseClause:
return current === (parent as CaseClause).expression;

case SyntaxKind.BinaryExpression:
const binaryExpr = parent as BinaryExpression;
switch (binaryExpr.operatorToken.kind) {
// Either operand of an equality/inequality comparison
// should get a literal type.
case SyntaxKind.EqualsEqualsEqualsToken:
case SyntaxKind.ExclamationEqualsEqualsToken:
case SyntaxKind.EqualsEqualsToken:
case SyntaxKind.ExclamationEqualsToken:
return current === binaryExpr.left || current === binaryExpr.right;
Copy link
Member

Choose a reason for hiding this comment

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

Don't think current could ever be anything but the the left or right operand.


case SyntaxKind.BarBarToken:
current = parent;
continue;

}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just break from the loop directly (using a label). If you do that you can put the break in a default clause. Then you can remove the final break at the bottom. Then you can convert the continues to breaks. And that will allow you to put current = parent at the bottom of the loop body instead of before each continue statement. I think it would make for a cleaner loop. It also avoids the double break.

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 originally had it that way, but I thought it would be too indirect. I could go either way on the style, so I'll just change it back.

Copy link
Member

Choose a reason for hiding this comment

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

I like Jason's idea, so that's two votes for.


case SyntaxKind.ConditionalExpression:
case SyntaxKind.ParenthesizedExpression:
current = parent;
continue;
}

// This is not a node we can account for.
// Let contextual typing take over.
break;
}

// We haven't found a "literal match location" (i.e. a location that signals
// a literal should get a literal type). Check whether the contextual type
// has a literal type in it.
//
// We could perform our walk, check if 'current' is an expression when we get to a
// a node that isn't a match location, and then get the contextual type of that.
// That would save a few steps but the checks in 'isExpression' seem so involved
// that it would probably be better to simply grab the contextual type if we didn't.
const contextualType = getContextualType(literalNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just pass current or parent here?

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 guess I assumed too much when I wrote the above comment. getContextualType expects an Expression, so we'd need to check if current is an expression using isExpression. But that function is so roundabout, I figured we'd be better off just performing the walk from literalNode again.

Additionally, you need to because we just skipped ||, where the RHS gets contextually typed by the LHS if a parent contextual type doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see what you mean now about isExpression. And good point about ||.

return !!contextualType && contextualTypeIsStringLiteralType(contextualType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to do this? What is an unsupported node?

Copy link
Member Author

Choose a reason for hiding this comment

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

You still want to get the contextual type in case you didn't hit a ===, case, switch, etc.

I'll add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I get it now, thanks. My question was, why does the contextual type matter at all? And now I realize you do still care whether the contextual type specifies a string literal, for basic cases like this:

var s: "hello" = "hello";

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly it

}


// If the given type is an object or union type, if that type has a single signature, and if
// that signature is non-generic, return the signature. Otherwise return undefined.
function getNonGenericSignature(type: Type): Signature {
Expand Down Expand Up @@ -9869,11 +9929,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);
}
}
Expand Down Expand Up @@ -10723,10 +10779,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();
}
Expand Down Expand Up @@ -10866,8 +10918,7 @@ namespace ts {
}

function checkStringLiteralExpression(node: StringLiteral): Type {
const contextualType = getContextualType(node);
if (contextualType && contextualTypeIsStringLiteralType(contextualType)) {
if (shouldAcquireLiteralType(node)) {
return getStringLiteralTypeForText(node.text);
}

Expand Down Expand Up @@ -13249,7 +13300,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) {
Expand All @@ -13271,12 +13321,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);
}
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/TypeGuardWithEnumUnion.types
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function f1(x: Color | string) {
>typeof x === "number" : boolean
>typeof x : string
>x : Color | string
>"number" : string
>"number" : "number"

var y = x;
>y : Color
Expand Down Expand Up @@ -43,7 +43,7 @@ function f2(x: Color | string | string[]) {
>typeof x === "object" : boolean
>typeof x : string
>x : Color | string | string[]
>"object" : string
>"object" : "object"

var y = x;
>y : string[]
Expand All @@ -56,7 +56,7 @@ function f2(x: Color | string | string[]) {
>typeof x === "number" : boolean
>typeof x : string
>x : Color | string | string[]
>"number" : string
>"number" : "number"

var z = x;
>z : Color
Expand All @@ -78,7 +78,7 @@ function f2(x: Color | string | string[]) {
>typeof x === "string" : boolean
>typeof x : string
>x : Color | string | string[]
>"string" : string
>"string" : "string"

var a = x;
>a : string
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/anonymousClassExpression1.types
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,5 @@ function f() {
>typeof class {} === "function" : boolean
>typeof class {} : string
>class {} : typeof (Anonymous class)
>"function" : string
>"function" : "function"
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class A {
>v : string

case "test": use(this);
>"test" : string
>"test" : "test"
>use(this) : void
>use : (a: any) => void
>this : this
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ if (typeof x !== "string") {
>typeof x !== "string" : boolean
>typeof x : string
>x : string | StringTreeCollection
>"string" : string
>"string" : "string"

x[0] = "";
>x[0] = "" : string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ typeof "123" == "string" ? exprBoolean1 : exprBoolean2;
>typeof "123" == "string" : boolean
>typeof "123" : string
>"123" : string
>"string" : string
>"string" : "string"
>exprBoolean1 : boolean
>exprBoolean2 : boolean

Expand Down Expand Up @@ -264,7 +264,7 @@ var resultIsBoolean3 = typeof "123" == "string" ? exprBoolean1 : exprBoolean2;
>typeof "123" == "string" : boolean
>typeof "123" : string
>"123" : string
>"string" : string
>"string" : "string"
>exprBoolean1 : boolean
>exprBoolean2 : boolean

Expand Down Expand Up @@ -301,7 +301,7 @@ var resultIsStringOrBoolean4 = typeof "123" === "string" ? exprString1 : exprBoo
>typeof "123" === "string" : boolean
>typeof "123" : string
>"123" : string
>"string" : string
>"string" : "string"
>exprString1 : string
>exprBoolean1 : boolean

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function foo(a: string): string | number {
if (a === "hello") {
>a === "hello" : boolean
>a : string
>"hello" : string
>"hello" : "hello"

return a.length;
>a.length : number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class Wat {
>() => test == 'abc' : () => boolean
>test == 'abc' : boolean
>test : string
>'abc' : string
>'abc' : "abc"

static whatever() {
>whatever : () => void
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/es3defaultAliasIsQuoted.types
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ assert(Foo.CONSTANT === "Foo");
>Foo.CONSTANT : string
>Foo : typeof Foo
>CONSTANT : string
>"Foo" : string
>"Foo" : "Foo"

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ if (typeof x !== "string") {
>typeof x !== "string" : boolean
>typeof x : string
>x : string | StringTreeArray
>"string" : string
>"string" : "string"

x.push("");
>x.push("") : number
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,5 @@ thing.doSomething((x, y) => x.name === "bob"); // should not error
>x.name : string
>x : { name: string; id: number; }
>name : string
>"bob" : string
>"bob" : "bob"

Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ module Bugs {
>args[index] : any
>args : any[]
>index : any
>'undefined' : string
>'undefined' : "undefined"

? args[index]
>args[index] : any
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/overloadReturnTypes.types
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function attr(nameOrMap: any, value?: string): any {
>typeof nameOrMap === "object" : boolean
>typeof nameOrMap : string
>nameOrMap : any
>"object" : string
>"object" : "object"

// handle map case
return new Accessor;
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/stringLiteralCheckedInIf01.types
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ function f(foo: T) {
if (foo === "a") {
>foo === "a" : boolean
>foo : ("a" | "b")[] | "a" | "b"
>"a" : string
>"a" : "a"

return foo;
>foo : ("a" | "b")[] | "a" | "b"
}
else if (foo === "b") {
>foo === "b" : boolean
>foo : ("a" | "b")[] | "a" | "b"
>"b" : string
>"b" : "b"

return foo;
>foo : ("a" | "b")[] | "a" | "b"
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/stringLiteralCheckedInIf02.types
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ function isS(t: T): t is S {
>t === "a" || t === "b" : boolean
>t === "a" : boolean
>t : ("a" | "b")[] | "a" | "b"
>"a" : string
>"a" : "a"
>t === "b" : boolean
>t : ("a" | "b")[] | "a" | "b"
>"b" : string
>"b" : "b"
}

function f(foo: T) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ switch (foo) {
>foo : ("a" | "b")[] | "a" | "b"

case "a":
>"a" : string
>"a" : "a"

case "b":
>"b" : string
>"b" : "b"

break;
default:
Expand Down
55 changes: 55 additions & 0 deletions tests/baselines/reference/stringLiteralTypeAssertion01.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
tests/cases/conformance/types/stringLiteral/stringLiteralTypeAssertion01.ts(22,5): error TS2352: Neither type 'string' nor type '("a" | "b")[] | "a" | "b"' is assignable to the other.
Type 'string' is not assignable to type '"b"'.
tests/cases/conformance/types/stringLiteral/stringLiteralTypeAssertion01.ts(23,5): error TS2352: Neither type 'string' nor type '("a" | "b")[] | "a" | "b"' is assignable to the other.
Type 'string' is not assignable to type '"b"'.
tests/cases/conformance/types/stringLiteral/stringLiteralTypeAssertion01.ts(30,7): error TS2352: Neither type '("a" | "b")[] | "a" | "b"' nor type 'string' is assignable to the other.
Type '("a" | "b")[]' is not assignable to type 'string'.
tests/cases/conformance/types/stringLiteral/stringLiteralTypeAssertion01.ts(31,7): error TS2352: Neither type '("a" | "b")[] | "a" | "b"' nor type 'string' is assignable to the other.
Type '("a" | "b")[]' is not assignable to type 'string'.


==== tests/cases/conformance/types/stringLiteral/stringLiteralTypeAssertion01.ts (4 errors) ====

type S = "a" | "b";
type T = S[] | S;
Copy link
Member

Choose a reason for hiding this comment

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

it would be easier to read to use the naming scheme

type Ab = "a" | "b"; 
type AbOrAbs = Ab[] | Ab; 
var ab: Ab; ...


var s: S;
var t: T;
var str: string;

////////////////

s = <S>t;
s = t as S;

s = <S>str;
s = str as S;

////////////////

t = <T>s;
t = s as T;

t = <T>str;
~~~~~~
!!! error TS2352: Neither type 'string' nor type '("a" | "b")[] | "a" | "b"' is assignable to the other.
!!! error TS2352: Type 'string' is not assignable to type '"b"'.
t = str as T;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to test two different syntaxes to make sure they behave the same? It seems like it's not the point of this 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 guess I don't see why it's a problem to have more coverage given that subtle inconsistencies are found all the time.

~~~~~~~~
!!! error TS2352: Neither type 'string' nor type '("a" | "b")[] | "a" | "b"' is assignable to the other.
!!! error TS2352: Type 'string' is not assignable to type '"b"'.

////////////////

str = <string>s;
str = s as string;

str = <string>t;
~~~~~~~~~
!!! error TS2352: Neither type '("a" | "b")[] | "a" | "b"' nor type 'string' is assignable to the other.
!!! error TS2352: Type '("a" | "b")[]' is not assignable to type 'string'.
str = t as string;
~~~~~~~~~~~
!!! error TS2352: Neither type '("a" | "b")[] | "a" | "b"' nor type 'string' is assignable to the other.
!!! error TS2352: Type '("a" | "b")[]' is not assignable to type 'string'.

Loading