Request For Running Psalm Upon Updating Document State

[PythooonUser]

Summary

Currently, it seems that Psalm only analyzes a document when you save it. E.g. for a syntax error, the error message only appears after saving, and disappears only after saving when fixing it.

Under certain circumstances this update seems to work without saving, probably because of some caching of a previous document state?

Expected Behavior

I would expect Psalm to report errors on any document state change, without me needing to save.

[tm1000]

Hi there

I can tell you that this works 100% on change and not only save. I'm not sure what you've got going on but a coworker of mine just complained that he wishes it wouldn't update on change.

In fact I was just working on the language server yesterday and it definitely updates on change without needing to save.

[tm1000]

Here's the comment from my coworker from yesterday for reference

@tm1000 is there any way you could change the psalm plugin so it doesn't run all the time? Only on save or something?

[PythooonUser]

Hey :wave:

Funny. So, do you consider making it an option?

Why did your co-worker suggest this? Low performance?

I can't think of any other system that only works on save (other than formatters).

[tm1000]

The internal mechanics of psalm don't lend themselves to live editing. Psalm was written first as a static analyzer you run after you are done editing. Later the original author added the language server, which is quite nice, however it uses php-parser to analyze files and that gets very confused when you have syntax errors and such as you are live typing. Most often this causes errors (as seen in the other issue you posted) that should at least be caught.

For him it's because while he's typing psalm will get confused and mark lines as wrong and such. The method on the server for live editing is called "textdocument/onchange" and a copy of the document as-is is sent from the client to the server. In psalm this copy then replaces the copy of the file that was read from the file system. On save that is then further replaced by another read of the file system (in both cases the client sends the file back to the server for save it's just under "/onsave"

The option already sort of exists as a cli as you can define lines that will be read for onchange events. But I've never used it

[PythooonUser]

I understand. Have you had a look at the https://github.com/microsoft/tolerant-php-parser project? It's unfortunately not ready for production yet and switching the parser backend is no easy task.

On the other hand, it makes sense for a linter to work on syntactically correct code only.

I've also seen some confusion going on, as I'm using the intelephense extension, along with sonarlint. Ranges get messed up, errors stay after I fixed them etc.

[tm1000]

I'm merely the guy who works on the vscode extension and then also some bugs and features for the language server. Changing psalms parser out would be a gigantic task. The parser they use now was written and maintained by the lead developer of php's c codebase for the last 10 years or so.

Are you saying you see errors sticking around from intelephense?

[tm1000]

Also I wonder what the use case scenario Microsoft is going with there. Are they going to write a replacement for the Felix (not you the other Felix) intelisense php extension that broken about a year ago?

[tm1000]

Got my answer: https://github.com/microsoft/tolerant-php-parser/issues/351#issuecomment-789289998

[PythooonUser]

I just mentioned the parser because it is specifically designed for IDE usage, so it can handle incomplete code. Not sure what the plan for implementation is. The original author left the project time ago.

The behavior is really erratic. Not sure which completion items/hovers/errors etc. have issues. Sometimes it updates only on save, sometimes it does on change. Recently the ranges got messed up and squiggly lines showed up at wrong places, hovers would highlight wrong lines, etc.

When I can reproduce it, I will create a new issue (for the corresponding project, whatever that may be) :D

[tm1000]

Ok yes. I have seen what you are talking about. If you wish you can open a new bug here for me to track it as opening it up on the upstream might be confusing to the maintainers and they don't use the LSP

I thought about reopening this ticket as well but it has nothing to do with the onChange as that does work. The issue is in the underlying project analysis code of psalm (again psalm was written first as an offline analysis tool)

I've seen this issue crop up in phpcs vscode extension as well but not as often in inteliphsense

Also it looks like Phan uses that Microsoft project as a fallback for when php-parser fails but it concerns me because the Microsoft employee says he has no time to work on it anymore and php itself is in a cycle of rapid development. Im not even sure it supported php 8.1 syntax (where-as php-parser does)

Also intelephense server is obfuscated but I think it's written in nodejs. Though not sure

[tm1000]

I'm also wondering if applying --no-cache to the server start would help somewhat

[tm1000]

I think I know the issue with this. When live typing the projectAnalyzer is being told to analyze that file many many times. I believe it gets "stuck" while analyzing and is actually returning a result from a previous request but not processing additional requests until the first request has been processed.

The best way to probably deal with this is a debounce method that can debounce the function until xxx ms has passed without the file being updated. This might honestly solve the problem outright?

[PythooonUser]

Hey,

two things.

Regarding the "getting stuck". Can I help in providing a failing functional test of sorts? And can we prevent the analyzer from getting stuck in the first place?

Also, you propose to add a delay before each execution? And if no changes happened in that time, fire the analyzer? I know that VS Code adds an increasing version for a document whenever it changes. The analyzer could also check this version with every request and discard a previous request when its version is lower than the current one. Should be a similar approach?

Is there a way for me to help you out?

[tm1000]

Well another thing on the version. The version can actually be passed back out to publishDiagnostics which I believe helps the IDE discard lower values as well. The issue in psalm is that there's no way to get the version from didChange to publishDiagnositics in a reliable way without some serious changes

I added a delay into didChange in psalm using amp (the async library psalm uses) but that didn't seem to help. I also found a few GitHub issues from the original maintainer of psalm from years ago about debouncing and he instead went a different route as a result of the maintainer of phan talking to him about it.

When I get it into a broken state the only way I can fix it is if I rewrite the entire line out slowly.

I'm wondering if doAnalysis is running way too frequently and perhaps that needs a delay. Right now it runs as part of the groupMessage protocol and signatureHelper. It's suppose to run less frequently but I'm discovering it's actually running a lot. This then triggers emit issues which triggers publishDiagnotics. I need to look more into this

If you want to help I can lead you to the code I'm looking at in the language server itself. I usually either live edit psalm or I link my checked out version of psalm to the project I'm working on as a path and slowly work my way through it.

In psalm itself the entire text of the file you are editing is sent over didChange. This text is then stored in a temporary file buffer. Then when doAnalysis runs it queried all files but if there is a temporary file set it uses that instead. As you are typing didChnage gets called numerous times so the temporary file keeps changing (with the version Id increasing but that's discarded in psalm itself)

doAnalysis also only runs against files that have been queued to run. Maybe even having a command to "re analyze file" would help temporarily at least to see if this is solvable

As for providing the failing function. I actually know what you are talking about and this is why my coworker wanted to disable didChange entirely because it was too confusing for him. So this is something o want to solve and it's pretty easy to replicate. However even my phpcs extension gets confused in similar ways so I'm not entirely sure this is 100% fixable but we should try. As for the title of this ticket didChange is functioning as intended it's just that the internals of psalm are getting confused a lot.

[PythooonUser]

Hey, @tm1000.

Do you have an upstream ticket I can have a look at? Or can you point me to the particular code section? I have not worked with the Psalm codebase before, so I can't promise anything :)

Thanks for looking into this!

[ging-dev]

@tm1000 @PythooonUser i think auto closing parenthesis is one of the cause of the above error. More precisely, something inserted too fast such as copy-paste also causes this error.

https://user-images.githubusercontent.com/42226341/164033382-0820579e-33f2-4fa9-9a76-1421a4662e87.mp4