Go to definition on callable property returns two definitions

[mjbvz]

Bug Report

🔎 Search Terms

• go to definition
• callable

🕗 Version & Regression Information

4.3.0-dev.20210316

⏯ Playground Link

Playground link with relevant code

💻 Code

interface ICallable {
    (): any
}
class K {
    callable: ICallable;
}

new K().callable()

Run go to definition on callable

🙁 Actual behavior

Two definitions are returned, for for K.callable and one for (): any in ICallable

🙂 Expected behavior

Just the definition for K.callable is returned

[RyanCavanaugh]

This seems like the right behavior for a method invocation? We don't know if you mean "show me the property" or "show me the overload that was selected"

[mjbvz]

When I think about this more I can sort ofunderstand the current behavior but it still catches me off guard.

The main place I see this in the VS Code code base is for our Event type. For example, when I run go to definition on the event onDidChangeActiveColorTheme in this file:

https://github.com/microsoft/vscode/blob/a6d7d86e85799e6d243aed4430456627c76f7782/src/vs/workbench/api/common/extHost.api.impl.ts#L672

I want to navigate to where the extHostTheming.onDidChangeActiveColorTheme property is set instead to the call signature of Event.

Perhaps this example is too specific and there are other cases where you really do want to go to the call signature? Would it be possible to have go to definition go to the property definition and some other operation (such as go to declaration) go to the original signature?

[RyanCavanaugh]

We can tweak it however we like; I'm not dead set on any particular behavior. Personally I don't find the "definition" vs "declaration" distinction to be particularly meaningful, but I can see the value in having one go one place and the other go elsewhere.

I think if you had overloads scattered in a few different declarations, which is somewhat common, it's useful to go to the right one. We didn't do this for a while and people found it very annoying. There should be some way to get to the selected overload; maybe this is just putting the caret between the method name and the parentheses.

[bpasero]

@RyanCavanaugh I am really hit badly by this, see this flow for example:

I am not ending up on the type I want anymore...

[RyanCavanaugh]

@bpasero that looks like a totally different bug? OP is about returning two definitions when one is expected

[bpasero]

@RyanCavanaugh I don't know if that is different, I filed https://github.com/microsoft/vscode/issues/119304 and it was closed as duplicate of this one. There are 2 definitions, but I have a setting enabled in VSCode to automatically go to the first definition. I would expect it should go to the most relevant.

[RyanCavanaugh]

It seems like that's a pretty bad setting to turn on.

[bpasero]

😕

[jrieken]

Can we start to treat this as a regression please. This was clearly working for years and has degraded significantly - any Event member in VS Code cannot be navigated to anymore.

[jrieken]

okay, so something in VS Code has changed with https://github.com/microsoft/vscode/pull/117424. There is some sorting happening too early and then we don't know anymore what TS returned as the first element. The first is the most relevant for us and the one we would goto by default. So, assuming TS has the order well defined, e.g the type definition last, this might be all good