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

Discussion: Parameter type inference from function body #17715

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 44 additions & 0 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4327,6 +4327,12 @@ namespace ts {
return getTypeFromBindingPattern(<BindingPattern>declaration.name, /*includePatternInType*/ false, /*reportErrors*/ true);
}

// Important to do this *after* attempt has been made to resolve via initializer
if (declaration.kind === SyntaxKind.Parameter) {
const inferredType = getParameterTypeFromBody(<ParameterDeclaration>declaration);
if (inferredType) return inferredType;
}

// No type specified and nothing can be inferred
return undefined;
}
Expand Down Expand Up @@ -12798,6 +12804,14 @@ namespace ts {
return undefined;
}

function getParameterTypeFromBody(parameter: ParameterDeclaration): Type {
const func = <FunctionLikeDeclaration>parameter.parent;
if (!func.body || isRestParameter(parameter)) return;

const types = checkAndAggregateParameterExpressionTypes(parameter);
return types ? getWidenedType(getIntersectionType(types)) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

getIntersectionType cannot handle flow sensitive typing. For example,

function test(a) {
  if (someCond) roundNumber(a)
  else trimString(a)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HerringtonDarkholme Could you provide a concrete example of this? For which reference is someCond affecting the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HerringtonDarkholme I understand what you mean now; in the case you highlighted a should be inferred as number | string instead of number & string. number & string is an excessively conservative inference, but it is a sound one: number & string is assignable to number | string.

Some options are to bail and infer any for parameters used in branched code, keep the existing behavior and expect the user to manually supply typings when they realize number & string is unsatisfiable, or to actually analyze the flow graph and generate the appropriate union type.

}

// Return contextual type of parameter or undefined if no contextual type is available
function getContextuallyTypedParameterType(parameter: ParameterDeclaration): Type {
const func = parameter.parent;
Expand Down Expand Up @@ -16729,6 +16743,36 @@ namespace ts {
return true;
}

function checkAndAggregateParameterExpressionTypes(parameter: ParameterDeclaration): Type[] {
const func = <FunctionLikeDeclaration>parameter.parent;
const usageTypes: Type[] = [];
const invocations: CallExpression[] = [];

function seekInvocations(f: FunctionBody) {
forEachInvocation(f, invocation => {
invocations.push(invocation);
invocation.arguments.filter(isFunctionExpressionOrArrowFunction).forEach(arg => seekInvocations(<Block>arg.body));
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work with recursive function? I guess it need some guard statement.

Copy link
Contributor Author

@masaeedu masaeedu Aug 10, 2017

Choose a reason for hiding this comment

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

getTypeOfParameter calls getTypeOfSymbol, which internally has a stack checking for circularity and ejecting with "unknown". I believe we can rely on this, but I need to add tests.


Copy link
Contributor

Choose a reason for hiding this comment

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

What if the parameterType is a generic type parameter? Should it be propagated to the inferred function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably it should be ensured that the function currently under inference does not already have an explicit type parameter list, and a type parameter to add polymorphism wrt the parameter under inference. If there is already a type parameter list we should just bail.

seekInvocations(<Block>func.body);

invocations.forEach(invocation => {
const usages = invocation.arguments
.map((arg, i) => ({ arg, symbol: getSymbolAtLocation(arg), i }))
.filter(({ symbol }) => symbol && symbol.valueDeclaration === parameter);
if (!usages.length) return;
const funcSymbol = getSymbolAtLocation(invocation.expression);
if (!funcSymbol || !isFunctionLike(funcSymbol.valueDeclaration)) return;
const sig = getSignatureFromDeclaration(funcSymbol.valueDeclaration);
const parameterTypes = sig.parameters.map(getTypeOfParameter);
const argumentTypes = usages.map(({ i }) => parameterTypes[i]).filter(t => !!t);
usageTypes.splice(0, 0, ...argumentTypes);
});

return usageTypes.length ? usageTypes : undefined;
}

function checkAndAggregateReturnExpressionTypes(func: FunctionLikeDeclaration, checkMode: CheckMode): Type[] {
const functionFlags = getFunctionFlags(func);
const aggregatedTypes: Type[] = [];
Expand Down
14 changes: 14 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,20 @@ namespace ts {
return false;
}

export function forEachInvocation<T>(body: Block, visitor: (stmt: CallExpression) => T): T {

return traverse(body);

function traverse(node: Node): T {
switch (node.kind) {
case SyntaxKind.CallExpression:
return visitor(<CallExpression>node);
default:
return forEachChild(node, traverse);
}
}
}

// Warning: This has the same semantics as the forEach family of functions,
// in that traversal terminates in the event that 'visitor' supplies a truthy value.
export function forEachReturnStatement<T>(body: Block, visitor: (stmt: ReturnStatement) => T): T {
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/controlFlowPropertyDeclarations.types
Original file line number Diff line number Diff line change
Expand Up @@ -196,16 +196,16 @@ function hyphenToCamelCase(string) {
* Determines if the specified string consists entirely of whitespace.
*/
function isEmpty(string) {
>isEmpty : (string: any) => boolean
>string : any
>isEmpty : (string: string) => boolean
>string : string

return !/[^\s]/.test(string);
>!/[^\s]/.test(string) : boolean
>/[^\s]/.test(string) : boolean
>/[^\s]/.test : (string: string) => boolean
>/[^\s]/ : RegExp
>test : (string: string) => boolean
>string : any
>string : string
}

/**
Expand All @@ -216,15 +216,15 @@ function isEmpty(string) {
* @return {boolean}
*/
function isConvertiblePixelValue(value) {
>isConvertiblePixelValue : (value: any) => boolean
>value : any
>isConvertiblePixelValue : (value: string) => boolean
>value : string

return /^\d+px$/.test(value);
>/^\d+px$/.test(value) : boolean
>/^\d+px$/.test : (string: string) => boolean
>/^\d+px$/ : RegExp
>test : (string: string) => boolean
>value : any
>value : string
}

export class HTMLtoJSX {
Expand Down
57 changes: 57 additions & 0 deletions tests/baselines/reference/parameterInference.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
//// [parameterInference.ts]
// CASE 1
function foo(s) {
Math.sqrt(s)
}
// => function foo(s: number): void

// CASE 2
declare function swapNumberString(n: string): number;
declare function swapNumberString(n: number): string;

function subs(s) {
return swapNumberString(s);
}
// => function subs(s: string): number
// NOTE: Still broken, needs to deal with overloads. Should have been inferred as:
// => (s: string) => number & (s: number) => string

// CASE 3
function f3(x: number){
return x;
}

function g3(x){ return f3(x); };
// => function g3(x: number): number

// CASE 4
declare function f4(g: Function)
function g4(x) {
f4(() => {
Math.sqrt(x)
})
}


//// [parameterInference.js]
// CASE 1
function foo(s) {
Math.sqrt(s);
}
function subs(s) {
return swapNumberString(s);
}
// => function subs(s: string): number
// NOTE: Still broken, needs to deal with overloads. Should have been inferred as:
// => (s: string) => number & (s: number) => string
// CASE 3
function f3(x) {
return x;
}
function g3(x) { return f3(x); }
;
function g4(x) {
f4(function () {
Math.sqrt(x);
});
}
74 changes: 74 additions & 0 deletions tests/baselines/reference/parameterInference.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
=== tests/cases/compiler/parameterInference.ts ===
// CASE 1
function foo(s) {
>foo : Symbol(foo, Decl(parameterInference.ts, 0, 0))
>s : Symbol(s, Decl(parameterInference.ts, 1, 13))

Math.sqrt(s)
>Math.sqrt : Symbol(Math.sqrt, Decl(lib.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))
>sqrt : Symbol(Math.sqrt, Decl(lib.d.ts, --, --))
>s : Symbol(s, Decl(parameterInference.ts, 1, 13))
}
// => function foo(s: number): void

// CASE 2
declare function swapNumberString(n: string): number;
>swapNumberString : Symbol(swapNumberString, Decl(parameterInference.ts, 3, 1), Decl(parameterInference.ts, 7, 53))
>n : Symbol(n, Decl(parameterInference.ts, 7, 34))

declare function swapNumberString(n: number): string;
>swapNumberString : Symbol(swapNumberString, Decl(parameterInference.ts, 3, 1), Decl(parameterInference.ts, 7, 53))
>n : Symbol(n, Decl(parameterInference.ts, 8, 34))

function subs(s) {
>subs : Symbol(subs, Decl(parameterInference.ts, 8, 53))
>s : Symbol(s, Decl(parameterInference.ts, 10, 14))

return swapNumberString(s);
>swapNumberString : Symbol(swapNumberString, Decl(parameterInference.ts, 3, 1), Decl(parameterInference.ts, 7, 53))
>s : Symbol(s, Decl(parameterInference.ts, 10, 14))
}
// => function subs(s: string): number
// NOTE: Still broken, needs to deal with overloads. Should have been inferred as:
// => (s: string) => number & (s: number) => string

// CASE 3
function f3(x: number){
>f3 : Symbol(f3, Decl(parameterInference.ts, 12, 1))
>x : Symbol(x, Decl(parameterInference.ts, 18, 12))

return x;
>x : Symbol(x, Decl(parameterInference.ts, 18, 12))
}

function g3(x){ return f3(x); };
>g3 : Symbol(g3, Decl(parameterInference.ts, 20, 1))
>x : Symbol(x, Decl(parameterInference.ts, 22, 12))
>f3 : Symbol(f3, Decl(parameterInference.ts, 12, 1))
>x : Symbol(x, Decl(parameterInference.ts, 22, 12))

// => function g3(x: number): number

// CASE 4
declare function f4(g: Function)
>f4 : Symbol(f4, Decl(parameterInference.ts, 22, 32))
>g : Symbol(g, Decl(parameterInference.ts, 26, 20))
>Function : Symbol(Function, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))

function g4(x) {
>g4 : Symbol(g4, Decl(parameterInference.ts, 26, 32))
>x : Symbol(x, Decl(parameterInference.ts, 27, 12))

f4(() => {
>f4 : Symbol(f4, Decl(parameterInference.ts, 22, 32))

Math.sqrt(x)
>Math.sqrt : Symbol(Math.sqrt, Decl(lib.d.ts, --, --))
>Math : Symbol(Math, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))
>sqrt : Symbol(Math.sqrt, Decl(lib.d.ts, --, --))
>x : Symbol(x, Decl(parameterInference.ts, 27, 12))

})
}

80 changes: 80 additions & 0 deletions tests/baselines/reference/parameterInference.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
=== tests/cases/compiler/parameterInference.ts ===
// CASE 1
function foo(s) {
>foo : (s: number) => void
>s : number

Math.sqrt(s)
>Math.sqrt(s) : number
>Math.sqrt : (x: number) => number
>Math : Math
>sqrt : (x: number) => number
>s : number
}
// => function foo(s: number): void

// CASE 2
declare function swapNumberString(n: string): number;
>swapNumberString : { (n: string): number; (n: number): string; }
>n : string

declare function swapNumberString(n: number): string;
>swapNumberString : { (n: string): number; (n: number): string; }
>n : number

function subs(s) {
>subs : (s: string) => number
>s : string

return swapNumberString(s);
>swapNumberString(s) : number
>swapNumberString : { (n: string): number; (n: number): string; }
>s : string
}
// => function subs(s: string): number
// NOTE: Still broken, needs to deal with overloads. Should have been inferred as:
// => (s: string) => number & (s: number) => string

// CASE 3
function f3(x: number){
>f3 : (x: number) => number
>x : number

return x;
>x : number
}

function g3(x){ return f3(x); };
>g3 : (x: number) => number
>x : number
>f3(x) : number
>f3 : (x: number) => number
>x : number

// => function g3(x: number): number

// CASE 4
declare function f4(g: Function)
>f4 : (g: Function) => any
>g : Function
>Function : Function

function g4(x) {
>g4 : (x: number) => void
>x : number

f4(() => {
>f4(() => { Math.sqrt(x) }) : any
>f4 : (g: Function) => any
>() => { Math.sqrt(x) } : () => void

Math.sqrt(x)
>Math.sqrt(x) : number
>Math.sqrt : (x: number) => number
>Math : Math
>sqrt : (x: number) => number
>x : number

})
}

Loading