-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Unify contextual signature type parameter assignment #31574
Conversation
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.
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?
src/compiler/checker.ts
Outdated
@@ -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 { |
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.
this function is only called twice so I would make forCache
required.
monomorphism!
src/compiler/checker.ts
Outdated
@@ -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 |
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.
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.
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.
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.
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.
can you point me to the tests?
No - in fact, LS requests will almost always come in via the |
24b37e7
to
7b16dd9
Compare
Fixes #28445
In
getTypeForVariableLikeDeclaration
(which was eventually called into bygetTypeOfSymbol
) 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 thecheckFunctionExpressionOrObjectLiteralMethod
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.