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

Multi-server views do not send requests to (all) capable servers for hover, actions etc. #562

Closed
rchl opened this issue Apr 11, 2019 · 28 comments · Fixed by #711
Closed
Labels

Comments

@rchl
Copy link
Member

rchl commented Apr 11, 2019

  • Mac OSX
  • language servers ts-ls, eslint, vetur
  • installed LSP from package control

I don't have any concrete bug repro yet but often, after starting Sublime, LSP will just not work for certain files.

I've quickly enabled logging and these were the first messages that I've gotten:

LSP: global configs ['reason=False', 'eslint=True', 'haskell-ide-engine=False', 'ts-ls=True', 'ocaml=False', 'scss-ls=True', 'phpls=False', 'vue-ls=True', 'typescript-language-server=False', 'pyls=True', 'lsp-tsserver=False', 'golsp=False', 'jdtls=False', 'cquery=False', 'polymer-ide=False', 'rls=False', 'javascript-typescript-langserver=False', 'clangd=False', 'intelephense-ls=True']
LSP: window 5 starting 1 initial views
OverrideAudit: Initializing
OverrideAudit: Sublime version is unchanged; skipping automatic report
Emmet: No need to update PyV8
Package Control: Skipping automatic upgrade, last run at 2019-04-11 12:56:26, next run at 2019-04-11 13:56:26 or after
LSP: window 5 requests eslint for /Users/../api.js
LSP: starting in /Users/.../project
LSP: starting ['node', '/Users/.../.nvm/versions/node/v10.15.3/lib/node_modules/eslint-server/lib/index', '--stdio']
LSP:  --> initialize
LSP: window 5 added session eslint
LSP: window 5 requests ts-ls for /Users/../api.js
LSP: starting in /Users/...
LSP: starting ['lsp-tsserver']
LSP:  --> initialize
LSP: window 5 added session ts-ls
server: internal/modules/cjs/loader.js:584
server: throw err;
server: ^
server: 
server: Error: Cannot find module '/Users/.../.nvm/versions/node/v10.15.3/lib/node_modules/eslint-server/lib/index'
server: at Function.Module._resolveFilename (internal/modules/cjs/loader.js:582:15)
server: at Function.Module._load (internal/modules/cjs/loader.js:508:25)
server: at Function.Module.runMain (internal/modules/cjs/loader.js:754:12)
server: at startup (internal/bootstrap/node.js:283:19)
server: at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)
LSP: LSP stream logger stopped.
LSP: LSP stdout process ended.
LSP:      {'capabilities': {'documentSymbolProvider': True, 'codeActionProvider': True, 'documentFormattingProvider': True, 'signatureHelpProvider': {'triggerCharacters': ['(', ',']}, 'completionProvider': {'triggerCharacters': ['.'], 'resolveProvider': False}, 'definitionProvider': True, 'hoverProvider': True, 'documentHighlightProvider': True, 'documentRangeFormattingProvider': True, 'workspaceSymbolProvider': True, 'executeCommandProvider': {'commands': []}, 'textDocumentSync': 1, 'documentOnTypeFormattingProvider': {'firstTriggerCharacter': '}', 'moreTriggerCharacter': [';', '\n']}, 'renameProvider': True, 'referencesProvider': True}}
LSP:  --> initialized
LSP:  --> textDocument/didOpen
LSP: <--  textDocument/publishDiagnostics
LSP:      {'diagnostics': [], 'uri': 'file:///Users/.../api.js'}

In that state, SublimeLSP doesn't provide any typescript checking on typing or hovering variables.

Could it be that failure to start eslint server makes typescript checking not work either??

(I've redacted some sensitive information).

@rchl
Copy link
Member Author

rchl commented Apr 11, 2019

I didn't have eslint-server installed for months now and typescript checking was sometimes working and sometimes not so it can't be the exact reason for the failure (maybe contributes to some timing issue?).

@tomv564
Copy link
Contributor

tomv564 commented Apr 12, 2019

It's absolutely possible that an un-startable config causes unexpected issues finding the "right" working language server session.

So if you have eslint-server enabled but not installed you have sporadic ts-ls functionality?
I'm curious if the situation changes if you disable the eslint-server config?

@rchl
Copy link
Member Author

rchl commented Apr 12, 2019

To be fair I don't think eslint failure to start is relevant. Maybe only in terms of changing timing a bit. I've now installed that server and still having same issue occasionally. I'll have to look into that issue more at one point but I don't have time for it right now.

@rchl
Copy link
Member Author

rchl commented Apr 16, 2019

I digged deeper and found the problem that is the cause of this.

Seems to be due to fundamental limitation in this package, that makes it only consider one language server per file. Or at least for some of the actions like LspHoverCommand where it gets session for file and that session happens to be for one of possibly many servers for given file.

So if I have two servers registered for a file, for example eslint and typescript, and I hover some symbol in a file, if eslint is the one that hover action picks, nothing will show up as eslint doesn't provide hover capability. Typescript does but SublimeLSP won't even attempt to ask in that case.

Probably the right way to fix it would be to let all servers handle request like hover and concatenate all results in one popup...

@tomv564
Copy link
Contributor

tomv564 commented Apr 16, 2019

Great find!
Agree that concatenating results is a promising catch-all but since servers won't reply at the same time (and some of them fail) the complexity goes up.
Feels like we can make a pretty easy improvement just by ignoring non-hover-providing servers!

@tomv564 tomv564 added the bug label Apr 16, 2019
@tomv564
Copy link
Contributor

tomv564 commented Apr 16, 2019

Another issue here is the debug logging doesn't tell you which server is receiving certain requests, perhaps we should replace the LSP prefix with the server's name there!

@rchl
Copy link
Member Author

rchl commented Apr 16, 2019

Another issue here is the debug logging doesn't tell you which server is receiving certain requests

Yes, that was annoying during debugging this problem. Had to add extra prints for that.

@rchl
Copy link
Member Author

rchl commented Apr 16, 2019

Also I was thinking that it would make sense to create separate console for LSP related (debug) messages. Then LSP prefix would definitively not be needed.

@tomv564
Copy link
Contributor

tomv564 commented Apr 16, 2019

Yes, but some users may not want that (one shared console means easier to correlate LSP messages with other debug logging)

@tomv564 tomv564 changed the title Does not always run correctly from the start Multi-server views do not send requests to (all) capable servers for hover, actions etc. Jun 7, 2019
@rchl
Copy link
Member Author

rchl commented Jul 23, 2019

I wonder how high is this issue on your (hypothetical) roadmap?

It's quite important for me as I would like to use eslint and typescript servers at the same time and currently it doesn't make sense as I will loose hover diagnostics for one of them. I wonder if I should start investigating this myself, but you probably wouldn't want someone with not-so-great understanding of the code suddenly rewriting big portions of it ;).

@tomv564
Copy link
Contributor

tomv564 commented Jul 27, 2019

It's not the highest on the roadmap :)

When you say "hover diagnostics" I am a bit confused, LSP should be showing diagnostics from both servers in the popup already. But the symbol hover requested from the server could absolutely suffer from the issue you described - and a first step would be for LSP to only request from the server that supports hover requests.

@tomv564
Copy link
Contributor

tomv564 commented Sep 5, 2019

I did some work on this (will link a PR if it matures), but also got the eslint plugin to the typescript server working, which removes one language server from your equation. Check the discussion on gitter of the last month about https://github.com/Quramy/typescript-eslint-language-service

@rchl
Copy link
Member Author

rchl commented Sep 5, 2019

I honestly can't make this plugin work. Also I wonder if it would work with plain JS files as that is what I'm using typescript lang server for mostly.

@tomv564
Copy link
Contributor

tomv564 commented Sep 10, 2019

LSP (in master) should now pick the right language server based on cursor position and capability. Would appreciate if you could test and feedback!

As discussed earlier, requesting things like completions from multiple servers and concatenating is a lot of complexity when the majority LSP users are likely served by a single process per language. In your case, I suspect the codeAction feature will not work as expected if both the typescript and eslint servers have the capability.

I didn't try this, but removing scopes from the eslint client config might have solved your issue as well.

@rchl
Copy link
Member Author

rchl commented Sep 10, 2019

I will try later. I've tried to quickly check now but can't make eslint server report anything. Need to re-figure that out.

I didn't try this, but removing scopes from the eslint client config might have solved your issue as well.

Which scopes are you thinking of? In case of vue files, it needs to run on whole file, same as LSP-vue.

@tomv564
Copy link
Contributor

tomv564 commented Sep 10, 2019 via email

@rchl
Copy link
Member Author

rchl commented Sep 10, 2019

@tomv564 I've tested now. Apparently I had to hack eslint server by hardcoding response to workspace/configuration message (#699). Otherwise, it wouldn't work at all.

But now that's done, the hover works for both servers running simultaneously but the squiggly lines added by eslint server over the errors are removed by lsp-vue server second later. So it's not very usable still.

@rchl
Copy link
Member Author

rchl commented Sep 10, 2019

I've created this LSP-eslint package that includes hacked server for easy testing. This is the hacking I had to do: https://github.com/rchl/LSP-eslint/commit/976532f8968c282a24f075a98e7d2f51d80dbb60

@tomv564
Copy link
Contributor

tomv564 commented Sep 10, 2019

Thanks, the regions disappearing for diagnostics in views is a bug - I started a fix but I'll take a bit of extra time to clean up some ugliness in that logic.

@tomv564 tomv564 reopened this Sep 10, 2019
tomv564 added a commit that referenced this issue Sep 11, 2019
The DiagnosticUpdate had a misleading "diagnostics" member that is incomplete,
this was removed.

For issue #562
@tomv564
Copy link
Contributor

tomv564 commented Sep 11, 2019

@rchl servers should not be fighting to get their squigglies to stay anymore, can you verify?

@rchl
Copy link
Member Author

rchl commented Sep 11, 2019

I can confirm that it's fixed. Thank you. :)

@rchl
Copy link
Member Author

rchl commented Sep 11, 2019

@tomv564 Getting this error though:

Traceback (most recent call last):
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/rpc.py", line 196, in handle
    handler(params, *args)
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/windows.py", line 489, in <lambda>
    lambda params: self._diagnostics.handle_client_diagnostics(session.config.name, params))
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/diagnostics.py", line 69, in handle_client_diagnostics
    Diagnostic.from_lsp(item) for item in update.get('diagnostics', []))
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/diagnostics.py", line 69, in <genexpr>
    Diagnostic.from_lsp(item) for item in update.get('diagnostics', []))
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 338, in from_lsp
    Range.from_lsp(lsp_diagnostic['range']),
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 283, in from_lsp
    return Range(Point.from_lsp(range['start']), Point.from_lsp(range['end']))
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 264, in from_lsp
    return Point(point['line'], point['character'])
  File "/Users/me/Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 256, in __init__
    self.row = int(row)
TypeError: int() argument must be a string or a number, not 'NoneType'

On commit 3b8686e

@rchl
Copy link
Member Author

rchl commented Sep 12, 2019

@tomv564 Hmm, maybe I wasn't on latest master at the time of the error. I think maybe package control override my linked LSP with latest release. I can't reproduce the error now.

@rchl
Copy link
Member Author

rchl commented Sep 12, 2019

@tomv564 Me again. Sorry, that error actually does trigger with eslint server.

Repro: In an empty file, type one character.

It seems to be something to do with offsets being out of range.

Logs:

LSP:  --> textDocument/didChange
LSP: <--  textDocument/publishDiagnostics
LSP:      {'uri': 'file:///.../_document.vue', 'diagnostics': [{'source': 'eslint', 'severity': 1, 'range': {'end': {'character': None, 'line': None}, 'start': {'character': 1, 'line': 0}}, 'message': 'Newline required at end of file but not found.', 'code': 'eol-last'}, {'source': 'eslint', 'severity': 1, 'range': {'end': {'character': 1, 'line': 0}, 'start': {'character': 1, 'line': 0}}, 'message': 'Insert `⏎`', 'code': 'prettier/prettier'}]}
Error handling notification textDocument/publishDiagnostics
Traceback (most recent call last):
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/rpc.py", line 198, in handle
    handler(params, *args)
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/windows.py", line 502, in <lambda>
    lambda params: self._diagnostics.handle_client_diagnostics(session.config.name, params))
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/diagnostics.py", line 69, in handle_client_diagnostics
    Diagnostic.from_lsp(item) for item in update.get('diagnostics', []))
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/diagnostics.py", line 69, in <genexpr>
    Diagnostic.from_lsp(item) for item in update.get('diagnostics', []))
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 338, in from_lsp
    Range.from_lsp(lsp_diagnostic['range']),
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 283, in from_lsp
    return Range(Point.from_lsp(range['start']), Point.from_lsp(range['end']))
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 264, in from_lsp
    return Point(point['line'], point['character'])
  File "/../Library/Application Support/Sublime Text 3/Packages/LSP/plugin/core/protocol.py", line 256, in __init__
    self.row = int(row)
TypeError: int() argument must be a string or a number, not 'NoneType'

LSP: <--  eslint/status
LSP:      {'state': 1}
LSP: Unhandled notification eslint/status
LSP:  --> textDocument/codeAction
LSP:      []

@tomv564
Copy link
Contributor

tomv564 commented Sep 12, 2019

Yes, that error showed up in your logs earlier and I think it's the server not following the spec with that diagnostic: Both character and line can only be integers in 'range': {'end': {'character': None, 'line': None}.
Can you double-check for me if what I read in the spec is true, and ask them if they intend to follow the spec?

@rchl
Copy link
Member Author

rchl commented Sep 12, 2019

I've created issue and PR in vscode-eslint project.

microsoft/vscode-eslint#753

@tomv564
Copy link
Contributor

tomv564 commented Sep 20, 2019

Saw your fix got merged, nice one!

To get back to this issue, are code actions from eslint and your js server working correctly?

@rchl
Copy link
Member Author

rchl commented Sep 21, 2019

Yes, all works fine as far as this issue. :)

@tomv564 tomv564 closed this as completed Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants