-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Changes from 5 commits
a602e1f
99b5362
adfd904
4d8a8b3
d14c511
db39b43
6095dfd
5bc6895
9bec8c1
9905af7
a5d6542
7672747
351f665
ef5f7bc
15b6c0c
d85a212
cd788ee
f2853ae
aff19fc
818b9c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4298,7 +4298,7 @@ namespace ts { | |
type = getContextualThisParameterType(func); | ||
} | ||
else { | ||
type = getContextuallyTypedParameterType(<ParameterDeclaration>declaration); | ||
type = getContextuallyTypedParameterType(<ParameterDeclaration>declaration) | ||
} | ||
if (type) { | ||
return addOptionality(type, /*optional*/ declaration.questionToken && includeOptionality); | ||
|
@@ -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; | ||
} | ||
|
@@ -12798,6 +12804,20 @@ namespace ts { | |
return undefined; | ||
} | ||
|
||
function getParameterTypeFromBody(parameter: ParameterDeclaration): Type { | ||
const func = <FunctionLikeDeclaration>parameter.parent | ||
if (!func.body) { | ||
return unknownType; | ||
} | ||
|
||
let type: Type; | ||
let types: Type[]; | ||
types = checkAndAggregateParameterExpressionTypes(parameter) | ||
type = types ? getWidenedType(getIntersectionType(types)) : undefined; | ||
|
||
return type; | ||
} | ||
|
||
// Return contextual type of parameter or undefined if no contextual type is available | ||
function getContextuallyTypedParameterType(parameter: ParameterDeclaration): Type { | ||
const func = parameter.parent; | ||
|
@@ -16729,6 +16749,24 @@ namespace ts { | |
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clearly, only collecting invocation isn't enough. But I wonder how other expressions can be handled. For example, how There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HerringtonDarkholme I've put out some initial thoughts in #15114 (comment). We can exploit co and contravariance to get sensible rules here. If something assigns to a parameter, the sensible thing is for the parameter to be inferred as having a supertype of said assignment. Unfortunately TypeScript can't represent this. This means we should restrict ourselves to inference in covariant usages of the parameter variable, i.e. wherever it is used as a source of information (assignment of the parameter to well-typed reference, invocation of the parameter with well-typed argument list, accessing properties on parameter, etc.) |
||
} | ||
|
||
function checkAndAggregateParameterExpressionTypes(parameter: ParameterDeclaration): Type[] { | ||
const func = <FunctionLikeDeclaration>parameter.parent | ||
const usageTypes: Type[] = [] | ||
forEachInvocation(<Block>func.body, invocation => { | ||
const usages = invocation.arguments | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
.map((arg, i) => ({ arg, symbol: getSymbolAtLocation(arg), i })) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
.filter(({ symbol }) => symbol && symbol.valueDeclaration === parameter) | ||
if (!usages.length) return | ||
const funcSymbol = getSymbolAtLocation(invocation.expression) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to find robust mechanism for resolving the node corresponding to the invoked function to a function signature. Right now things like |
||
if (!funcSymbol) return | ||
const sig = getSignatureFromDeclaration(funcSymbol.valueDeclaration as FunctionLikeDeclaration) | ||
const parameterTypes = sig.parameters.map(getTypeOfParameter) | ||
const argumentTypes = usages.map(({ i }) => parameterTypes[i]) | ||
usageTypes.splice(0, 0, ...argumentTypes); | ||
}); | ||
return usageTypes.length ? usageTypes : undefined; | ||
} | ||
|
||
function checkAndAggregateReturnExpressionTypes(func: FunctionLikeDeclaration, checkMode: CheckMode): Type[] { | ||
const functionFlags = getFunctionFlags(func); | ||
const aggregatedTypes: Type[] = []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// CASE 1 | ||
function foo(s) { | ||
Math.sqrt(s) | ||
} | ||
|
||
// CASE 2 | ||
declare function swapNumberString(n: string): number; | ||
declare function swapNumberString(n: number): string; | ||
|
||
// Should have identical signature to swapNumberString | ||
function subs(s) { | ||
return swapNumberString(s); | ||
} | ||
|
||
// CASE 3 | ||
function f(x: number){ | ||
return x; | ||
} | ||
|
||
function g(x){ return x}; | ||
|
||
function h(x){ return f(x); }; |
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 asnumber | string
instead ofnumber & string
.number & string
is an excessively conservative inference, but it is a sound one:number & string
is assignable tonumber | 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 realizenumber & string
is unsatisfiable, or to actually analyze the flow graph and generate the appropriate union type.