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.

Sign up for GitHub

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

Jump to bottom

This type predicates for type guards #5906

Merged
merged 17 commits into from
Dec 10, 2015
Merged

This type predicates for type guards #5906

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c4cff98
first pass at this type predicates
weswigham Dec 3, 2015
859cc53
Add tests, remove internal annotation form enum
weswigham Dec 3, 2015
d7208e8
feedback from pr
weswigham Dec 3, 2015
cfe2cc3
utilize type guard on type guard logic more
weswigham Dec 3, 2015
d51dd84
remove unneeded cast
weswigham Dec 3, 2015
b9f310d
first pass at this type predicate members
weswigham Dec 4, 2015
be6e341
Fix narrowing, interfaces. Expose issue with generic instantiation
weswigham Dec 8, 2015
b6eb4ae
fix generic instantiation, nested this guards now work
weswigham Dec 8, 2015
4594301
grand unified theory of predicate types
weswigham Dec 9, 2015
0228ec3
preserve some old behavior at @DanielRosenwassers request
weswigham Dec 9, 2015
b0bfa0f
Merge branch 'master' into this-type-guards
weswigham Dec 9, 2015
dc765b8
accept new baseline
weswigham Dec 9, 2015
0284846
most pr feedback
weswigham Dec 9, 2015
8238656
destructuring applied
weswigham Dec 9, 2015
bc01b16
reorder if
weswigham Dec 10, 2015
2885eb2
Merge branch 'master' into this-type-guards
weswigham Dec 10, 2015
8e58694
accept baselines
weswigham Dec 10, 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
306 changes: 231 additions & 75 deletions src/compiler/checker.ts
View file
Open in desktop

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,6 +1632,14 @@
"category": "Error",
"code": 2517
},
"A 'this'-based type guard is not assignable to a parameter-based type guard": {
Copy link
Member

Choose a reason for hiding this comment

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

period, "assignable to or from" or "compatible with"

"category": "Error",
"code": 2518
},
"A 'this'-based type predicate is only allowed in class or interface members, get accessors, or return type positions for functions and methods": {
Copy link
Member

Choose a reason for hiding this comment

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

"A 'this'-based type predicate is only allowed as the return type of functions, methods, and get accessors, or on members of classes and interfaces."

"category": "Error",
"code": 2519
},
"Duplicate identifier '{0}'. Compiler uses declaration '{1}' to support async functions.": {
"category": "Error",
"code": 2520
Expand Down
29 changes: 20 additions & 9 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1959,11 +1959,7 @@ namespace ts {
function parseTypeReferenceOrTypePredicate(): TypeReferenceNode | TypePredicateNode {
const typeName = parseEntityName(/*allowReservedWords*/ false, Diagnostics.Type_expected);
if (typeName.kind === SyntaxKind.Identifier && token === SyntaxKind.IsKeyword && !scanner.hasPrecedingLineBreak()) {
nextToken();
const node = <TypePredicateNode>createNode(SyntaxKind.TypePredicate, typeName.pos);
node.parameterName = <Identifier>typeName;
node.type = parseType();
return finishNode(node);
return parseTypePredicate(typeName as Identifier);
}
const node = <TypeReferenceNode>createNode(SyntaxKind.TypeReference, typeName.pos);
node.typeName = typeName;
Expand All @@ -1973,8 +1969,16 @@ namespace ts {
return finishNode(node);
}

function parseThisTypeNode(): TypeNode {
const node = <TypeNode>createNode(SyntaxKind.ThisType);
function parseTypePredicate(lhs: Identifier | ThisTypeNode): TypePredicateNode {
nextToken();
const node = createNode(SyntaxKind.TypePredicate, lhs.pos) as TypePredicateNode;
node.parameterName = lhs;
node.type = parseType();
return finishNode(node);
}

function parseThisTypeNode(): ThisTypeNode {
const node = createNode(SyntaxKind.ThisType) as ThisTypeNode;
nextToken();
return finishNode(node);
}
Expand Down Expand Up @@ -2420,8 +2424,15 @@ namespace ts {
return parseStringLiteralTypeNode();
case SyntaxKind.VoidKeyword:
return parseTokenNode<TypeNode>();
case SyntaxKind.ThisKeyword:
return parseThisTypeNode();
case SyntaxKind.ThisKeyword: {
const thisKeyword = parseThisTypeNode();
if (token === SyntaxKind.IsKeyword && !scanner.hasPrecedingLineBreak()) {
return parseTypePredicate(thisKeyword);
}
else {
return thisKeyword;
}
}
case SyntaxKind.TypeOfKeyword:
return parseTypeQuery();
case SyntaxKind.OpenBraceToken:
Expand Down
33 changes: 29 additions & 4 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,11 +733,15 @@ namespace ts {
// @kind(SyntaxKind.StringKeyword)
// @kind(SyntaxKind.SymbolKeyword)
// @kind(SyntaxKind.VoidKeyword)
// @kind(SyntaxKind.ThisType)
export interface TypeNode extends Node {
_typeNodeBrand: any;
}

// @kind(SyntaxKind.ThisType)
export interface ThisTypeNode extends TypeNode {
_thisTypeNodeBrand: any;
}

export interface FunctionOrConstructorTypeNode extends TypeNode, SignatureDeclaration {
_functionOrConstructorTypeNodeBrand: any;
}
Expand All @@ -756,7 +760,7 @@ namespace ts {

// @kind(SyntaxKind.TypePredicate)
export interface TypePredicateNode extends TypeNode {
parameterName: Identifier;
parameterName: Identifier | ThisTypeNode;
type: TypeNode;
}

Expand Down Expand Up @@ -1820,10 +1824,25 @@ namespace ts {
CannotBeNamed
}

export const enum TypePredicateKind {
This,
Identifier
}

export interface TypePredicate {
kind: TypePredicateKind;
type: Type;
}

// @kind (TypePredicateKind.This)
export interface ThisTypePredicate extends TypePredicate {
_thisTypePredicateBrand: any;
}

// @kind (TypePredicateKind.Identifier)
export interface IdentifierTypePredicate extends TypePredicate {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like overkill to have two different kinds of TypePredicate types and a dedicated enum. Could you just use parameterIndex = -1 to indicate this?

parameterName: string;
parameterIndex: number;
type: Type;
}

/* @internal */
Expand Down Expand Up @@ -2089,6 +2108,7 @@ namespace ts {
ESSymbol = 0x01000000, // Type of symbol primitive introduced in ES6
ThisType = 0x02000000, // This type
ObjectLiteralPatternWithComputedProperties = 0x04000000, // Object literal type implied by binding pattern has computed properties
PredicateType = 0x08000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does a type predicate act as a type? I think this is the source of the orphaning issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why special case type predicates everywhere with extra slots for them when they're mutually exclusive with a return type (if there is a type predicate, the return type must be boolean)? They even affect type equality (differently than booleans do) when they're nested within a type. Why not unify them all as predicate types? It would be fairly easy to unify them in this way, IMO.

Yes, it being a type is why it follows assignments at present (like all types), but it shouldn't be hard to fix - it's just a matter of making the right branches of getWidenedType return boolean for type predicates.

The more interesting issue I've noted with them right now is that this types aren't properly reinstantiated when narrowing. I can't nest two this type guards, like so:

class FSO {
  isFile: this is File;
  isNetworked: this is (this & Networked);
}
class File {
  path: string;
}
interface Networked {
  host: string;
}
let f: FSO = new File();
if (f.isFile) {
  // f should be File
  if (f.isNetworked) {
    // f should be File & Networked, but is just File 
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that is an interesting issue with the nested 'this' type predicates. It does seem like it should work.

The original reason why type predicates were special cased, was to prevent them from flowing around. That could be taken care of with widening, but it still requires type relations for type predicates. The main thing is that the a type predicate is in intimately tied to one of the parameters in its associated signature (or this), so once it departs from the signature or property, it is really just a boolean. Is widening really enough to make sure it doesn't travel around?

Copy link
Member Author

Choose a reason for hiding this comment

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

It certainly seems like it, given that we widen on assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I am thinking about it. You may be right actually. It's possible we did not think of widening at the time. @mhegazy or @tinganho might have thoughts on this point of whether to treat type predicates as types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the issue - it was simply that this type guards weren't re-instantiated by instantiateType. 👍


/* @internal */
Intrinsic = Any | String | Number | Boolean | ESSymbol | Void | Undefined | Null,
Copy link
Member

Choose a reason for hiding this comment

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

More than anything else, can you leave a comment about the interaction here with intrinsicName?

Expand Down Expand Up @@ -2121,6 +2141,11 @@ namespace ts {
intrinsicName: string; // Name of intrinsic type
}

// Predicate types (TypeFlags.Predicate)
export interface PredicateType extends Type {
predicate: ThisTypePredicate;
}

// String literal types (TypeFlags.StringLiteral)
export interface StringLiteralType extends Type {
text: string; // Text of string literal
Expand Down Expand Up @@ -2237,7 +2262,7 @@ namespace ts {
declaration: SignatureDeclaration; // Originating declaration
typeParameters: TypeParameter[]; // Type parameters (undefined if non-generic)
parameters: Symbol[]; // Parameters
typePredicate?: TypePredicate; // Type predicate
typePredicate?: ThisTypePredicate | IdentifierTypePredicate; // Type predicate
/* @internal */
resolvedReturnType: Type; // Resolved return type
/* @internal */
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,10 @@ namespace ts {
return node && node.kind === SyntaxKind.MethodDeclaration && node.parent.kind === SyntaxKind.ObjectLiteralExpression;
}

export function isIdentifierTypePredicate(predicate: TypePredicate): predicate is IdentifierTypePredicate {
return predicate && predicate.kind === TypePredicateKind.Identifier;
}

export function getContainingFunction(node: Node): FunctionLikeDeclaration {
while (true) {
node = node.parent;
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/typeGuardFunctionErrors.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(91,1):
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(96,9): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(97,16): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(98,20): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(104,25): error TS1228: A type predicate is only allowed in return type position for functions and methods.
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(104,25): error TS2519: A 'this'-based type predicate is only allowed in class or interface members, get accessors, or return type positions for functions and methods
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(105,16): error TS2322: Type 'boolean' is not assignable to type 'D'.
Property 'm1' is missing in type 'Boolean'.
tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(105,16): error TS2409: Return type of constructor signature must be assignable to the instance type of the class
Expand Down Expand Up @@ -194,7 +194,7 @@ tests/cases/conformance/expressions/typeGuards/typeGuardFunctionErrors.ts(137,39
class D {
constructor(p1: A): p1 is C {
~~~~~~~
!!! error TS1228: A type predicate is only allowed in return type position for functions and methods.
!!! error TS2519: A 'this'-based type predicate is only allowed in class or interface members, get accessors, or return type positions for functions and methods
return true;
~~~~
!!! error TS2322: Type 'boolean' is not assignable to type 'D'.
Expand Down
Loading