`Go to Implementations` command to show the declaration if not abstract

[FMorschel]

If you request Go to Implementations on an abstract member, and there is a concrete implementation for it, you get directly moved there by VS Code (if you have more than one, it will show Peek, and you can choose an implementation to open). This command works both in the declaration or invocation of the member (in this case, A.doSomething).

If the method is not abstract, this command gives no results. This creates a small UX problem, because if I want to see the implementation of the methods below, the most consistent way of doing it is either Go to Definition and then Go to Implementations (if abstract) or try Go to Implementations first (in a package like analyzer this is what I tend to do since most classes are abstract) and if nothing happens, to Go to Definition.

But if the definition is in itself an implementation, I was expecting the Go to Implementations command to return that instead of nothing.

Repro:

abstract class A {
  void doSomething();
}

class B implements A {
  @override
  void doSomething() {
    print('Doing something in class B');
  }
}

class C {
  void performAction() {
    print('Performing action in class C');
  }
}

void foo(A a, C c) {
  a.doSomething();   // Request `Go to Implementations` get moved to B.doSomething
  c.performAction(); // Request `Go to Implementations` get `No implementation found for 'performAction'` message
}

CC @bwilkerson @DanTup

[DanTup]

It sounds reasonable to me - it would be convenient if "Go to implementations" just worked whether or not the target abstract, otherwise it's two steps away (Go To Definition, then Go to Implementations).

Looks like this is standard LSP (textDocument/implementations) though, so we should verify whether the spec says anything about this (and if not, compare to languages like TypeScript).

[bwilkerson]

Looks like this is standard LSP (textDocument/implementations) though, so we should verify whether the spec says anything about this (and if not, compare to languages like TypeScript).

Agreed.

I don't remember how implementations is implemented, but we should make it consistent and then document the design we've settled on.

[DanTup]

The LSP spec isn't particularly detailed, but says:

The go to implementation request is sent from the client to the server to resolve the implementation location of a symbol at a given text document position.

I think I would probably interpret this as requested here (no restriction on only working if it's abstract).

I also tested with TypeScript, and it does behave as requested there:

go_to_implementations.mp4

Looking at the code, I think some of this might be LSP-specific. The legacy protocol works by sending a notification with the implementation data, whereas in LSP we locate the element at a given location with TypeHierarchyComputerHelper and then use SearchEngine.appendAllSubtypes and search in those subtypes.. Probably we just need to include the target class itself in the list (and ensure we don't add and results that are abstract, if we don't already).

[bwilkerson]

Yes, we should add the target class.

What about the case where the implementation in the target class overrides a member from a higher parent? Do we want only subclasses of the target class, or should we include subclasses of the highest declaring parent?

The latter might be noisy, but sometimes users might want the longer list.