Fix type when annotated with a JSDoc function type #22692

sandersn · 2018-03-19T16:53:55Z

Previously,

A variable annotated with a JSDoc function type would not require all its parameters to be provided. This should only apply to functions without a type annotation.
A parameter in a function with a JSDoc function type annotation would still have the type 'any'.
Two var declarations in a Typescript and Javascript file, respectively, would error even when they had identical function types.

Fixes #22080

I don't think there are bugs filed for (2) or (3).

Previously, 1. A variable annotated with a JSDoc function type would not require all its parameters to be provided. This should only apply to functions without a type annotation. 2. A parameter in a function with a JSDoc function type annotation would still have the type 'any'. 3. Two `var` declarations in a Typescript and Javascript file, respectively, would error even when they had identical function types.

weswigham

Don't construct signatures need to be handled in the same way?

weswigham · 2018-03-19T16:55:53Z

src/compiler/checker.ts

+                const isUntypedSignatureInJSFile = !iife &&
+                    !isJSConstructSignature &&
+                    isInJavaScriptFile(declaration) &&
+                    declaration.kind !== SyntaxKind.FunctionType &&


ConstructorType?

sandersn · 2018-03-19T17:33:18Z

Yep, good catch. Also I forgot to update baselines. A few of them changed now that more types are available.

weswigham · 2018-03-19T17:49:25Z

src/compiler/checker.ts

@@ -6699,7 +6699,14 @@ namespace ts {
                let hasThisParameter: boolean;
                const iife = getImmediatelyInvokedFunctionExpression(declaration);
                const isJSConstructSignature = isJSDocConstructSignature(declaration);
-                const isUntypedSignatureInJSFile = !iife && !isJSConstructSignature && isInJavaScriptFile(declaration) && !hasJSDocParameterTags(declaration);
+                const isUntypedSignatureInJSFile = !iife &&


What case is handling when the declaration is an object literal with call/construct signatures, (ie, {(): void})?

!getJSDocType(declaration). I'll add a test case. Note that the new code in getJSDocType doesn't look deep enough to get type of the parameter out of the method declaration inside the object literal type.

ghost · 2018-03-19T18:36:15Z

src/compiler/checker.ts

@@ -6699,7 +6699,14 @@ namespace ts {
                let hasThisParameter: boolean;
                const iife = getImmediatelyInvokedFunctionExpression(declaration);
                const isJSConstructSignature = isJSDocConstructSignature(declaration);
-                const isUntypedSignatureInJSFile = !iife && !isJSConstructSignature && isInJavaScriptFile(declaration) && !hasJSDocParameterTags(declaration);
+                const isUntypedSignatureInJSFile = !iife &&
+                    !isJSConstructSignature &&


!isJSConstructSignature is redundant if we test declaration.kind !== SyntaxKind.FunctionType anyway.

Nice catch. Removed.

ghost · 2018-03-19T18:38:06Z

src/compiler/utilities.ts

+                if (typeTag && typeTag.typeExpression && isFunctionLike(typeTag.typeExpression.type)) {
+                    const i = node.parent.parameters.indexOf(node);
+                    if (i > -1) {
+                        return typeTag.typeExpression.type.parameters[i].type;


Needs a length check -- we don't know that typeTag.typeExpression.type has the same # parameters as the actual function.

Instead of a syntactic check in getJSDocTag

sandersn · 2018-03-19T20:25:32Z

src/compiler/checker.ts

-                    return type;
+            if (!isContextSensitiveFunctionOrObjectLiteralMethod(func)) {
+                return undefined;
+            }


This change is just an inversion of the guard and a subsequent un-indent.

sandersn · 2018-03-19T20:27:36Z

@weswigham I moved the change from getJSDocType to getContextualSignature after our discussion this morning.

weswigham · 2018-03-19T21:30:20Z

tests/baselines/reference/checkJsdocTypeTagOnObjectProperty2.errors.txt

-!!! error TS2322:   Types of parameters 'num' and 'arg0' are incompatible.
-!!! error TS2322:     Type 'number' is not assignable to type 'string | undefined'.
+                  ~~~~~~~
+!!! error TS2322: Type '"0"' is not assignable to type 'number'.


Do you know why we infer a literal type here now? AFAIK we only generate a literal if the contextual type of the literal is the same domain as the literal itself, which would imply that the contextual type here is string, which seems wrong, given the annotation above.

The inferred type of "0" is for the initialiser, which was already of type "0" (see the type baselines below). You get the same error in Typescript with an type annotation, too:

function f(x: number = "hi") { }

Gives the error "Type "hi" is not assignable to type 'number'." This error is expected now that num is of type number from contextual typing. Previously the arrow was typed with no contextual type, then assigned to arrowFunc and checked against its type annotation.

It's not great, but it's consistent with the rest of the the compiler. Note that our baselines have gone back and forth on this in the last year or two.

weswigham · 2018-03-19T21:55:19Z

src/compiler/checker.ts

-                const isUntypedSignatureInJSFile = !iife && !isJSConstructSignature && isInJavaScriptFile(declaration) && !hasJSDocParameterTags(declaration);
+                const isUntypedSignatureInJSFile = !iife &&
+                    isInJavaScriptFile(declaration) &&
+                    declaration.kind !== SyntaxKind.FunctionType &&


Rather than skipping (some of the) the type-space signature declaration kinds, can we just check if it is a concrete declaration kind? That is the point here, right; to exclude type-space declaration kinds?

export type ConcreteSignatureDeclaration = | FunctionDeclaration | MethodDeclaration | ConstructorDeclaration | AccessorDeclaration | FunctionExpression | ArrowFunction; export function isConcreteSignatureDeclaration(declaration: Node): node is ConcreteSignatureDeclaration { return isFunctionExpression(declaration) || isArrowFunction(declaration) || isMethodOrAccessor(declaration) || isFunctionDeclaration(declaration) || isConstructorDeclaration(declaration); }

Yes, a type signature is by definition not untyped, even if it's in a JS file. I used your code in the new commit.

Instead of excluding type signatures piecemeal.

brendankenny · 2018-03-27T21:06:00Z

@sandersn Not sure how your point releases work exactly, but any chance of this being pulled into 2.8.2 or whatever is next? It appears it didn't make it into 2.8.1 and it unlocks a lot of new type checking/inference goodness via jsdocs :)

sandersn · 2018-03-27T21:16:14Z

Chances are not good. We keep point releases down to fixing regressions from the major release, and the bugs fixed in this PR have been around for ages. We have a major release every 6-8 weeks, so 2.9 should at least have a release candidate by then. :)

brendankenny · 2018-03-27T21:19:02Z

Got it, no problem. Thanks! And thanks for all the work getting these working.

sandersn requested review from weswigham and a user March 19, 2018 16:54

sandersn mentioned this pull request Mar 19, 2018

Required parameters are not checked in JS files #22080

Closed

weswigham reviewed Mar 19, 2018

View reviewed changes

sandersn added 2 commits March 19, 2018 10:32

Update baselines and add constructor test

d79b4df

Handle ConstructorType too

34c977e

weswigham reviewed Mar 19, 2018

View reviewed changes

Add test:method sig inside literal type

dfbae12

ghost reviewed Mar 19, 2018

View reviewed changes

ghost mentioned this pull request Mar 19, 2018

Put simpler condition in front of more complex one #22696

Merged

Contextually type parameters by parent sig's JSDoc

1f6106b

Instead of a syntactic check in getJSDocTag

sandersn commented Mar 19, 2018

View reviewed changes

Remove redundant check:isUntypedSignatureInJSFile

e09bd63

weswigham reviewed Mar 19, 2018

View reviewed changes

Positive check for value signatures

deba8e2

Instead of excluding type signatures piecemeal.

weswigham approved these changes Mar 19, 2018

View reviewed changes

sandersn merged commit b56093f into master Mar 19, 2018

sandersn deleted the jsdoc-require-parameters-when-function-type-annotated branch March 19, 2018 23:00

brendankenny mentioned this pull request Mar 28, 2018

core(tsc): add type checking to use of CRDP events GoogleChrome/lighthouse#4886

Merged

microsoft locked and limited conversation to collaborators Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type when annotated with a JSDoc function type #22692

Fix type when annotated with a JSDoc function type #22692

sandersn commented Mar 19, 2018

weswigham left a comment

weswigham Mar 19, 2018

sandersn commented Mar 19, 2018

weswigham Mar 19, 2018 •

edited

Loading

sandersn Mar 19, 2018

ghost Mar 19, 2018

sandersn Mar 19, 2018

ghost Mar 19, 2018

sandersn Mar 19, 2018

sandersn commented Mar 19, 2018

weswigham Mar 19, 2018

sandersn Mar 19, 2018

weswigham Mar 19, 2018

sandersn Mar 19, 2018

weswigham Mar 19, 2018

sandersn Mar 19, 2018

brendankenny commented Mar 27, 2018 •

edited

Loading

sandersn commented Mar 27, 2018

brendankenny commented Mar 27, 2018

Fix type when annotated with a JSDoc function type #22692

Fix type when annotated with a JSDoc function type #22692

Conversation

sandersn commented Mar 19, 2018

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Mar 19, 2018

weswigham Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Mar 27, 2018 • edited Loading

sandersn commented Mar 27, 2018

brendankenny commented Mar 27, 2018

weswigham Mar 19, 2018 •

edited

Loading

brendankenny commented Mar 27, 2018 •

edited

Loading