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

Unify contextual signature type parameter assignment #31574

Conversation

weswigham
Copy link
Member

Fixes #28445

In getTypeForVariableLikeDeclaration (which was eventually called into by getTypeOfSymbol) we looked at the contextual signature for an unannotated parameter's type, but failed to look at the inference context's mapper, so we'd have uninstantiated context variables saved into the symbol type cache which'd pollute future results. With this, we instantiate and fix the same as we do via the checkFunctionExpressionOrObjectLiteralMethod entrypoint to parameter type calculation.

This exposed a small bug where we didn't consider function declarations with jsdoc tags as potentially context sensitive in JS, which is also fixed.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Is it possible to early-detect this situation and use the normal code path instead of hacking the current code path to behave the same?

Probably not, but I can't quite remember why not. Is it that TONS of special-case code would now have to be called from the normal code path?

@@ -18172,7 +18176,7 @@ namespace ts {
}

// Return contextual type of parameter or undefined if no contextual type is available
function getContextuallyTypedParameterType(parameter: ParameterDeclaration): Type | undefined {
function getContextuallyTypedParameterType(parameter: ParameterDeclaration, forCache?: boolean): Type | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

this function is only called twice so I would make forCache required.

monomorphism!

@@ -11484,6 +11484,7 @@ namespace ts {
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.FunctionDeclaration: // Function declarations can have context when annotated with a jsdoc @type
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find a test with an example, and couldn't come up with one myself. I didn't think @type was treated as a contextual type.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a trio of jsdoc tests that demonstrate it which were borked with the better handling of contextual mappers and context checking (ie, they abused how the different entrypoint cached a different type to force a type to be cached from the jsdoc annotation) without proper handling here. The responsible codepath is definitely doing an unsafe cast, technically, since this function isn't even declared to accept a FunctionDeclaration, however checking it is perfectly reasonable when an annotated declaration is viewed in the same light as an annotated assigned expression.

Copy link
Member

Choose a reason for hiding this comment

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

can you point me to the tests?

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@weswigham
Copy link
Member Author

weswigham commented May 24, 2019

Is it possible to early-detect this situation and use the normal code path instead of hacking the current code path to behave the same?

No - in fact, LS requests will almost always come in via the getTypeOfSymbol route first, but if quickinfo for the containing object is asked for the other might be first. The context checked pattern forces us to take a single result now (that respects any contextual mappers applied), which is at least better. Tbh, there being a similar codepath between some of the respective checkX methods and getTypeOfVariableParameterOrProperty that can produce types for the same symbol for is probably always an error - but iirc contextual parameter assignment is literally the one place we assign to a symbol's type outside of symbol-based type lookup caching. The symbol lookup is "designed" to be both highly polymorphic on symbol declaration and to have an output for every conceivable declaration kind, while the other is meant to simply handle a top-down walk of a node tree, and so can make many simplifying assumptions...

@weswigham weswigham force-pushed the unify-contextual-signature-type-assignment branch from 24b37e7 to 7b16dd9 Compare May 31, 2019 23:01
@weswigham weswigham requested a review from sandersn May 31, 2019 23:02
@weswigham weswigham merged commit 38da682 into microsoft:master Jun 5, 2019
@weswigham weswigham deleted the unify-contextual-signature-type-assignment branch June 20, 2019 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression bug - false typescript error with regards to generics and generics inference
2 participants