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.

Sign up for GitHub

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

Fixed a crash in completions on functionlike types #56257

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Fixed a crash in completions on functionlike types #56257

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c4fae6
Fixed a crash in completions on functionlike types
Andarist Oct 30, 2023
0009f7f
Tighten checks even more
Andarist Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1868,7 +1868,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const nodeLinks = getNodeLinks(node);
cachedResolvedSignatures.push([nodeLinks, nodeLinks.resolvedSignature] as const);
nodeLinks.resolvedSignature = undefined;
if (isFunctionLike(node)) {
if (isFunctionLikeDeclaration(node)) {
const symbolLinks = getSymbolLinks(getSymbolOfDeclaration(node));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm wrong here, but is it the case that this could also be fixed by instead checking if the declaration has a symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm wrong here

You are not 😉

but is it the case that this could also be fixed by instead checking if the declaration has a symbol?

It could have been fixed like that but the possibility of lacking a symbol isn't reflected by the types. I also just noticed that this kinda happened because I was using a function that was matching more types than I wanted so this just seemed like a slightly better fix.

Copy link
Member

Choose a reason for hiding this comment

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

the possibility of lacking a symbol isn't reflected by the types

This is an ongoing struggle; I should really resurrect my PR which attempts to make it known-optional.

const type = symbolLinks.type;
cachedTypes.push([symbolLinks, type] as const);
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/utilitiesPublic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1928,8 +1928,8 @@ export function isPropertyAccessOrQualifiedName(node: Node): node is PropertyAcc
}

/** @internal */
export function isCallLikeOrFunctionLikeExpression(node: Node): node is CallLikeExpression | SignatureDeclaration {
Copy link
Member

Choose a reason for hiding this comment

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

Was this function really not used elsewhere? Super surprised, but it does seem to be true.

return isCallLikeExpression(node) || isFunctionLike(node);
export function isCallLikeOrFunctionLikeExpression(node: Node): node is CallLikeExpression | FunctionLikeDeclaration {
return isCallLikeExpression(node) || isFunctionLikeDeclaration(node);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that this reflects the name better and perhaps the change reflects the original intention

Copy link
Member

Choose a reason for hiding this comment

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

I guess I don't expect this to return true for function declarations, except those in expression positions... am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yee, you are right. I limited this further to only return true for function expressions and arrows

Copy link
Member

Choose a reason for hiding this comment

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

Are you trying to limit to functions that can be contextually typed? What about object literal method declarations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you trying to limit to functions that can be contextually typed?

Somewhat. IIRC I meant to limit it to expression-space functions and both of those "sets" are almost the same. I think that either would be OK here (at least for the purpose of my fix).

What about object literal method declarations?

They are already covered by isFunctionLikeDeclaration check


export function isCallLikeExpression(node: Node): node is CallLikeExpression {
Expand Down
13 changes: 13 additions & 0 deletions tests/cases/fourslash/jsDocFunctionTypeCompletionsNoCrash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
///<reference path="fourslash.ts" />

//// /**
//// * @returns {function/**/(): string}
//// */
//// function updateCalendarEvent() {
//// return "";
//// }

verify.completions({
marker: "",
exact: completion.globalTypes
});
Loading