Skip to content

Improve type guard consistiency #5442

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

Merged
merged 10 commits into from
Nov 11, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
138 changes: 70 additions & 68 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ namespace ts {
const undefinedType = createIntrinsicType(TypeFlags.Undefined | TypeFlags.ContainsUndefinedOrNull, "undefined");
const nullType = createIntrinsicType(TypeFlags.Null | TypeFlags.ContainsUndefinedOrNull, "null");
const unknownType = createIntrinsicType(TypeFlags.Any, "unknown");
const circularType = createIntrinsicType(TypeFlags.Any, "__circular__");

const emptyObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);
const emptyUnionType = emptyObjectType;
const emptyGenericType = <GenericType><ObjectType>createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);
emptyGenericType.instantiations = {};

Expand Down Expand Up @@ -4333,7 +4333,7 @@ namespace ts {
// a named type that circularly references itself.
function getUnionType(types: Type[], noSubtypeReduction?: boolean): Type {
if (types.length === 0) {
return emptyObjectType;
return emptyUnionType;
}
const typeSet: Type[] = [];
addTypesToSet(typeSet, types, TypeFlags.Union);
Expand Down Expand Up @@ -6203,27 +6203,6 @@ namespace ts {
Debug.fail("should not get here");
}

// For a union type, remove all constituent types that are of the given type kind (when isOfTypeKind is true)
// or not of the given type kind (when isOfTypeKind is false)
function removeTypesFromUnionType(type: Type, typeKind: TypeFlags, isOfTypeKind: boolean, allowEmptyUnionResult: boolean): Type {
if (type.flags & TypeFlags.Union) {
const types = (<UnionType>type).types;
if (forEach(types, t => !!(t.flags & typeKind) === isOfTypeKind)) {
// Above we checked if we have anything to remove, now use the opposite test to do the removal
const narrowedType = getUnionType(filter(types, t => !(t.flags & typeKind) === isOfTypeKind));
if (allowEmptyUnionResult || narrowedType !== emptyObjectType) {
return narrowedType;
}
}
}
else if (allowEmptyUnionResult && !!(type.flags & typeKind) === isOfTypeKind) {
// Use getUnionType(emptyArray) instead of emptyObjectType in case the way empty union types
// are represented ever changes.
return getUnionType(emptyArray);
}
return type;
}

function hasInitializer(node: VariableLikeDeclaration): boolean {
return !!(node.initializer || isBindingPattern(node.parent) && hasInitializer(<VariableLikeDeclaration>node.parent.parent));
}
Expand Down Expand Up @@ -6325,53 +6304,71 @@ namespace ts {
// Only narrow when symbol is variable of type any or an object, union, or type parameter type
if (node && symbol.flags & SymbolFlags.Variable) {
if (isTypeAny(type) || type.flags & (TypeFlags.ObjectType | TypeFlags.Union | TypeFlags.TypeParameter)) {
const originalType = type;
const nodeStack: {node: Node, child: Node}[] = [];
loop: while (node.parent) {
const child = node;
node = node.parent;
let narrowedType = type;
switch (node.kind) {
case SyntaxKind.IfStatement:
case SyntaxKind.ConditionalExpression:
case SyntaxKind.BinaryExpression:
nodeStack.push({node, child});
break;
case SyntaxKind.SourceFile:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.Constructor:
Copy link
Member

Choose a reason for hiding this comment

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

You should just check isFunctionLike

Copy link
Member Author

Choose a reason for hiding this comment

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

isFunctionLike also returns affirmatively for FunctionExpression, ArrowFunction, CallSignature, ConstructSignature, IndexSignature, FunctionType, and ConstructorType - the difference visibly changes our behavior (as in: tests fail). I think these kinds are moreso to find "top-level" declarations where logic outside of it is unlikely to affect the type within the declaration.

// Stop at the first containing function or module declaration
break loop;
}
}

let nodes: {node: Node, child: Node};
while (nodes = nodeStack.pop()) {
const {node, child} = nodes;
switch (node.kind) {
case SyntaxKind.IfStatement:
// In a branch of an if statement, narrow based on controlling expression
if (child !== (<IfStatement>node).expression) {
narrowedType = narrowType(type, (<IfStatement>node).expression, /*assumeTrue*/ child === (<IfStatement>node).thenStatement);
type = narrowType(type, (<IfStatement>node).expression, /*assumeTrue*/ child === (<IfStatement>node).thenStatement);
}
break;
case SyntaxKind.ConditionalExpression:
// In a branch of a conditional expression, narrow based on controlling condition
if (child !== (<ConditionalExpression>node).condition) {
narrowedType = narrowType(type, (<ConditionalExpression>node).condition, /*assumeTrue*/ child === (<ConditionalExpression>node).whenTrue);
type = narrowType(type, (<ConditionalExpression>node).condition, /*assumeTrue*/ child === (<ConditionalExpression>node).whenTrue);
}
break;
case SyntaxKind.BinaryExpression:
// In the right operand of an && or ||, narrow based on left operand
if (child === (<BinaryExpression>node).right) {
if ((<BinaryExpression>node).operatorToken.kind === SyntaxKind.AmpersandAmpersandToken) {
narrowedType = narrowType(type, (<BinaryExpression>node).left, /*assumeTrue*/ true);
type = narrowType(type, (<BinaryExpression>node).left, /*assumeTrue*/ true);
}
else if ((<BinaryExpression>node).operatorToken.kind === SyntaxKind.BarBarToken) {
narrowedType = narrowType(type, (<BinaryExpression>node).left, /*assumeTrue*/ false);
type = narrowType(type, (<BinaryExpression>node).left, /*assumeTrue*/ false);
}
}
break;
case SyntaxKind.SourceFile:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.Constructor:
// Stop at the first containing function or module declaration
break loop;
default:
Debug.fail("Unreachable!");
}
// Use narrowed type if construct contains no assignments to variable
if (narrowedType !== type) {
if (isVariableAssignedWithin(symbol, node)) {
break;
}
type = narrowedType;

// Use original type if construct contains assignments to variable
if (type !== originalType && isVariableAssignedWithin(symbol, node)) {
type = originalType;
}
}

// Preserve old top-level behavior - if the branch is really an empty set, revert to prior type
if (type === emptyUnionType) {
type = originalType;
}
}
}

Expand All @@ -6387,31 +6384,31 @@ namespace ts {
if (left.expression.kind !== SyntaxKind.Identifier || getResolvedSymbol(<Identifier>left.expression) !== symbol) {
return type;
}
const typeInfo = primitiveTypeInfo[right.text];
if (expr.operatorToken.kind === SyntaxKind.ExclamationEqualsEqualsToken) {
assumeTrue = !assumeTrue;
}
if (assumeTrue) {
// Assumed result is true. If check was not for a primitive type, remove all primitive types
if (!typeInfo) {
return removeTypesFromUnionType(type, /*typeKind*/ TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.Boolean | TypeFlags.ESSymbol,
/*isOfTypeKind*/ true, /*allowEmptyUnionResult*/ false);
}
// Check was for a primitive type, return that primitive type if it is a subtype
if (isTypeSubtypeOf(typeInfo.type, type)) {
return typeInfo.type;
}
// Otherwise, remove all types that aren't of the primitive type kind. This can happen when the type is
// union of enum types and other types.
return removeTypesFromUnionType(type, /*typeKind*/ typeInfo.flags, /*isOfTypeKind*/ false, /*allowEmptyUnionResult*/ false);
const typeInfo = primitiveTypeInfo[right.text];
// If the type to be narrowed is any and we're checking a primitive with assumeTrue=true, return the primitive
if (!!(type.flags & TypeFlags.Any) && typeInfo && assumeTrue) {
return typeInfo.type;
}
let flags: TypeFlags;
if (typeInfo) {
flags = typeInfo.flags;
Copy link
Member

Choose a reason for hiding this comment

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

removeTypesFromUnionType is only used one place now, I think. I bet you could considerably simplify the interface and implementation (typeKind and isTypeOfKind and allowEmptyUnionResult at least).

Actually, the lone usage might also be replaceable by getUnionType . filter as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable, I was thinking about the same thing - just wanted to avoid making the change cascade into too many other places. I'll replace the invocation.

}
else {
// Assumed result is false. If check was for a primitive type, remove that primitive type
if (typeInfo) {
return removeTypesFromUnionType(type, /*typeKind*/ typeInfo.flags, /*isOfTypeKind*/ true, /*allowEmptyUnionResult*/ false);
}
// Otherwise we don't have enough information to do anything.
return type;
assumeTrue = !assumeTrue;
flags = TypeFlags.NumberLike | TypeFlags.StringLike | TypeFlags.ESSymbol | TypeFlags.Boolean;
}
// At this point we can bail if it's not a union
if (!(type.flags & TypeFlags.Union)) {
// If the active non-union type would be removed from a union by this type guard, return an empty union
return filterUnion(type) ? type : emptyUnionType;
}
return getUnionType(filter((type as UnionType).types, filterUnion), /*noSubtypeReduction*/ true);

function filterUnion(type: Type) {
return assumeTrue === !!(type.flags & flags);
}
}

Expand All @@ -6425,7 +6422,7 @@ namespace ts {
// and the second operand was false. We narrow with those assumptions and union the two resulting types.
return getUnionType([
narrowType(type, expr.left, /*assumeTrue*/ false),
narrowType(narrowType(type, expr.left, /*assumeTrue*/ true), expr.right, /*assumeTrue*/ false)
narrowType(type, expr.right, /*assumeTrue*/ false)
]);
}
}
Expand All @@ -6436,7 +6433,7 @@ namespace ts {
// and the second operand was true. We narrow with those assumptions and union the two resulting types.
return getUnionType([
narrowType(type, expr.left, /*assumeTrue*/ true),
narrowType(narrowType(type, expr.left, /*assumeTrue*/ false), expr.right, /*assumeTrue*/ true)
narrowType(type, expr.right, /*assumeTrue*/ true)
]);
}
else {
Expand Down Expand Up @@ -12588,9 +12585,14 @@ namespace ts {

// After we remove all types that are StringLike, we will know if there was a string constituent
// based on whether the remaining type is the same as the initial type.
const arrayType = removeTypesFromUnionType(arrayOrStringType, TypeFlags.StringLike, /*isTypeOfKind*/ true, /*allowEmptyUnionResult*/ true);
let arrayType = arrayOrStringType;
if (arrayOrStringType.flags & TypeFlags.Union) {
arrayType = getUnionType(filter((arrayOrStringType as UnionType).types, t => !(t.flags & TypeFlags.StringLike)));
}
else if (arrayOrStringType.flags & TypeFlags.StringLike) {
arrayType = emptyUnionType;
}
const hasStringConstituent = arrayOrStringType !== arrayType;

let reportedError = false;
if (hasStringConstituent) {
if (languageVersion < ScriptTarget.ES5) {
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/symbolType18.types
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,5 @@ if (typeof x === "object") {
}
else {
x;
>x : symbol | Foo
>x : symbol
}
41 changes: 41 additions & 0 deletions tests/baselines/reference/typeGuardEnums.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//// [typeGuardEnums.ts]
enum E {}
enum V {}

let x: number|string|E|V;

if (typeof x === "number") {
x; // number|E|V
}
else {
x; // string
}

if (typeof x !== "number") {
x; // string
}
else {
x; // number|E|V
}


//// [typeGuardEnums.js]
var E;
(function (E) {
})(E || (E = {}));
var V;
(function (V) {
})(V || (V = {}));
var x;
if (typeof x === "number") {
x; // number|E|V
}
else {
x; // string
}
if (typeof x !== "number") {
x; // string
}
else {
x; // number|E|V
}
34 changes: 34 additions & 0 deletions tests/baselines/reference/typeGuardEnums.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardEnums.ts ===
enum E {}
>E : Symbol(E, Decl(typeGuardEnums.ts, 0, 0))

enum V {}
>V : Symbol(V, Decl(typeGuardEnums.ts, 0, 9))

let x: number|string|E|V;
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))
>E : Symbol(E, Decl(typeGuardEnums.ts, 0, 0))
>V : Symbol(V, Decl(typeGuardEnums.ts, 0, 9))

if (typeof x === "number") {
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))

x; // number|E|V
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))
}
else {
x; // string
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))
}

if (typeof x !== "number") {
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))

x; // string
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))
}
else {
x; // number|E|V
>x : Symbol(x, Decl(typeGuardEnums.ts, 3, 3))
}

40 changes: 40 additions & 0 deletions tests/baselines/reference/typeGuardEnums.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardEnums.ts ===
enum E {}
>E : E

enum V {}
>V : V

let x: number|string|E|V;
>x : number | string | E | V
>E : E
>V : V

if (typeof x === "number") {
>typeof x === "number" : boolean
>typeof x : string
>x : number | string | E | V
>"number" : string

x; // number|E|V
>x : number | E | V
}
else {
x; // string
>x : string
}

if (typeof x !== "number") {
>typeof x !== "number" : boolean
>typeof x : string
>x : number | string | E | V
>"number" : string

x; // string
>x : string
}
else {
x; // number|E|V
>x : number | E | V
}

31 changes: 31 additions & 0 deletions tests/baselines/reference/typeGuardNesting.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//// [typeGuardNesting.ts]
let strOrBool: string|boolean;
if ((typeof strOrBool === 'boolean' && !strOrBool) || typeof strOrBool === 'string') {
let label: string = (typeof strOrBool === 'string') ? strOrBool : "string";
let bool: boolean = (typeof strOrBool === 'boolean') ? strOrBool : false;
let label2: string = (typeof strOrBool !== 'boolean') ? strOrBool : "string";
let bool2: boolean = (typeof strOrBool !== 'string') ? strOrBool : false;
}

if ((typeof strOrBool !== 'string' && !strOrBool) || typeof strOrBool !== 'boolean') {
let label: string = (typeof strOrBool === 'string') ? strOrBool : "string";
let bool: boolean = (typeof strOrBool === 'boolean') ? strOrBool : false;
let label2: string = (typeof strOrBool !== 'boolean') ? strOrBool : "string";
let bool2: boolean = (typeof strOrBool !== 'string') ? strOrBool : false;
}


//// [typeGuardNesting.js]
var strOrBool;
if ((typeof strOrBool === 'boolean' && !strOrBool) || typeof strOrBool === 'string') {
var label = (typeof strOrBool === 'string') ? strOrBool : "string";
var bool = (typeof strOrBool === 'boolean') ? strOrBool : false;
var label2 = (typeof strOrBool !== 'boolean') ? strOrBool : "string";
var bool2 = (typeof strOrBool !== 'string') ? strOrBool : false;
}
if ((typeof strOrBool !== 'string' && !strOrBool) || typeof strOrBool !== 'boolean') {
var label = (typeof strOrBool === 'string') ? strOrBool : "string";
var bool = (typeof strOrBool === 'boolean') ? strOrBool : false;
var label2 = (typeof strOrBool !== 'boolean') ? strOrBool : "string";
var bool2 = (typeof strOrBool !== 'string') ? strOrBool : false;
}
Loading