improve LSP #7887
improve LSP #7887
Conversation
|
@tm1000 Would you mind taking a look? I don't know LSP well enough to judge |
There was a problem hiding this comment.
My biggest issue with this is I know from my own work on the LSP if you disable cache by just setting file provider that the LSP will eventually break while in use. This is because psalm relies on its own cache to reference other files. If there is no cache when working with a large code base you will start getting random issues or no issues at all.
Instead a clear cache on boot option is better. Which essentially forces a re-indexing.
The rest looks pretty good and matches similarly to the work I have been doing in my upcoming PR
| new ProjectCacheProvider(Composer::getLockFilePath($current_dir)) | ||
| ); | ||
| if (isset($options['no-cache']) || isset($options['i'])) { | ||
| $providers = new Providers( |
There was a problem hiding this comment.
Unless something has changed. When I tried this last it totally breaks the LSP. Cache must remain on. A better way to do this is provide the ability to clear cache on boot.
There was a problem hiding this comment.
I use LSP on some projects that are too small to detect problems with cache. I agree with using cache, but I really want no-file-cache as default (ParserCacheProvider($config, false). Every time the didChange event fires, Psalm does a file analysis and caches the files in the project, which actually happens all too often. I hope you will have no problem turning this off.
There was a problem hiding this comment.
The better way to achieve what you want is to remove the group listener that performs analysis.
As I said disabling the cache breaks psalm on larger projects and including this will only cause more issues from users who don't understand what's going on
|
The issue with removing the cache still exists in this PR and I believe overall that's a bad idea from my own testing. This week I'll run this pr through a few tests of my own to see how it performs. |
|
For reference the work I've been doing is here: https://github.com/tm1000/psalm/tree/feature/upgrade-lsp (this also resolves #7881) The only reason this is not a PR yet is I'm working on writing unit tests so that the LSP is my reliable through community contributions. |
This comment was marked as outdated.
This comment was marked as outdated.
|
@tm1000 you're right, I've noticed that Psalm actually works incorrectly when cache is missing. PR is ready! |
| @@ -361,18 +358,6 @@ public function codeAction(TextDocumentIdentifier $textDocument, Range $range): | |||
| return new Success(null); | |||
| } | |||
|
|
|||
| $all_file_paths_to_analyze = [$file_path]; | |||
There was a problem hiding this comment.
Note that code action can actually be passed the code through context from the issue itself. This is what I did in my own upcoming PR. But this works for now
| @@ -266,8 +265,6 @@ public function hover(TextDocumentIdentifier $textDocument, Position $position): | |||
| */ | |||
| public function completion(TextDocumentIdentifier $textDocument, Position $position): Promise | |||
| { | |||
| $this->server->doAnalysis(); | |||
There was a problem hiding this comment.
Removing this is fine but because code analysis happens in the listener for group messages it gets hammered. So the analysis calls will still happen quite frequently unless the group analysis listener is removed
| new ProjectCacheProvider(Composer::getLockFilePath($current_dir)) | ||
| ); | ||
| if (isset($options['no-cache']) || isset($options['i'])) { | ||
| $providers = new Providers( |
There was a problem hiding this comment.
The better way to achieve what you want is to remove the group listener that performs analysis.
As I said disabling the cache breaks psalm on larger projects and including this will only cause more issues from users who don't understand what's going on
|
@orklah this is good to go now. Additionally I can fix the test-with-real-projects but that error seems to be different for ever other PR I've checked it against |
test-with-real-projects is kinda special, we have our own forks of those projects and every time we make a change that report new (legit) errors on those real projects, we have to update the baseline on the fork. I have to get around to do that (or check if I lack permissions to do so) but I don't have a lot of time these days |
orklah
added
the
release:fix
|
Thanks! |
fixes #7881