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

Fix type when annotated with a JSDoc function type #22692

Merged
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 5 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
99 changes: 58 additions & 41 deletions 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
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
Member Author

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

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
Member Author

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
Member

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
Member Author

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
Member

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 Expand Up @@ -9123,7 +9130,8 @@ namespace ts {
}

function isContextSensitiveFunctionOrObjectLiteralMethod(func: Node): func is FunctionExpression | ArrowFunction | MethodDeclaration {
return (isFunctionExpressionOrArrowFunction(func) || isObjectLiteralMethod(func)) && isContextSensitiveFunctionLikeDeclaration(func);
return (isInJavaScriptFile(func) && isFunctionDeclaration(func) || isFunctionExpressionOrArrowFunction(func) || isObjectLiteralMethod(func)) &&
isContextSensitiveFunctionLikeDeclaration(func);
}

function getTypeWithoutSignatures(type: Type): Type {
Expand Down Expand Up @@ -14191,49 +14199,49 @@ namespace ts {
// Return contextual type of parameter or undefined if no contextual type is available
function getContextuallyTypedParameterType(parameter: ParameterDeclaration): Type | undefined {
const func = parameter.parent;
if (isContextSensitiveFunctionOrObjectLiteralMethod(func)) {
const iife = getImmediatelyInvokedFunctionExpression(func);
if (iife && iife.arguments) {
const indexOfParameter = func.parameters.indexOf(parameter);
if (parameter.dotDotDotToken) {
const restTypes: Type[] = [];
for (let i = indexOfParameter; i < iife.arguments.length; i++) {
restTypes.push(getWidenedLiteralType(checkExpression(iife.arguments[i])));
}
return restTypes.length ? createArrayType(getUnionType(restTypes)) : undefined;
}
const links = getNodeLinks(iife);
const cached = links.resolvedSignature;
links.resolvedSignature = anySignature;
const type = indexOfParameter < iife.arguments.length ?
getWidenedLiteralType(checkExpression(iife.arguments[indexOfParameter])) :
parameter.initializer ? undefined : undefinedWideningType;
links.resolvedSignature = cached;
return type;
if (!isContextSensitiveFunctionOrObjectLiteralMethod(func)) {
return undefined;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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

const iife = getImmediatelyInvokedFunctionExpression(func);
if (iife && iife.arguments) {
const indexOfParameter = func.parameters.indexOf(parameter);
if (parameter.dotDotDotToken) {
const restTypes: Type[] = [];
for (let i = indexOfParameter; i < iife.arguments.length; i++) {
restTypes.push(getWidenedLiteralType(checkExpression(iife.arguments[i])));
}
return restTypes.length ? createArrayType(getUnionType(restTypes)) : undefined;
}
const links = getNodeLinks(iife);
const cached = links.resolvedSignature;
links.resolvedSignature = anySignature;
const type = indexOfParameter < iife.arguments.length ?
getWidenedLiteralType(checkExpression(iife.arguments[indexOfParameter])) :
parameter.initializer ? undefined : undefinedWideningType;
links.resolvedSignature = cached;
return type;
}
const contextualSignature = getContextualSignature(func);
if (contextualSignature) {
const funcHasRestParameters = hasRestParameter(func);
const len = func.parameters.length - (funcHasRestParameters ? 1 : 0);
let indexOfParameter = func.parameters.indexOf(parameter);
if (getThisParameter(func) !== undefined && !contextualSignature.thisParameter) {
Debug.assert(indexOfParameter !== 0); // Otherwise we should not have called `getContextuallyTypedParameterType`.
indexOfParameter -= 1;
}
const contextualSignature = getContextualSignature(func);
if (contextualSignature) {
const funcHasRestParameters = hasRestParameter(func);
const len = func.parameters.length - (funcHasRestParameters ? 1 : 0);
let indexOfParameter = func.parameters.indexOf(parameter);
if (getThisParameter(func) !== undefined && !contextualSignature.thisParameter) {
Debug.assert(indexOfParameter !== 0); // Otherwise we should not have called `getContextuallyTypedParameterType`.
indexOfParameter -= 1;
}

if (indexOfParameter < len) {
return getTypeAtPosition(contextualSignature, indexOfParameter);
}
if (indexOfParameter < len) {
return getTypeAtPosition(contextualSignature, indexOfParameter);
}

// If last parameter is contextually rest parameter get its type
if (funcHasRestParameters &&
indexOfParameter === (func.parameters.length - 1) &&
isRestParameterIndex(contextualSignature, func.parameters.length - 1)) {
return getTypeOfSymbol(lastOrUndefined(contextualSignature.parameters));
}
// If last parameter is contextually rest parameter get its type
if (funcHasRestParameters &&
indexOfParameter === (func.parameters.length - 1) &&
isRestParameterIndex(contextualSignature, func.parameters.length - 1)) {
return getTypeOfSymbol(lastOrUndefined(contextualSignature.parameters));
}
}
return undefined;
}

// In a variable, parameter or property declaration with a type annotation,
Expand Down Expand Up @@ -14796,7 +14804,16 @@ namespace ts {
// union type of return types from these signatures
function getContextualSignature(node: FunctionExpression | ArrowFunction | MethodDeclaration): Signature {
Debug.assert(node.kind !== SyntaxKind.MethodDeclaration || isObjectLiteralMethod(node));
const type = getContextualTypeForFunctionLikeDeclaration(node);
let type: Type;
if (isInJavaScriptFile(node)) {
const jsdoc = getJSDocType(node);
if (jsdoc) {
type = getTypeFromTypeNode(jsdoc);
}
}
if (!type) {
type = getContextualTypeForFunctionLikeDeclaration(node);
}
if (!type) {
return undefined;
}
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
Member

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
Member Author

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
Member

Choose a reason for hiding this comment

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

Hm. OK.

Copy link
Member Author

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

Loading