-
Notifications
You must be signed in to change notification settings - Fork 12.9k
enable go-to-type-definition on type nodes #46714
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
Conversation
src/services/goToDefinition.ts
Outdated
@@ -205,7 +205,21 @@ namespace ts.GoToDefinition { | |||
const returnType = tryGetReturnTypeOfFunction(symbol, typeAtLocation, typeChecker); | |||
const fromReturnType = returnType && definitionFromType(returnType, typeChecker, node); | |||
// If a function returns 'void' or some other type with no definition, just return the function definition. | |||
return fromReturnType && fromReturnType.length !== 0 ? fromReturnType : definitionFromType(typeAtLocation, typeChecker, node); | |||
const typeDefinitions = fromReturnType && fromReturnType.length !== 0 ? fromReturnType : definitionFromType(typeAtLocation, typeChecker, node); | |||
return typeDefinitions.length |
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 don’t think this is quite right. Here’s a test case to consider:
let Foo: unresolved;
type Foo = { x: string };
Foo/*go to type def*/;
Remember that “go to type definition” usually means “get the type of this value and go to the definition of that type.” It does not mean “go to the definition of this symbol that is a type.” The proposal here is to extend the meaning of the command only where the symbol in question has no value meaning, so “get the type of this value” is an invalid starting point. If you replace unresolved
with { foo: any }
and run go-to-type-definition on Foo
, you end up on { foo: any }
. However, with unresolved
being unresolved, we can’t figure out the type of the value Foo
and therefore we don’t return any results. This is the correct behavior, but I think this PR would break that and return a location for type Foo
. I want to make sure we aren’t blurring the lines any more than necessary, so I think this test case should continue to return no results. (In the future, I think returning the location of unresolved
itself would be better, but that’s out of scope for this PR.)
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.
Looks good. Just one suggestion to improve the formatting.
Co-authored-by: Nathan Shively-Sanders <[email protected]>
* enable go-to-type-definition on type nodes * only go when symbol has no value meaning * Update formatting of src/services/goToDefinition.ts Co-authored-by: Nathan Shively-Sanders <[email protected]> Co-authored-by: Nathan Shively-Sanders <[email protected]>
Fixes #46627