Skip to content

In JS, constructor functions infer from call+construct#28353

Merged
sandersn merged 2 commits into
masterfrom
infer-from-usage/constructor-functions
Nov 16, 2018
Merged

In JS, constructor functions infer from call+construct#28353
sandersn merged 2 commits into
masterfrom
infer-from-usage/constructor-functions

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Nov 6, 2018

In JS, the inference code fix now infers from both calls and constructs of a function. I thought about only doing this for functions that the compiler thinks are constructor functions, but I'd rather err on the side of permissiveness for the codefix.

Also fix an incorrect combining of inferences for rest parameters: the inferred types will be arrays in the body of the function and the arguments from outside the function will be the element type.

Also fix an incorrect combining of inferences for rest parameters: the
inferred types will be arrays in the body of the function and the
arguments from outside the function will be the element type.
@sandersn sandersn requested review from a user and PranavSenthilnathan November 6, 2018 00:08
@@ -414,27 +414,33 @@ namespace ts.codefix {
inferTypeFromContext(reference, checker, usageContext);
}
const isConstructor = declaration.kind === SyntaxKind.Constructor;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is only used in one place, you could just inline to isConstructorDeclaration(declaration)

return callContexts && declaration.parameters.map((parameter, parameterIndex): ParameterInference => {
const types: Type[] = [];
const isJS = isInJSFile(declaration);
const callContexts = isJS ? [...usageContext.constructContexts || [], ...usageContext.callContexts || []] :
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could just combine construct and call unconditionally?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Welllll.....yeah, why not?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Doesn't make a difference when run on the user test suite, and it simplifies the code considerably, so I think it's a good change.

if (callContexts) {
for (const callContext of callContexts) {
if (callContext.argumentTypes.length <= parameterIndex) {
isOptional = isJS;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly, does this need to depend on whether we're in JS?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought about this one, but it's a bigger change -- right now the codefix only adds annotations, it doesn't add other punctuation around parameters. I'll open a bug to track this, though, because you are right, it should work (and it's probably not too hard).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ref: #28377

////class TokenType {
//// label;
//// token;
//// constructor([|label, token? |]) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the user forgot type annotations they would probably forget ? -- I think this should work without it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Moved to #28377

@sandersn sandersn merged commit ea8ccc2 into master Nov 16, 2018
@sandersn sandersn deleted the infer-from-usage/constructor-functions branch November 16, 2018 17:51
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant