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

Allow type predicates as return types only #5992

Merged
merged 11 commits into from
Jan 13, 2016

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Dec 8, 2015

It does this in the parser instead of piecemeal in the checker. This means that the checker has some unused error checking code, so I took the opportunity to move the type predicate checking code into checkTypePredicate instead of checkSignatureDeclaration where it was previously.

Fixed #5903 and #5731. Obsoletes PRs #5936 and #5977.

@ahejlsberg and @weswigham, can you take a look?

return id;
}
});
const t = parseType();
Copy link
Member

Choose a reason for hiding this comment

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

Rename t to type

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

@@ -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.

@sandersn sandersn added this to the TypeScript 1.8 milestone Dec 8, 2015
@sandersn
Copy link
Member Author

sandersn commented Dec 9, 2015

@ahejlsberg does this look good now?

@DanielRosenwasser
Copy link
Member

You might want to wait on @weswigham's work as a heads up.

Also:
1. Remove notes I wrote myself for merging.
2. Switch to pattern matching on properties in a few places.
@sandersn
Copy link
Member Author

@DanielRosenwasser I merged to get Wesley's changes. The parser may need some work to forbid this is X but I'll do that in a separate change I think.

@sandersn
Copy link
Member Author

sandersn commented Jan 6, 2016

I'd like to get this in for 1.8. @ahejlsberg, @weswigham, can you take another look?

case SyntaxKind.PropertySignature:
case SyntaxKind.GetAccessor:
return node === (node.parent as (PropertyDeclaration | GetAccessorDeclaration | PropertySignature)).type;
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

checkBindingPatternForTypeVariable ->
checkIfTypeVariableIsDeclaredInBindingPattern
@sandersn
Copy link
Member Author

@ahejlsberg can you take a look so I can merge this for 1.8?

let typePredicateVariable: Identifier;
if (isIdentifier()) {
typePredicateVariable = tryParse(parseTypePredicatePrefix);
}
Copy link
Member

Choose a reason for hiding this comment

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

const typePredicateVariable = isIdentifier() && tryParse(parseTypePredicatePrefix);

@ahejlsberg
Copy link
Member

👍 With my minor suggested change.

sandersn added a commit that referenced this pull request Jan 13, 2016
…rn-types-only

Allow type predicates as return types only
@sandersn sandersn merged commit 911d07a into master Jan 13, 2016
@sandersn sandersn deleted the allow-type-predicates-as-return-types-only branch January 13, 2016 17:55
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants