Skip to content

Allow type predicates in JSDoc #26343

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 6 commits into from
Aug 10, 2018
Merged

Allow type predicates in JSDoc #26343

merged 6 commits into from
Aug 10, 2018

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Aug 9, 2018

Previously, non-this type predicates almost worked. They just needed a couple of lines of code.

  1. Parse type predicates. Note that they are parsed everywhere, and get the appropriate error when used places besides a return type.
  2. When creating a type predicate, correctly find the function's parameters starting from the jsdoc return type.

Fixes #26297

1. Parse type predicates. Note that they are parsed everywhere, and get
the appropriate error when used places besides a return type.
2. When creating a type predicate, correctly find the function's parameters
starting from the jsdoc return type.
@sandersn sandersn requested review from RyanCavanaugh and a user August 9, 2018 21:00
@@ -7408,10 +7408,11 @@ namespace ts {
function createTypePredicateFromTypePredicateNode(node: TypePredicateNode): IdentifierTypePredicate | ThisTypePredicate {
const { parameterName } = node;
const type = getTypeFromTypeNode(node.type);
const func = isJSDocTypeExpression(node.parent) ? getJSDocHost(node.parent) as FunctionLikeDeclaration : node.parent;
Copy link

Choose a reason for hiding this comment

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

node.parent.kind === SyntaxKind.JSDocTypeExpression; is a compile error. (#25316)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the correct fix is to change the type of TypePredicateNode.parent. Do you agree?

Copy link

Choose a reason for hiding this comment

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

👍
Also, the only reason the cast is valid is because the only place we call this function is getEffectiveReturnTypeNode(signature.declaration); -- could just pass signature.declaration in as a parameter func: SignatureDeclaration | JSDocSignature to avoid computing this.
Then getTypePredicateParameterIndex needs to be updated to function getTypePredicateParameterIndex(parameterList: ReadonlyArray<ParameterDeclaration | JSDocParameterTag>, parameter: Identifier) (and remove the if since parameters are non-optional).

Copy link

Choose a reason for hiding this comment

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

Previous comment may have been a little rambling so I'll rewrite. It is basically 3 comments.

In createTypePredicateFromTypePredicate you compute const func = ...etc....
createTypePredicateFromTypePredicate is only called in exactly one place, getTypePredicateOfSignature. In getTypePredicateOfSignature you already have access to signature.declaration. So you could add a second parameter to createTypeFromTypePredicateNode, func: SignatureDeclaration | JSDocSignature, rather than computing it inside the function.

I also had a somewhat unrelated comment to make: getTypePredicateParameterIndex can take defined inputs, so parameterList: NodeArray<ParameterDeclaration>, parameter: Identifier instead of parameterList: NodeArray<ParameterDeclaration> | undefined, parameter: Identifier | undefined. Then you can remove the if at the start of that function.

But, you'll note that once we are passing in func as a parameter without a cast, we've needed to allow JSDocSignature. That means that parameterList in getTypePredicateParameterIndex needs to be of type ReadonlyArray<ParameterDeclaration | JSDocParameterTag>, which just happens to work without changing any of the actual code in that function.

@@ -7408,10 +7408,11 @@ namespace ts {
function createTypePredicateFromTypePredicateNode(node: TypePredicateNode): IdentifierTypePredicate | ThisTypePredicate {
const { parameterName } = node;
const type = getTypeFromTypeNode(node.type);
const func = isJSDocTypeExpression(node.parent) ? getJSDocHost(node.parent) as FunctionLikeDeclaration : node.parent;
if (parameterName.kind === SyntaxKind.Identifier) {
return createIdentifierTypePredicate(
parameterName && parameterName.escapedText as string, // TODO: GH#18217
Copy link

Choose a reason for hiding this comment

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

We just accessed a property on parameterName, so it must be defined.

@ghost
Copy link

ghost commented Aug 10, 2018

Failing test case:

/**
 * @callback Cb
 * @param {unknown} x
 * @return {x is number}
 */

/** @type {Cb} */
function isNumber(x) { return typeof x === "number" }

/** @param {unknown} x */
function g(x) {
    if (isNumber(x)) {
        x * 2;
    }
}

@sandersn sandersn merged commit a6c5d50 into master Aug 10, 2018
@sandersn sandersn deleted the jsdoc/allow-type-predicates branch August 10, 2018 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant