Skip to content

Fix type when annotated with a JSDoc function type #22692

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

Merged
sandersn merged 7 commits into from
Mar 19, 2018
Merged

Fix type when annotated with a JSDoc function type #22692

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aeaf83c
Fix type when annotated with a JSDoc function type
sandersn Mar 19, 2018
d79b4df
Update baselines and add constructor test
sandersn Mar 19, 2018
34c977e
Handle ConstructorType too
sandersn Mar 19, 2018
dfbae12
Add test:method sig inside literal type
sandersn Mar 19, 2018
1f6106b
Contextually type parameters by parent sig's JSDoc
sandersn Mar 19, 2018
e09bd63
Remove redundant check:isUntypedSignatureInJSFile
sandersn Mar 19, 2018
deba8e2
Positive check for value signatures
sandersn Mar 19, 2018
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
9 changes: 8 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Copy Markdown
Member

@weswigham weswigham Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@sandersn sandersn Mar 19, 2018

Choose a reason for hiding this comment

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

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

!isJSConstructSignature &&
Copy link
Copy Markdown

@ghost ghost Mar 19, 2018

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@sandersn sandersn Mar 19, 2018

Choose a reason for hiding this comment

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

Nice catch. Removed.

isInJavaScriptFile(declaration) &&
Copy link
Copy Markdown
Member

@weswigham weswigham Mar 19, 2018

Choose a reason for hiding this comment

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

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);
}

Copy link
Copy Markdown
Member Author

@sandersn sandersn Mar 19, 2018

Choose a reason for hiding this comment

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

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.

declaration.kind !== SyntaxKind.FunctionType &&
Copy link
Copy Markdown
Member

@weswigham weswigham Mar 19, 2018

Choose a reason for hiding this comment

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

ConstructorType?

declaration.kind !== SyntaxKind.ConstructorType &&
declaration.kind !== SyntaxKind.JSDocFunctionType &&
!hasJSDocParameterTags(declaration) &&
!getJSDocType(declaration);

// If this is a JSDoc construct signature, then skip the first parameter in the
// parameter list. The first parameter represents the return type of the construct
Expand Down
9 changes: 9 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4510,6 +4510,15 @@ namespace ts {
let tag: JSDocTypeTag | JSDocParameterTag | undefined = getFirstJSDocTag(node, isJSDocTypeTag);
if (!tag && isParameter(node)) {
tag = find(getJSDocParameterTags(node), tag => !!tag.typeExpression);
if (!tag && isFunctionLike(node.parent)) {
const typeTag = getJSDocTypeTag(node.parent);
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;
Copy link
Copy Markdown

@ghost ghost Mar 19, 2018

Choose a reason for hiding this comment

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

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

}
}
}
}

return tag && tag.typeExpression && tag.typeExpression.type;
Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/checkJsdocTypeTagOnObjectProperty1.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ const obj = {

/** @type {function(number): number} */
method1(n1) {
>method1 : (n1: any) => any
>n1 : any
>method1 : (n1: number) => number
>n1 : number

return n1 + 42;
>n1 + 42 : any
>n1 : any
>n1 + 42 : number
>n1 : number
>42 : 42

},
Expand All @@ -44,10 +44,10 @@ const obj = {
/** @type {function(number): number} */
arrowFunc: (num) => num + 42
>arrowFunc : (arg0: number) => number
>(num) => num + 42 : (num: any) => any
>num : any
>num + 42 : any
>num : any
>(num) => num + 42 : (num: number) => number
>num : number
>num + 42 : number
>num : number
>42 : 42
}
obj.foo = 'string'
Expand Down
18 changes: 7 additions & 11 deletions tests/baselines/reference/checkJsdocTypeTagOnObjectProperty2.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
tests/cases/conformance/jsdoc/0.js(5,3): error TS2322: Type 'number' is not assignable to type 'string | undefined'.
tests/cases/conformance/jsdoc/0.js(7,3): error TS2322: Type '(n1: any) => string' is not assignable to type '(arg0: number) => number'.
tests/cases/conformance/jsdoc/0.js(7,3): error TS2322: Type '(n1: number) => string' is not assignable to type '(arg0: number) => number'.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/jsdoc/0.js(11,3): error TS2322: Type '(n1: any) => string' is not assignable to type '(arg0: number) => number'.
tests/cases/conformance/jsdoc/0.js(11,3): error TS2322: Type '(n1: number) => string' is not assignable to type '(arg0: number) => number'.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/jsdoc/0.js(13,3): error TS2322: Type '(num?: string) => string' is not assignable to type '(arg0: number) => number'.
Types of parameters 'num' and 'arg0' are incompatible.
Type 'number' is not assignable to type 'string | undefined'.
tests/cases/conformance/jsdoc/0.js(13,15): error TS2322: Type '"0"' is not assignable to type 'number'.
tests/cases/conformance/jsdoc/0.js(15,3): error TS2322: Type 'undefined' is not assignable to type 'string'.
tests/cases/conformance/jsdoc/0.js(19,5): error TS2322: Type 'number' is not assignable to type 'string'.
tests/cases/conformance/jsdoc/0.js(22,22): error TS2345: Argument of type '"0"' is not assignable to parameter of type 'number'.
Expand All @@ -22,21 +20,19 @@ tests/cases/conformance/jsdoc/0.js(22,22): error TS2345: Argument of type '"0"'
/** @type {function(number): number} */
method1(n1) {
~~~~~~~
!!! error TS2322: Type '(n1: any) => string' is not assignable to type '(arg0: number) => number'.
!!! error TS2322: Type '(n1: number) => string' is not assignable to type '(arg0: number) => number'.
!!! error TS2322: Type 'string' is not assignable to type 'number'.
return "42";
},
/** @type {function(number): number} */
method2: (n1) => "lol",
~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type '(n1: any) => string' is not assignable to type '(arg0: number) => number'.
!!! error TS2322: Type '(n1: number) => string' is not assignable to type '(arg0: number) => number'.
!!! error TS2322: Type 'string' is not assignable to type 'number'.
/** @type {function(number): number} */
arrowFunc: (num="0") => num + 42,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type '(num?: string) => string' is not assignable to type '(arg0: number) => number'.
!!! 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'.
Copy link
Copy Markdown
Member

@weswigham weswigham Mar 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@sandersn sandersn Mar 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@weswigham weswigham Mar 19, 2018

Choose a reason for hiding this comment

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

Hm. OK.

Copy link
Copy Markdown
Member Author

@sandersn sandersn Mar 19, 2018

Choose a reason for hiding this comment

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

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.

/** @type {string} */
lol
~~~
Expand Down
16 changes: 8 additions & 8 deletions tests/baselines/reference/checkJsdocTypeTagOnObjectProperty2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ const obj = {

/** @type {function(number): number} */
method1(n1) {
>method1 : (n1: any) => string
>n1 : any
>method1 : (n1: number) => string
>n1 : number

return "42";
>"42" : "42"
Expand All @@ -24,18 +24,18 @@ const obj = {
/** @type {function(number): number} */
method2: (n1) => "lol",
>method2 : (arg0: number) => number
>(n1) => "lol" : (n1: any) => string
>n1 : any
>(n1) => "lol" : (n1: number) => string
>n1 : number
>"lol" : "lol"

/** @type {function(number): number} */
arrowFunc: (num="0") => num + 42,
>arrowFunc : (arg0: number) => number
>(num="0") => num + 42 : (num?: string) => string
>num : string
>(num="0") => num + 42 : (num?: number) => number
>num : number
>"0" : "0"
>num + 42 : string
>num : string
>num + 42 : number
>num : number
>42 : 42

/** @type {string} */
Expand Down
14 changes: 14 additions & 0 deletions tests/baselines/reference/jsdocTypeTag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ var obj;

/** @type {Function} */
var Func;

/** @type {(s: string) => number} */
var f;

/** @type {new (s: string) => { s: string }} */
var ctor;

//// [b.ts]
var S: string;
Expand All @@ -82,6 +88,8 @@ var nullable: number | null;
var Obj: any;
var obj: any;
var Func: Function;
var f: (s: string) => number;
var ctor: new (s: string) => { s: string };


//// [a.js]
Expand Down Expand Up @@ -125,6 +133,10 @@ var Obj;
var obj;
/** @type {Function} */
var Func;
/** @type {(s: string) => number} */
var f;
/** @type {new (s: string) => { s: string }} */
var ctor;
//// [b.js]
var S;
var s;
Expand All @@ -146,3 +158,5 @@ var nullable;
var Obj;
var obj;
var Func;
var f;
var ctor;
17 changes: 17 additions & 0 deletions tests/baselines/reference/jsdocTypeTag.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ var obj;
var Func;
>Func : Symbol(Func, Decl(a.js, 58, 3), Decl(b.ts, 19, 3))

/** @type {(s: string) => number} */
var f;
>f : Symbol(f, Decl(a.js, 61, 3), Decl(b.ts, 20, 3))

/** @type {new (s: string) => { s: string }} */
var ctor;
>ctor : Symbol(ctor, Decl(a.js, 64, 3), Decl(b.ts, 21, 3))

=== tests/cases/conformance/jsdoc/b.ts ===
var S: string;
>S : Symbol(S, Decl(a.js, 1, 3), Decl(b.ts, 0, 3))
Expand Down Expand Up @@ -143,3 +151,12 @@ var Func: Function;
>Func : Symbol(Func, Decl(a.js, 58, 3), Decl(b.ts, 19, 3))
>Function : Symbol(Function, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))

var f: (s: string) => number;
>f : Symbol(f, Decl(a.js, 61, 3), Decl(b.ts, 20, 3))
>s : Symbol(s, Decl(b.ts, 20, 8))

var ctor: new (s: string) => { s: string };
>ctor : Symbol(ctor, Decl(a.js, 64, 3), Decl(b.ts, 21, 3))
>s : Symbol(s, Decl(b.ts, 21, 15))
>s : Symbol(s, Decl(b.ts, 21, 30))

17 changes: 17 additions & 0 deletions tests/baselines/reference/jsdocTypeTag.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ var obj;
var Func;
>Func : Function

/** @type {(s: string) => number} */
var f;
>f : (s: string) => number

/** @type {new (s: string) => { s: string }} */
var ctor;
>ctor : new (s: string) => { s: string; }

=== tests/cases/conformance/jsdoc/b.ts ===
var S: string;
>S : string
Expand Down Expand Up @@ -146,3 +154,12 @@ var Func: Function;
>Func : Function
>Function : Function

var f: (s: string) => number;
>f : (s: string) => number
>s : string

var ctor: new (s: string) => { s: string };
>ctor : new (s: string) => { s: string; }
>s : string
>s : string

18 changes: 18 additions & 0 deletions tests/baselines/reference/jsdocTypeTagParameterType.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
tests/cases/conformance/jsdoc/a.js(3,5): error TS2322: Type '1' is not assignable to type 'string'.
tests/cases/conformance/jsdoc/a.js(7,5): error TS2322: Type '1' is not assignable to type 'string'.


==== tests/cases/conformance/jsdoc/a.js (2 errors) ====
/** @type {function(string): void} */
const f = (value) => {
value = 1 // should error
~~~~~
!!! error TS2322: Type '1' is not assignable to type 'string'.
};
/** @type {(s: string) => void} */
function g(s) {
s = 1 // Should error
~
!!! error TS2322: Type '1' is not assignable to type 'string'.
}

19 changes: 19 additions & 0 deletions tests/baselines/reference/jsdocTypeTagParameterType.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/conformance/jsdoc/a.js ===
/** @type {function(string): void} */
const f = (value) => {
>f : Symbol(f, Decl(a.js, 1, 5))
>value : Symbol(value, Decl(a.js, 1, 11))

value = 1 // should error
>value : Symbol(value, Decl(a.js, 1, 11))

};
/** @type {(s: string) => void} */
function g(s) {
>g : Symbol(g, Decl(a.js, 3, 2))
>s : Symbol(s, Decl(a.js, 5, 11))

s = 1 // Should error
>s : Symbol(s, Decl(a.js, 5, 11))
}

24 changes: 24 additions & 0 deletions tests/baselines/reference/jsdocTypeTagParameterType.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
=== tests/cases/conformance/jsdoc/a.js ===
/** @type {function(string): void} */
const f = (value) => {
>f : (arg0: string) => void
>(value) => { value = 1 // should error} : (value: string) => void
>value : string

value = 1 // should error
>value = 1 : 1
>value : string
>1 : 1

};
/** @type {(s: string) => void} */
function g(s) {
>g : (s: string) => void
>s : string

s = 1 // Should error
>s = 1 : 1
>s : string
>1 : 1
}

29 changes: 29 additions & 0 deletions tests/baselines/reference/jsdocTypeTagRequiredParameters.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
tests/cases/conformance/jsdoc/a.js(8,12): error TS7006: Parameter 's' implicitly has an 'any' type.
tests/cases/conformance/jsdoc/a.js(11,1): error TS2554: Expected 1 arguments, but got 0.
tests/cases/conformance/jsdoc/a.js(12,1): error TS2554: Expected 1 arguments, but got 0.
tests/cases/conformance/jsdoc/a.js(13,1): error TS2554: Expected 1 arguments, but got 0.


==== tests/cases/conformance/jsdoc/a.js (4 errors) ====
/** @type {function(string): void} */
const f = (value) => {
};
/** @type {(s: string) => void} */
function g(s) {
}
/** @type {{(s: string): void}} */
function h(s) {
~
!!! error TS7006: Parameter 's' implicitly has an 'any' type.
}

f() // should error
~~~
!!! error TS2554: Expected 1 arguments, but got 0.
g() // should error
~~~
!!! error TS2554: Expected 1 arguments, but got 0.
h()
~~~
!!! error TS2554: Expected 1 arguments, but got 0.

27 changes: 27 additions & 0 deletions tests/baselines/reference/jsdocTypeTagRequiredParameters.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
=== tests/cases/conformance/jsdoc/a.js ===
/** @type {function(string): void} */
const f = (value) => {
>f : Symbol(f, Decl(a.js, 1, 5))
>value : Symbol(value, Decl(a.js, 1, 11))

};
/** @type {(s: string) => void} */
function g(s) {
>g : Symbol(g, Decl(a.js, 2, 2))
>s : Symbol(s, Decl(a.js, 4, 11))
}
/** @type {{(s: string): void}} */
function h(s) {
>h : Symbol(h, Decl(a.js, 5, 1))
>s : Symbol(s, Decl(a.js, 7, 11))
}

f() // should error
>f : Symbol(f, Decl(a.js, 1, 5))

g() // should error
>g : Symbol(g, Decl(a.js, 2, 2))

h()
>h : Symbol(h, Decl(a.js, 5, 1))

31 changes: 31 additions & 0 deletions tests/baselines/reference/jsdocTypeTagRequiredParameters.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
=== tests/cases/conformance/jsdoc/a.js ===
/** @type {function(string): void} */
const f = (value) => {
>f : (arg0: string) => void
>(value) => {} : (value: string) => void
>value : string

};
/** @type {(s: string) => void} */
function g(s) {
>g : (s: string) => void
>s : string
}
/** @type {{(s: string): void}} */
function h(s) {
>h : (s: any) => void
>s : any
}

f() // should error
>f() : void
>f : (arg0: string) => void

g() // should error
>g() : void
>g : (s: string) => void

h()
>h() : void
>h : (s: any) => void

Loading