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

Allow type predicates as return types only #5992

Merged
merged 11 commits into from
Jan 13, 2016
Merged

Allow type predicates as return types only #5992

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
af7df83
Parse type predicates only in return types.
sandersn Dec 8, 2015
fd311d4
Fix lint
sandersn Dec 8, 2015
1b63040
Add tests and accept baselines
sandersn Dec 8, 2015
47d267c
Move type predicate checking to checkTypePredicate
sandersn Dec 8, 2015
3306ee8
Accept baselines
sandersn Dec 8, 2015
a4e21d7
Address comments
sandersn Dec 8, 2015
f9846ff
Address comments
sandersn Dec 8, 2015
7882fd3
Merge branch 'master' into allow-type-predicates-as-return-types-only
sandersn Dec 22, 2015
9ab9940
Remove unused error for this-type predicates.
sandersn Dec 22, 2015
9b13a0c
Better name for checkTypePredicate helper function
sandersn Jan 6, 2016
a9f2cb6
Make `parseTypeOrTypePredicate` terser.
sandersn Jan 13, 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
136 changes: 66 additions & 70 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10985,7 +10985,43 @@ namespace ts {
return -1;
}

function isInLegalTypePredicatePosition(node: Node): boolean {
function checkTypePredicate(node: TypePredicateNode) {
const parent = getTypePredicateParent(node);
if (!parent) {
return;
}
const typePredicate = getSignatureFromDeclaration(parent).typePredicate;
if (typePredicate.parameterIndex >= 0) {
if (parent.parameters[typePredicate.parameterIndex].dotDotDotToken) {
error(node.parameterName,
Diagnostics.A_type_predicate_cannot_reference_a_rest_parameter);
}
else {
checkTypeAssignableTo(typePredicate.type,
getTypeOfNode(parent.parameters[typePredicate.parameterIndex]),
node.type);
}
}
else if (node.parameterName) {
let hasReportedError = false;
for (const param of parent.parameters) {
if ((param.name.kind === SyntaxKind.ObjectBindingPattern ||
param.name.kind === SyntaxKind.ArrayBindingPattern) &&
checkBindingPatternForTypePredicateVariable(
<BindingPattern>param.name,
node.parameterName,
typePredicate.parameterName)) {
hasReportedError = true;
break;
}
}
if (!hasReportedError) {
error(node.parameterName, Diagnostics.Cannot_find_parameter_0, typePredicate.parameterName);
}
}
}

function getTypePredicateParent(node: Node): SignatureDeclaration {
switch (node.parent.kind) {
case SyntaxKind.ArrowFunction:
case SyntaxKind.CallSignature:
Expand All @@ -10994,9 +11030,35 @@ namespace ts {
case SyntaxKind.FunctionType:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
return node === (<SignatureDeclaration>node.parent).type;
const parent = <SignatureDeclaration>node.parent;
if (node === parent.type) {
return parent;
}
}
}

function checkBindingPatternForTypePredicateVariable(
Copy link
Member

Choose a reason for hiding this comment

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

Consider checkIfTypePredicateVariableIsDeclaredInBindingPattern

Copy link
Member Author

Choose a reason for hiding this comment

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

what about isTypePredicateVariableDeclaredInBindingPattern

Copy link
Member

Choose a reason for hiding this comment

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

I considered that, but it also reports errors. I think that would still be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I forgot about that -- check* is a better prefix. (This is just a move of the code, so I hadn't looked too closely at the contents.)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

pattern: BindingPattern,
predicateVariableNode: Node,
predicateVariableName: string) {
for (const element of pattern.elements) {
if (element.name.kind === SyntaxKind.Identifier &&
(<Identifier>element.name).text === predicateVariableName) {
error(predicateVariableNode,
Diagnostics.A_type_predicate_cannot_reference_element_0_in_a_binding_pattern,
predicateVariableName);
return true;
}
else if (element.name.kind === SyntaxKind.ArrayBindingPattern ||
element.name.kind === SyntaxKind.ObjectBindingPattern) {
if (checkBindingPatternForTypePredicateVariable(
<BindingPattern>element.name,
predicateVariableNode,
predicateVariableName)) {
return true;
}
}
}
return false;
}

function checkSignatureDeclaration(node: SignatureDeclaration) {
Expand All @@ -11015,67 +11077,7 @@ namespace ts {

forEach(node.parameters, checkParameter);

if (node.type) {
if (node.type.kind === SyntaxKind.TypePredicate) {
const typePredicate = getSignatureFromDeclaration(node).typePredicate;
const typePredicateNode = <TypePredicateNode>node.type;
if (isInLegalTypePredicatePosition(typePredicateNode)) {
if (typePredicate.parameterIndex >= 0) {
if (node.parameters[typePredicate.parameterIndex].dotDotDotToken) {
error(typePredicateNode.parameterName,
Diagnostics.A_type_predicate_cannot_reference_a_rest_parameter);
}
else {
checkTypeAssignableTo(typePredicate.type,
getTypeOfNode(node.parameters[typePredicate.parameterIndex]),
typePredicateNode.type);
}
}
else if (typePredicateNode.parameterName) {
let hasReportedError = false;
for (var param of node.parameters) {
if (hasReportedError) {
break;
}
if (param.name.kind === SyntaxKind.ObjectBindingPattern ||
param.name.kind === SyntaxKind.ArrayBindingPattern) {

(function checkBindingPattern(pattern: BindingPattern) {
for (const element of pattern.elements) {
if (element.name.kind === SyntaxKind.Identifier &&
(<Identifier>element.name).text === typePredicate.parameterName) {

error(typePredicateNode.parameterName,
Diagnostics.A_type_predicate_cannot_reference_element_0_in_a_binding_pattern,
typePredicate.parameterName);
hasReportedError = true;
break;
}
else if (element.name.kind === SyntaxKind.ArrayBindingPattern ||
element.name.kind === SyntaxKind.ObjectBindingPattern) {

checkBindingPattern(<BindingPattern>element.name);
}
}
})(<BindingPattern>param.name);
}
}
if (!hasReportedError) {
error(typePredicateNode.parameterName,
Diagnostics.Cannot_find_parameter_0,
typePredicate.parameterName);
}
}
}
else {
error(typePredicateNode,
Diagnostics.A_type_predicate_is_only_allowed_in_return_type_position_for_functions_and_methods);
}
}
else {
checkSourceElement(node.type);
}
}
checkSourceElement(node.type);

if (produceDiagnostics) {
checkCollisionWithArgumentsInGeneratedCode(node);
Expand Down Expand Up @@ -14217,12 +14219,6 @@ namespace ts {
}
}

function checkTypePredicate(node: TypePredicateNode) {
if (!isInLegalTypePredicatePosition(node)) {
error(node, Diagnostics.A_type_predicate_is_only_allowed_in_return_type_position_for_functions_and_methods);
}
}

function checkSourceElement(node: Node): void {
if (!node) {
return;
Expand Down
4 changes: 0 additions & 4 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,10 +711,6 @@
"category": "Error",
"code": 1227
},
"A type predicate is only allowed in return type position for functions and methods.": {
"category": "Error",
"code": 1228
},
"A type predicate cannot reference a rest parameter.": {
"category": "Error",
"code": 1229
Expand Down
53 changes: 34 additions & 19 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,17 +908,19 @@ namespace ts {
return result;
}

// Invokes the provided callback then unconditionally restores the parser to the state it
// was in immediately prior to invoking the callback. The result of invoking the callback
// is returned from this function.
/** Invokes the provided callback then unconditionally restores the parser to the state it
* was in immediately prior to invoking the callback. The result of invoking the callback
* is returned from this function.
*/
function lookAhead<T>(callback: () => T): T {
return speculationHelper(callback, /*isLookAhead*/ true);
}

// Invokes the provided callback. If the callback returns something falsy, then it restores
// the parser to the state it was in immediately prior to invoking the callback. If the
// callback returns something truthy, then the parser state is not rolled back. The result
// of invoking the callback is returned from this function.
/** Invokes the provided callback. If the callback returns something falsy, then it restores
* the parser to the state it was in immediately prior to invoking the callback. If the
* callback returns something truthy, then the parser state is not rolled back. The result
* of invoking the callback is returned from this function.
*/
function tryParse<T>(callback: () => T): T {
return speculationHelper(callback, /*isLookAhead*/ false);
}
Expand Down Expand Up @@ -1960,15 +1962,8 @@ namespace ts {

// TYPES

function parseTypeReferenceOrTypePredicate(): TypeReferenceNode | TypePredicateNode {
function parseTypeReference(): TypeReferenceNode {
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);
}
const node = <TypeReferenceNode>createNode(SyntaxKind.TypeReference, typeName.pos);
node.typeName = typeName;
if (!scanner.hasPrecedingLineBreak() && token === SyntaxKind.LessThanToken) {
Expand Down Expand Up @@ -2100,10 +2095,10 @@ namespace ts {

if (returnTokenRequired) {
parseExpected(returnToken);
signature.type = parseType();
signature.type = parseTypeOrTypePredicate();
}
else if (parseOptional(returnToken)) {
signature.type = parseType();
signature.type = parseTypeOrTypePredicate();
}
}

Expand Down Expand Up @@ -2419,7 +2414,7 @@ namespace ts {
case SyntaxKind.SymbolKeyword:
// If these are followed by a dot, then parse these out as a dotted type reference instead.
const node = tryParse(parseKeywordAndNoDot);
return node || parseTypeReferenceOrTypePredicate();
return node || parseTypeReference();
case SyntaxKind.StringLiteral:
return parseStringLiteralTypeNode();
case SyntaxKind.VoidKeyword:
Expand All @@ -2435,7 +2430,7 @@ namespace ts {
case SyntaxKind.OpenParenToken:
return parseParenthesizedType();
default:
return parseTypeReferenceOrTypePredicate();
return parseTypeReference();
}
}

Expand Down Expand Up @@ -2542,6 +2537,26 @@ namespace ts {
return false;
}

function parseTypeOrTypePredicate(): TypeNode {
const typePredicateVariable = tryParse(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Only call tryParse if a call to isIdentifier() is true. No point in incurring the tryParse overhead if it isn't. Also, don't pass an arrow function to tryParse. Instead, make a function called nextTokenIsIsKeyword or some such and pass that. Avoids creating a function object on every invocation.

const id = parseIdentifier();
if (token === SyntaxKind.IsKeyword && !scanner.hasPrecedingLineBreak()) {
nextToken();
return id;
}
});
const type = parseType();
if (typePredicateVariable) {
const node = <TypePredicateNode>createNode(SyntaxKind.TypePredicate, typePredicateVariable.pos);
node.parameterName = typePredicateVariable;
node.type = type;
return finishNode(node);
}
else {
return type;
}
}

function parseType(): TypeNode {
// The rules about 'yield' only apply to actual code/expression contexts. They don't
// apply to 'type' contexts. So we disable these parameters here before moving on.
Expand Down
52 changes: 51 additions & 1 deletion tests/baselines/reference/typeAssertions.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,23 @@ tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(37,13): err
Property 'q' is missing in type 'SomeDerived'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(38,13): error TS2352: Neither type 'SomeBase' nor type 'SomeOther' is assignable to the other.
Property 'q' is missing in type 'SomeBase'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,5): error TS2304: Cannot find name 'numOrStr'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,14): error TS1005: '>' expected.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,14): error TS2304: Cannot find name 'is'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,17): error TS1005: ')' expected.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,17): error TS2304: Cannot find name 'string'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(44,48): error TS1005: ';' expected.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(45,2): error TS2322: Type 'number | string' is not assignable to type 'string'.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,32): error TS2304: Cannot find name 'numOrStr'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,41): error TS1005: ')' expected.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,41): error TS2304: Cannot find name 'is'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,44): error TS1005: ';' expected.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,44): error TS2304: Cannot find name 'string'.
tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(48,50): error TS1005: ';' expected.


==== tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts (5 errors) ====
==== tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts (18 errors) ====
// Function call whose argument is a 1 arg generic function call with explicit type arguments
function fn1<T>(t: T) { }
function fn2(t: any) { }
Expand Down Expand Up @@ -64,5 +78,41 @@ tests/cases/conformance/expressions/typeAssertions/typeAssertions.ts(38,13): err
!!! error TS2352: Property 'q' is missing in type 'SomeBase'.
someOther = <SomeOther>someOther;

// Type assertion cannot be a type-predicate type
var numOrStr: number | string;
var str: string;
if(<numOrStr is string>(numOrStr === undefined)) { // Error
~~~~~~~~
!!! error TS2304: Cannot find name 'numOrStr'.
~~
!!! error TS1005: '>' expected.
~~
!!! error TS2304: Cannot find name 'is'.
~~~~~~
!!! error TS1005: ')' expected.
~~~~~~
!!! error TS2304: Cannot find name 'string'.
~
!!! error TS1005: ';' expected.
str = numOrStr; // Error, no narrowing occurred
~~~
!!! error TS2322: Type 'number | string' is not assignable to type 'string'.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
}

if((numOrStr === undefined) as numOrStr is string) { // Error
~~~~~~~~
!!! error TS2304: Cannot find name 'numOrStr'.
~~
!!! error TS1005: ')' expected.
~~
!!! error TS2304: Cannot find name 'is'.
~~~~~~
!!! error TS1005: ';' expected.
~~~~~~
!!! error TS2304: Cannot find name 'string'.
~
!!! error TS1005: ';' expected.
}


22 changes: 22 additions & 0 deletions tests/baselines/reference/typeAssertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ someOther = <SomeOther>someDerived; // Error
someOther = <SomeOther>someBase; // Error
someOther = <SomeOther>someOther;

// Type assertion cannot be a type-predicate type
var numOrStr: number | string;
var str: string;
if(<numOrStr is string>(numOrStr === undefined)) { // Error
str = numOrStr; // Error, no narrowing occurred
}

if((numOrStr === undefined) as numOrStr is string) { // Error
}



Expand Down Expand Up @@ -87,3 +96,16 @@ someDerived = someOther; // Error
someOther = someDerived; // Error
someOther = someBase; // Error
someOther = someOther;
// Type assertion cannot be a type-predicate type
var numOrStr;
var str;
if (is)
string > (numOrStr === undefined);
{
str = numOrStr; // Error, no narrowing occurred
}
if ((numOrStr === undefined))
is;
string;
{
}
Loading