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

Multiple session and request handling #2526

Open
rchl opened this issue Oct 4, 2024 · 10 comments
Open

Multiple session and request handling #2526

rchl opened this issue Oct 4, 2024 · 10 comments

Comments

@rchl
Copy link
Member

rchl commented Oct 4, 2024

The new version of Volar (Vue Language Features) is expected to run in Vue files alongside a typescript server (LSP-typescript, for example). This seems to create issues with handling of some requests since some requests are supposed to go to LSP-volar and some to LSP-typescript. For example within a script tag like:

<script lang="ts">
export default {
  methods: {
    getSomething(key: string): HTMLElement {
      // ...
    }
  }
}
</script>
  • "expand selection" request should go to LSP-volar
  • "go to definition" should go to LSP-typescript

We have prioritity_selector which can force all requests go to one or the other but in this case it doesn't help because we need different behavior for different requests and scopes are not sufficient for determining which one to trigger.

Observing VSCode, it seems like triggering "go to definition" triggers requests both to Volar (which returns an empty list) and its internal Typescript server (note that it's not technically LSP-based in there).

But then I also see some issues with expand selection in VSCode that I don't see in ST. For example here it incorrectly expands selection to just set or Timeout in setTimeout:

Screen.Recording.2024-10-04.at.10.33.28.mov

So not sure what is the best way for handling this issue.

@predragnikolic
Copy link
Member

predragnikolic commented Oct 4, 2024

Thanks for the info @rchl .

I feel a bit down that Volar decided to require LSP clients to invest time and thoughts just to make it work.
We stumbled upon a few language servers that required a bit of client side code, but this is not the same. There are a few language servers that gives priority to VS Code, to only work there by default, and they do not take into account other LSP clients, I do not encourage that.

I would not with invest my time in adopting this approach. I cannot say that Volar is not following the spec. But deciding what request should be handled by what server is not something that is seen so far. Why does Volar do that?

Sorry for the ping @johnsoncodehk, is there a discussion on GitHub where we can get some insights on why things are done the way they are done?

@rchl
Copy link
Member Author

rchl commented Oct 4, 2024

To be fair it can run independently from LSP-typescript when changing the hybridMode setting to false. Though that's less memory efficient and maybe with some other downsides.

@rchl
Copy link
Member Author

rchl commented Oct 16, 2024

Regardless of Volar, I'm thinking that it makes sense to request data from multiple servers for requests that return a list of things (definition, references etc). We are already doing it for things like code actions after all.

@predragnikolic
Copy link
Member

predragnikolic commented Oct 16, 2024

I misunderstood the issue.

Is the request is to send a request to all language servers supporting the capability?

Initially I thought the the request is to introduce a mechanism to orchestrate certain requests to certain language servers.

@rchl
Copy link
Member Author

rchl commented Oct 16, 2024

Initially I thought the the request is to introduce a mechanism to orchestrate certain requests to certain language servers.

I've intended to describe the problem in this issue rather than suggesting specific solution. So what you've mentioned could potentially be a solution but likely not the most optimal. Requesting Location's from multiple capable servers seems like the correct thing to do though.

@jwortmann
Copy link
Member

I don't use multiple servers at the same time, but I imagine for definitions, references, etc. to merge the responses would not be very useful, because then you would have duplicate results everywhere. In particular if you use "goto definition" on a symbol, instead of directly scrolling to the location or opening the file, it would then always open the command palette first with the available results. So it would require an additional step with user input.

@rchl
Copy link
Member Author

rchl commented Oct 17, 2024

For cases where there are two servers running that handle similar functionality (pyright and pylsp for example) it could be a problem indeed. It's solvable by disabling relevant capabilities or server settings though.

I'm thinking more of cases like Single File Components where there are multiple servers running to handle different parts of the file. That case would benefit from this feature and there should be no issues since then only one of the servers should respond with non-empty response for any particular location.

@jwortmann
Copy link
Member

I'm thinking more of cases like Single File Components where there are multiple servers running to handle different parts of the file.

Okay. But shouldn't this already be possible? The "Goto" features use best_session which should select the appropriate session via the best scope name match for the position:

def best_session(view: sublime.View, sessions: Iterable[Session], point: int | None = None) -> Session | None:
if point is None:
try:
point = view.sel()[0].b
except IndexError:
return None
try:
return max(sessions, key=lambda s: view.score_selector(point, s.config.priority_selector))
except ValueError:
return None

For example a <style> block in a HTML file has the scope text.html.basic source.css.embedded.html and

>>> sublime.score_selector('text.html.basic source.css.embedded.html', 'text.html.basic')
6
>>> sublime.score_selector('text.html.basic source.css.embedded.html', 'source.css')
32

If LSP-typescript and LSP-volar use the same base "selector", shouldn't it work if one of them increases the priority for script blocks via "priority_selector"?

@rchl
Copy link
Member Author

rchl commented Oct 18, 2024

If LSP-typescript and LSP-volar use the same base "selector", shouldn't it work if one of them increases the priority for script blocks via "priority_selector"?

Check my initial comment for the exact case where this wouldn't work.

@jwortmann
Copy link
Member

Oh, right. I guess then it would need something to set the priority for a given position and for each request type individually.

Still I think just sending requests to all sessions and then merging all responses would be the wrong approach. It works for things like code actions and diagnostics because those are more or less unique to each language server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants