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.

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

server->client "show location list" request #1148

Open
sam-mccall opened this issue Nov 24, 2020 · 12 comments
Open

server->client "show location list" request #1148

sam-mccall opened this issue Nov 24, 2020 · 12 comments
Labels
Milestone

Comments

@sam-mccall
Copy link
Contributor

codeAction and codeLens are valuable generic hooks for the server to provide features.
These features are limited by what actions LSP allows the server to trigger on the client.

Today the most useful are: applyTextEdits, showMessage, showDocument (new, thanks!).
It'd be a nice addition to be able to publish a list of locations, similar to how textDocument/references results are displayed.

The most obvious use case: "show references" is the stock example of a code lens, but can't be implemented directly in LSP today. (It requires some extension like a known Command name that is handled by an editor plugin).
#788 suggests to standardize some such Commands, but a server->client request would be more composable, easier to document, be guarded by capabilities in a better-understood way, etc.

(clangd is considering implementing this as an extension as part of our codeLens support, in order to clearly document what client support is required)

@sam-mccall
Copy link
Contributor Author

Strawman:

window/showLocations: server->client request. 
Requests that the client show a list of source locations, allowing the user to jump to any of them.

ShowLocationParams {
  // The locations to show.
  locations: Location[];

  // Identifies the list being shown.
  // If the same list is already open, the contents are replaced.
  // Well-known values are: "references".
  // Ignored if the client doesn't support multiple location lists.
  locationList: string;

  // Human-readable description of the list, which a client may display.
  title: string;
}

ShowLocationResult: null

ShowLocationClientCapabilities {
  // The client supports multiple independent location lists.
  multipleLocationLists: bool;
}

(The "multiple location lists" part could be dropped - the idea is to allow a nice rich UI in e.g. VSCode while supporting editors like vim where the native location-list is a singleton)

@puremourning
Copy link

I'd be kind of concerned about the server trying to micro-manage the client UI in this regard... Under what circumstances would you expect a server to pop up a location list ? In response to some user action, or something ?

@sam-mccall
Copy link
Contributor Author

Yes, only ever in response to a user action. Particularly activating a code action/code lens.
A server could be badly-behaved and send a location-list when it doesn't make sense (after accepting a code completion?) or without any user action at all. The same is true for applyTextEdits and showDocument though.

(I do think there's a real tradeoff here - maybe doing everything declaratively is better for clients and worse for servers. But I think allowing servers to switch documents but not send references is the worst of both worlds)

@puremourning
Copy link

(I do think there's a real tradeoff here - maybe doing everything declaratively is better for clients and worse for servers. But I think allowing servers to switch documents but not send references is the worst of both worlds)

Agree, I actually had the same concern about showDocument, but in response to a codeAction ... I can see why you might want that. I personally felt like the well-defined "get location of definition" requests that already exist should be extended. Otherwise, you could start implementing any arbitrary thing with 'code actions' and no longer have a normalised protocol (code action + commands, etc become an unregulated meta language).

Anyway in summary, if this is qualified to only be valid between an 'executeCommand' request and the 'executeCommand' response (or as part of a 'codeAction' response), then I have less of a concern, as that boxes it to a "valid" action in response to a user "codeAction" request. I don't really grok what codeLens is, but if that's user-initiated, then sure that too :)

so comment on the straw man is that I think you may want to include it in the codeAction response like you can with a applyTextEdit to avoid the executeCommand round trip ?

@dbaeumer
Copy link
Member

I am not sure I understand this. What should happen with the location list. Should it stay open until a user selects something and the selected element is then returned to the server.

I am not a fan of showing a list somewhere which doesn't auto close. I raises IMO lots of problems around who owns this list and how do we update the list.

@dbaeumer dbaeumer added feature-request Request for new features or functionality info-needed Issue requires more information from poster labels Nov 24, 2020
@dbaeumer dbaeumer added this to the Backlog milestone Nov 24, 2020
@sam-mccall
Copy link
Contributor Author

I am not sure I understand this. What should happen with the location list. Should it stay open until a user selects something and the selected element is then returned to the server.

This is most likely some specialized find-references action triggered by a codelens/codeaction. E.g. "show tests of this function".

I'd expect similar UI choices to find-all-references: typically it opens a panel and it's up to the user to hide it.
It could literally just be the find-references list in VSCode, and in vim it would have to be the location list.
(Having multiple distinct lists isn't very important really, and you're right that ownership/cleanup becomes an issue)

and the selected element is then returned to the server.

Ah, I should have been clearer: when the user selects a location from the list, the editor navigates to that location. Just like the find-references UI. No coordination with the server required.

@dbaeumer
Copy link
Member

IMO this is the wrong approach to this since it is not clear to the client what to do with the list. In LSP data pushed by the server is owned by the server. Data pull by the client is owned by the client. To keep things consistent that list has to come from the server using some sort of pull request and shouldn't be pushed by the server.

@sam-mccall
Copy link
Contributor Author

sam-mccall commented Nov 26, 2020

That's a reasonable position. So how should this feature be built in a client-pull world?

One option (strictest sense of "pull"), the client needs to be given a hint that it's appropriate to explicitly request a location list in response to an action/lens.

  • CodeAction has a locationListRequest which is effectively an alternative to command/edit, and tells the client that the action is to fetch a list (passing some data to new client->server method) and display it.
  • CodeLens has a similar locationListRequest which is an alternative to command

Another option (slightly broader sense) is that a command can be a "pull" of various types of data

  • workspace/executeCommand returns a result which can include a WorkspaceEdit, Location, LocationList, message, etc

And the broadest sense of "pull" is what i'd proposed here: executing a command is a high-level request, and well-behaved servers "push" the response, but the two are not linked by the protocol.

  • executeCommand gives the server "permission" to push something back
  • it does this with server->client requests: applyTextEdits, showMessage, showDocument, showLocationList

There's some precedent for both options 1 and 3. 3 is more consistent in a "different types of actions work in a similar way" sense. But I think you're more concerned about ownership of the UI, and draw a strong distinction between say the editor pane and other panes.

(I think there's some underlying tension: language servers would love to offer contextual, language-specific features without having to fully generalize and standardize them in LSP. Generic features like codeAction/codeLens offer this, and are limited by how the server can present results. On the other hand, the richer and more freeform and "platformy" the facilities LSP provides are, the less you can shape the overall product in a good and consistent direction)

@rwols
Copy link
Contributor

rwols commented Nov 26, 2020

This seems to have overlap with #1119

For what it's worth: there are already some language servers out in the wild that expect the client to handle some of the provided commands in a codeLens/codeAction. Examples of command identifiers are (I'm not sure if I have these correct, but just from memory):

editor.action.signatureSuggest: "please show the signature help widget"
editor.action.suggest: "please show the auto-complete widget"
editor.action.showReferences: "please present a list of locations"

Due to the way VSCode works internally, if a command with the given name is registered as a command in the extension, it will run that command via the extension, instead of doing the workspace/executeCommand RPC call. This behavior is not documented in the spec. But many VSCode-focused language servers are already making great use of this behavior.

We have started to implement the same behavior: https://github.com/sublimelsp/LSP/blob/270cd2dfa5bf25584a123b9c896a7167888f0d78/plugin/core/sessions.py#L535-L545

This does mean that all clients will have to add more client glue code.

In LSP data pushed by the server is owned by the server.

I agree. But this principle does not seem applicable here. In what way is the server supposed to manage this "location list" resource? I consider this fleeting data (unlike diagnostics).

@dbaeumer dbaeumer removed the info-needed Issue requires more information from poster label Nov 27, 2020
@dbaeumer
Copy link
Member

I see a couple of options:

  1. We say that the extension integrating the server into the tool is responsible for that integration part since it will result in the best UI experience. A server documents which integrations it expects the extension does. So the extension basically interprets the command by install a corresponding handler.
  2. We standardize a set of command ids. I thing this is doable for commands for which we have corresponding LSP requests.
  3. We make this explicit in LSP by having a concrete concept that can be used instead of a command. But this can't be something generic like show list and can't contain the data to be presented since it would bring us back to square one. What we would do is to say Trigger reference search with these params or Trigger call hierarchy with these params

@puremourning
Copy link

My 2p FWIW.

  1. Experience has shown that: server vendors won't document it, and they will only implement client glue for vscode. These sorts of assumptions make it impossible to write "generic" clients which don't have per-server code. This is essential for smaller project that don't have the scale to write code for every possible LSP server (there are many examples of this type of client)

2/3. that seems doable. I.e. server can request that client makes a particular type of existing LSP request. i think I prefer the way 3 is defined personally, as the whole mechanism of 'commands' isn't truly general and assumes particular type of client implementation, whereas 3 only uses existing high level protocol concepts (admittedly, commands are part of the protocol, though they're kind of nuts-and-bolts requiring a lot of client-side knowledge of servers)

@rwols
Copy link
Contributor

rwols commented Nov 27, 2020

I am okay with any of the three proposed options. But I do agree with @puremourning that (1) won't really benefit the ecosystem in practice.

If we go with (2), then #1119 seems to become redundant. Perhaps @kdvolder and @mickaelistria want to have a say in this as well.

@dbaeumer dbaeumer added idea and removed feature-request Request for new features or functionality labels Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants