Skip to content

Parameters infer from body and call sites#28342

Merged
sandersn merged 4 commits intomasterfrom
infer-from-usage/parameters-infer-from-body-and-call-sites
Nov 5, 2018
Merged

Parameters infer from body and call sites#28342
sandersn merged 4 commits intomasterfrom
infer-from-usage/parameters-infer-from-body-and-call-sites

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Nov 5, 2018

  1. Function expressions check their parent for an applicable name. Currently the only applicable name is a a declaration name that must be an identifier. This is a small change that I will follow up with a comprehensive change to support JS-style assignment declarations.
  2. Parameters use inferences both from usage in their function body and from call sites. Previously inferences from call sites would be the only ones used if any were present, and the algorithm would fall back to body-usage inferences. Now they are combined upfront and then unified.

The latter change decreases coverage about 0.5% (the number of previously-any sites that are now annotated), but decreases the introduced error rate by more than 5%.

@sandersn sandersn requested review from a user and PranavSenthilnathan November 5, 2018 17:37
return InferFromReference.inferTypeFromReferences(getReferences(token, program, cancellationToken), program.getTypeChecker(), cancellationToken);
const references = getReferences(token, program, cancellationToken);
const checker = program.getTypeChecker();
const types = InferFromReference.inferTypesFromReferences(references, checker, cancellationToken);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like we could combine inferTypesFromReferences and unifyFromContext to avoid exposing two different functions?

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.

Well, the two are decoupled in inferTypeForParametersFromReferences; I decided to do the same here instead of keeping a function named inferTypeFromReferences around, which would just be the compose of inferTypesFromRefences >>> unifyFromContext.

If the pair were called together more than once I think a function like that would make sense. Right now it doesn't.

let searchToken;
switch (containingFunction.kind) {
case SyntaxKind.Constructor:
searchToken = findChildOfKind<Token<SyntaxKind.ConstructorKeyword>>(containingFunction, SyntaxKind.ConstructorKeyword, sourceFile);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we have a constructor test?

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.

codeFixInferFromUsageMember3

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.

In other words, coverage is not great. I'm going to improve typing of JS constructor functions next, so I'll plan to add equivalent TS tests for each new JS test.

@sandersn sandersn merged commit 4cb210c into master Nov 5, 2018
@sandersn sandersn deleted the infer-from-usage/parameters-infer-from-body-and-call-sites branch November 5, 2018 19:29
@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