Skip to content

Commit

Permalink
Narrowing of variable types using typeof/instanceof type guards
Browse files Browse the repository at this point in the history
  • Loading branch information
ahejlsberg committed Oct 7, 2014
1 parent e836fe1 commit d70494f
Show file tree
Hide file tree
Showing 2 changed files with 277 additions and 35 deletions.
293 changes: 267 additions & 26 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3844,44 +3844,285 @@ module ts {

// EXPRESSION TYPE CHECKING

function checkIdentifier(node: Identifier): Type {
function isInTypeQuery(node: Node): boolean {
// TypeScript 1.0 spec (April 2014): 3.6.3
// A type query consists of the keyword typeof followed by an expression.
// The expression is restricted to a single identifier or a sequence of identifiers separated by periods
while (node) {
function getResolvedSymbol(node: Identifier): Symbol {
var links = getNodeLinks(node);
if (!links.resolvedSymbol) {
links.resolvedSymbol = resolveName(node, node.text, SymbolFlags.Value | SymbolFlags.ExportValue, Diagnostics.Cannot_find_name_0, identifierToString(node)) || unknownSymbol;
}
return links.resolvedSymbol;
}

function isInTypeQuery(node: Node): boolean {
// TypeScript 1.0 spec (April 2014): 3.6.3
// A type query consists of the keyword typeof followed by an expression.
// The expression is restricted to a single identifier or a sequence of identifiers separated by periods
while (node) {
switch (node.kind) {
case SyntaxKind.TypeQuery:
return true;
case SyntaxKind.Identifier:
case SyntaxKind.QualifiedName:
node = node.parent;
continue;
default:
return false;
}
}
Debug.fail("should not get here");
}

// Remove one or more primitive types from a union type
function subtractPrimitiveTypes(type: Type, subtractMask: TypeFlags): Type {
if (type.flags & TypeFlags.Union) {
var types = (<UnionType>type).types;
if (forEach(types, t => t.flags & subtractMask)) {
var newTypes: Type[] = [];
forEach(types, t => {
if (!(t.flags & subtractMask)) {
newTypes.push(t);
}
});

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 7, 2014

Member

Just use filter here.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

Doh!

return getUnionType(newTypes);
}
}
return type;
}

// Check if a given variable is assigned within a given syntax node
function IsVariableAssignedWithin(symbol: Symbol, node: Node): boolean {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

Fix casing of first character

var links = getNodeLinks(node);
if (links.assignmentChecks) {
var cachedResult = links.assignmentChecks[symbol.id];
if (cachedResult !== undefined) {
return cachedResult;
}
}
else {
links.assignmentChecks = {};
}
return links.assignmentChecks[symbol.id] = isAssignedIn(node);

function isAssignedInBinaryExpression(node: BinaryExpression) {
if (node.operator >= SyntaxKind.FirstAssignment && node.operator <= SyntaxKind.LastAssignment) {
var n = node.left;
while (n.kind === SyntaxKind.ParenExpression) {
n = (<ParenExpression>n).expression;
}
if (n.kind === SyntaxKind.Identifier && getResolvedSymbol(<Identifier>n) === symbol) {
return true;
}
}
return forEachChild(node, isAssignedIn);
}

function isAssignedInVariableDeclaration(node: VariableDeclaration) {
if (getSymbolOfNode(node) === symbol && node.initializer) {
return true;
}
return forEachChild(node, isAssignedIn);
}

function isAssignedIn(node: Node): boolean {
switch (node.kind) {
case SyntaxKind.BinaryExpression:
return isAssignedInBinaryExpression(<BinaryExpression>node);
case SyntaxKind.VariableDeclaration:
return isAssignedInVariableDeclaration(<VariableDeclaration>node);
case SyntaxKind.ArrayLiteral:
case SyntaxKind.ObjectLiteral:
case SyntaxKind.PropertyAccess:
case SyntaxKind.IndexedAccess:
case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
case SyntaxKind.TypeAssertion:
case SyntaxKind.ParenExpression:
case SyntaxKind.PrefixOperator:
case SyntaxKind.PostfixOperator:
case SyntaxKind.ConditionalExpression:
case SyntaxKind.Block:
case SyntaxKind.VariableStatement:
case SyntaxKind.ExpressionStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.DoStatement:
case SyntaxKind.WhileStatement:
case SyntaxKind.ForStatement:
case SyntaxKind.ForInStatement:
case SyntaxKind.ReturnStatement:
case SyntaxKind.WithStatement:
case SyntaxKind.SwitchStatement:
case SyntaxKind.CaseClause:
case SyntaxKind.DefaultClause:
case SyntaxKind.LabeledStatement:
case SyntaxKind.ThrowStatement:
case SyntaxKind.TryStatement:
case SyntaxKind.TryBlock:
case SyntaxKind.CatchBlock:
case SyntaxKind.FinallyBlock:

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

Looking at this list of cases, it seems like they all have in common that they can contain expressions (directly or indirectly), and that they do not introduce a function block. It would be worth calling that out, or making helper functions for those concepts. Also, I think it would be worth unifying this with other code that checks expressions / various SyntaxKind sets. The fact that we have these case lists in so many places feels error prone.

Does BinaryExpression include the comma operator?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

I guess I could make it into a forEachAssignment function and move it to parser.ts (where we also have forEachReturnStatement). I will think about it.

Yes, BinaryExpression includes the comma operator.

return forEachChild(node, isAssignedIn);
}
return false;
}
}

// Get the narrowed type of a given symbol at a given location
function getNarrowedTypeOfSymbol(symbol: Symbol, node: Node) {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 7, 2014

Member

Should node just be an Identifier?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

It's not required to be, it's only purpose is to specify a location in the AST.

var type = getTypeOfSymbol(symbol);
// Only narrow when symbol is variable of a non-primitive type
if (symbol.flags & SymbolFlags.Variable && isTypeAnyOrObjectOrTypeParameter(type)) {
while (true) {
var child = node;
node = node.parent;
// Stop at containing function or module block
if (!node || node.kind === SyntaxKind.FunctionBlock || node.kind === SyntaxKind.ModuleBlock) {

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 7, 2014

Member

Will node ever have originally been a SourceFile? If not, just you can toss away !node.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

Yes, node might be a SourceFile, but I kind of like having the !node backstop instead.

break;
}
var narrowedType = type;
switch (node.kind) {
case SyntaxKind.TypeQuery:
return true;
case SyntaxKind.Identifier:
case SyntaxKind.QualifiedName:
node = node.parent;
continue;
default:
return false;
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, child === (<IfStatement>node).thenStatement);

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 7, 2014

Member

Document your third parameter (assumeTrue) for every call to narrowType

}
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, 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).operator === SyntaxKind.AmpersandAmpersandToken) {
narrowedType = narrowType(type, (<BinaryExpression>node).left, true);
}
else if ((<BinaryExpression>node).operator === SyntaxKind.BarBarToken) {
narrowedType = narrowType(type, (<BinaryExpression>node).left, false);
}
}
break;
}
// Only use narrowed type if construct contains no assignments to variable
if (narrowedType !== type && !IsVariableAssignedWithin(symbol, node)) {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

Seems that once isVariableAssignedWithin returns true, you would want to exit the loop. It will be true on every subsequent iteration, correct?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

You're right, we're walking up the tree so once it is true it will continue to be true.

type = narrowedType;
}
}
Debug.fail("should not get here");
}
return type;

function narrowTypeByEquality(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {
var left = <UnaryExpression>expr.left;
var right = <LiteralExpression>expr.right;
// Check that we have 'typeof <symbol>' on the left and string literal on the right

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

I would also handle the reverse: a string literal on the left, and typeof on the right?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

Daniel mentioned the same thing, but I like keeping the pattern simple. Otherwise, should we also recognize all the various parenthesized forms you can construct?

This comment has been minimized.

Copy link
@danquirk

danquirk Oct 8, 2014

Member

I did a quick check on GitHub projects that mention TypeScript in their description (~1000) and only found 6 instances across 2 projects that matched the pattern "typeof ==" and none that match "typeof !=" so it appears that only working with typeof on the LHS ends up covering essentially all real cases.

if (left.kind !== SyntaxKind.PrefixOperator || left.operator !== SyntaxKind.TypeOfKeyword ||
left.operand.kind !== SyntaxKind.Identifier || right.kind !== SyntaxKind.StringLiteral ||
getResolvedSymbol(<Identifier>left.operand) !== symbol) {
return type;
}
var t = right.text;
var checkType: Type = t === "string" ? stringType : t === "number" ? numberType : t === "boolean" ? booleanType : emptyObjectType;
if (expr.operator === SyntaxKind.ExclamationEqualsEqualsToken) {
assumeTrue = !assumeTrue;
}
if (assumeTrue) {
// The assumed result is true. If check was for a primitive type, that type is the narrowed type. Otherwise we can
// remove the primitive types from the narrowed type.
return checkType === emptyObjectType ? subtractPrimitiveTypes(type, TypeFlags.String | TypeFlags.Number | TypeFlags.Boolean) : checkType;
}
else {
// The assumed result is false. If check was for a primitive type we can remove that type from the narrowed type.
// Otherwise we don't have enough information to do anything.
return checkType === emptyObjectType ? type : subtractPrimitiveTypes(type, checkType.flags);
}
}

var symbol = resolveName(node, node.text, SymbolFlags.Value | SymbolFlags.ExportValue, Diagnostics.Cannot_find_name_0, identifierToString(node));
if (!symbol) {
symbol = unknownSymbol;
function narrowTypeByAnd(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {
if (assumeTrue) {
// The assumed result is true, therefore we narrow assuming each operand to be true.
return narrowType(narrowType(type, expr.left, true), expr.right, true);
}
else {
// The assumed result is true. This means either the first operand was false, or the first operand was true

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 8, 2014

Member

Correct to "The assumed result is true".

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

false actually

This comment has been minimized.

Copy link
@DanielRosenwasser

DanielRosenwasser Oct 8, 2014

Member

Amended ;)

// and the second operand was false. We narrow with those assumptions and union the two resulting types.
return getUnionType([narrowType(type, expr.left, false), narrowType(narrowType(type, expr.left, true), expr.right, false)]);
}
}

function narrowTypeByOr(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {
if (assumeTrue) {
// The assumed result is true. This means either the first operand was true, or the first operand was false
// and the second operand was true. We narrow with those assumptions and union the two resulting types.
return getUnionType([narrowType(type, expr.left, true), narrowType(narrowType(type, expr.left, false), expr.right, true)]);
}
else {
// The assumed result is false, therefore we narrow assuming each operand to be false.
return narrowType(narrowType(type, expr.left, false), expr.right, false);
}
}

function narrowTypeByInstanceof(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

Check that assumeTrue is true. Or check it right before calling this function, and remove the parameter.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

Good catch.

// Check that we have variable symbol on the left
if (expr.left.kind !== SyntaxKind.Identifier || getResolvedSymbol(<Identifier>expr.left) !== symbol) {
return type;
}
// Check that right operand is a function type with a prototype property
var rightType = checkExpression(expr.right);
if (!isTypeSubtypeOf(rightType, globalFunctionType)) {
return type;
}
var prototypeProperty = getPropertyOfType(getApparentType(rightType), "prototype");

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

Do you need to do getPropertyOfApparentType / getPropertyOfAugmentedType? That way for types with call/construct signatures, you would get the prototype from Function. Then again, that would just give you any as your narrowed type, so I'm not sure it is valuable.

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

That's right, it would just deliver a result that we'd ignore anyways.

if (!prototypeProperty) {
return type;
}
var prototypeType = getTypeOfSymbol(prototypeProperty);
// Narrow to type of prototype property if it is a subtype of current type
return isTypeSubtypeOf(prototypeType, type) ? prototypeType : type;

This comment has been minimized.

Copy link
@JsonFreeman

JsonFreeman Oct 8, 2014

Contributor

I would comment what the desired result is for each of 3 cases:

  1. prototypeType <: type (I see why you would want to narrow)
  2. prototypeType :> type (I see why you would not want to narrow)
  3. prototypeType and type are unrelated (this is the one I have a question about)

Also, I'm not sure you want subtype here - maybe assignability is better. Because you can have the following:

class C {
    x: number;
}
class D extends C {
    x: any;
    y: any;
}
var v: C = new D;
if (v instanceOf D) {
     v.y; // Would want this to work even though D is not a subtype of C
}

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Oct 8, 2014

Author Member

I think I prefer subtype. Assignability might make the type any (if that's the type of the prototype property) and we wouldn't want that.

}

// Narrow the given type based on the given expression having the assumed boolean value
function narrowType(type: Type, expr: Expression, assumeTrue: boolean): Type {
switch (expr.kind) {
case SyntaxKind.ParenExpression:
return narrowType(type, (<ParenExpression>expr).expression, assumeTrue);
case SyntaxKind.BinaryExpression:
var operator = (<BinaryExpression>expr).operator;
if (operator === SyntaxKind.EqualsEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsEqualsToken) {
return narrowTypeByEquality(type, <BinaryExpression>expr, assumeTrue);
}
else if (operator === SyntaxKind.AmpersandAmpersandToken) {
return narrowTypeByAnd(type, <BinaryExpression>expr, assumeTrue);
}
else if (operator === SyntaxKind.BarBarToken) {
return narrowTypeByOr(type, <BinaryExpression>expr, assumeTrue);
}
else if (operator === SyntaxKind.InstanceOfKeyword) {
return narrowTypeByInstanceof(type, <BinaryExpression>expr, assumeTrue);
}
break;
case SyntaxKind.PrefixOperator:
if ((<UnaryExpression>expr).operator === SyntaxKind.ExclamationToken) {
return narrowType(type, (<UnaryExpression>expr).operand, !assumeTrue);
}
break;
}
return type;
}
}

function checkIdentifier(node: Identifier): Type {
var symbol = getResolvedSymbol(node);

if (symbol.flags & SymbolFlags.Import) {
// Mark the import as referenced so that we emit it in the final .js file.
// exception: identifiers that appear in type queries
getSymbolLinks(symbol).referenced = !isInTypeQuery(node);
}

getNodeLinks(node).resolvedSymbol = symbol;

checkCollisionWithCapturedSuperVariable(node, node);
checkCollisionWithCapturedThisVariable(node, node);
checkCollisionWithIndexVariableInGeneratedCode(node, node);

return getTypeOfSymbol(getExportSymbolOfValueSymbolIfExported(symbol));
return getNarrowedTypeOfSymbol(getExportSymbolOfValueSymbolIfExported(symbol), node);
}

function captureLexicalThis(node: Node, container: Node): void {
Expand Down Expand Up @@ -5134,8 +5375,8 @@ module ts {
return numberType;
}

function isTypeAnyTypeObjectTypeOrTypeParameter(type: Type): boolean {
return type === anyType || ((type.flags & (TypeFlags.ObjectType | TypeFlags.TypeParameter)) !== 0);
function isTypeAnyOrObjectOrTypeParameter(type: Type): boolean {
return (type.flags & (TypeFlags.Any | TypeFlags.ObjectType | TypeFlags.TypeParameter)) !== 0;
}

function checkInstanceOfExpression(node: BinaryExpression, leftType: Type, rightType: Type): Type {
Expand All @@ -5144,7 +5385,7 @@ module ts {
// and the right operand to be of type Any or a subtype of the 'Function' interface type.
// The result is always of the Boolean primitive type.
// NOTE: do not raise error if leftType is unknown as related error was already reported
if (leftType !== unknownType && !isTypeAnyTypeObjectTypeOrTypeParameter(leftType)) {
if (leftType !== unknownType && !isTypeAnyOrObjectOrTypeParameter(leftType)) {
error(node.left, Diagnostics.The_left_hand_side_of_an_instanceof_expression_must_be_of_type_any_an_object_type_or_a_type_parameter);
}
// NOTE: do not raise error if right is unknown as related error was already reported
Expand All @@ -5162,7 +5403,7 @@ module ts {
if (leftType !== anyType && leftType !== stringType && leftType !== numberType) {
error(node.left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_types_any_string_or_number);
}
if (!isTypeAnyTypeObjectTypeOrTypeParameter(rightType)) {
if (!isTypeAnyOrObjectOrTypeParameter(rightType)) {
error(node.right, Diagnostics.The_right_hand_side_of_an_in_expression_must_be_of_type_any_an_object_type_or_a_type_parameter);
}
return booleanType;
Expand Down Expand Up @@ -6338,7 +6579,7 @@ module ts {
var exprType = checkExpression(node.expression);
// unknownType is returned i.e. if node.expression is identifier whose name cannot be resolved
// in this case error about missing name is already reported - do not report extra one
if (!isTypeAnyTypeObjectTypeOrTypeParameter(exprType) && exprType !== unknownType) {
if (!isTypeAnyOrObjectOrTypeParameter(exprType) && exprType !== unknownType) {
error(node.expression, Diagnostics.The_right_hand_side_of_a_for_in_statement_must_be_of_type_any_an_object_type_or_a_type_parameter);
}

Expand Down
Loading

1 comment on commit d70494f

@JsonFreeman
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to say overall that this idea is really awesome!

Please sign in to comment.