nwolverson / purescript-language-server

MIT License
183 stars 41 forks source link

Diagnostics on type #170

Closed wclr closed 2 years ago

wclr commented 2 years ago

This PR adds getting diagnostics on type capability. Currently, the compiler doesn't support flawless workflow for this feature, so it is implemented with some drawbacks.

What we lack from purs-ide:

This PR fixes it in an experimental way (and it works ok).

Current implementation:

Settings:

More advanced:

Some details of internal changes:

Questions:

wclr commented 2 years ago

On possible implementation strategies of the feature:

1) Do not touch externs/cache-db. We send purs source by copying it into temp directory. In this case purs ide generate updated externs and places in cache-db entries with dummy 1800 dates, wich will cause a rebuild be performed for updated modules during next full rebuild.

+ modules see online changes in other modules.
- if the code was edited but not updated eventually it will still cause the rebuild
- disagnostics on open (if enabled) will cause the rebuild
- foreign modules/declarations are not checked

To reduce the full build polution, we exlude library files from diagnostics on open.

+ foreign modules/declarations are are checked when `diagnosticsCodegen` enabled.

This is a default strategy enabled with: `diagnosticsOnType` and `diagnosticsOnOpen` settings.

2) Do save/revert externs/cache-db Before sending files to purs ide we save the state of cache-db and externs of the module, after we get diagnostics we revert changes back. Purs ide, in this case, returns to the old state: as purs-ide checks cache-db file modified state (which will be changed externally by revert operation) it will reload all the modules (externs) from a disk.

- modules DON'T see online (not saved) changes in other modules
- additional hacky fs (save/revert) operations
+ full build is not polluted (if no eventual changes made)

Modification of this strategy: to revert externs only for documents with version 1 (and if the content is the same as on disk), this allows to avoid polluting of full build on file open and allow:

+ modules see online changes of other modules
+ diagnostics of opened files doesn't pollute full build
- polluting rebuild (in case of any unintentional update of file content)

3) Use fake module names. Do not touch externs/cache-db (not implemented). We replace the module name in the source with some fake one, and send it to purs ide, so output and cache-db get populated with those fake entries, but it should not have an effect on real modules.

- modules DON'T see online changes in other modules
- modules (their externs) are built into a fake directory in the output, so they pollute the output directory and cache db with those fake entries.
+ full build should not be polluted
+ foreign modules/declarations will be checked
wclr commented 2 years ago

@nwolverson I've updated the PR to the current verson and would like it to be merged eventually. So it would be nice if you review and check it. This PR also involves some refactoring to state management api (setter/getter) but nothing significant.

I've seen this commit as the only thing that added build features, and this PR I belive should cover it.

I use diagnostics on type feature for a long time though though with update compiler version (without involving FS), without it has obvious flaws, maybe some of strategies should be even just removed (hacky manipulations with externs).

Maybe @i-am-the-slime could check this too.

i-am-the-slime commented 2 years ago

@wclr @nwolverson What's the status on this? How can we/do we still want to move this forward?

wclr commented 2 years ago

@i-am-the-slime can you try this branch for yourself?

nwolverson commented 2 years ago

I'm testing this at the moment.

For better or worse the project I'm in has some files that take a significant time to rebuild, I'm not sure what the best way to make that better - perhaps just visibility of what's happening, a way to configure excluded files (simpler than the heuristic version included in the other timeline), I don't know. Not a blocker, the feature can be turned off.

The compiler PR - that needs to evolve but that can happen after this is merged.

Revert externs/cachedb hack - I need to think about that. But it looks easy to remove so don't care too much.

wclr commented 2 years ago

Revert externs/cachedb hack - I need to think about that.

I don't really like those hacks, so I implemented to see how this works out even with thouse hacks there some flaws and speed is definetly not the best. But I don't worry about them myself much, because I use updated compiler.

I think we really should move the sugessted compiler PR forward after some additional considerations, it should not break anthing, but will allow this feature be much more robust.

At the same time there are still some issues with how purs ide works, wich should be fixed to make rebuild process smoother (esp. it refers to the interaction of ide process with full build results).

@nwolverson you may want to try it and compare compile speeds for your projects.

wclr commented 2 years ago

@nwolverson

For better or worse the project I'm in has some files that take a significant time to rebuild

I've recently been working with large files 2000-4000 lines and they really take significant time to be rebuilt by purs ide. Also, this can refer to modules that depend on large files: I have files ~100 line that use a module of 4000 lines (which also has a lot of exports ~500) and those small modules take long to be rebuilt too (this was a surprise for me, maybe like purs ide does something not very efficiently in such cases)

By long, I mean more than one sec: 1,5~4 sec. This visual feedback delay also happens only when a module is built without errors, when there are errors even with large modules usually it is quick to bring the error feedback.

So this large delay is more about compler speed, not file system usage .

Are you talking about large files? What time do diagnostics take in your case? Does this slow experience refer to successful (without errors) rebuilds?

nwolverson commented 2 years ago

We have one file that is taking about 15s to rebuild (and it's not huge, if we can make a standalone repro we'll raise an issue). And yeah even in that case, if there's a parse error the build is lightning fast, it's just when typechecking goes bad that problems arise.

So my experience just running on this branch for a while - I got used to it, normally don't notice it and grow to expect it. Couple of negatives:

Anyway not reasons not to proceed, only not to be default.

wclr commented 2 years ago

Ah, I wanted to do a couple of clean ups/updates for this one.

nwolverson commented 2 years ago

Go ahead and do them against master, I'm not ready to do a release anyway - need changes in vscode extension and finish updating build for 0.15.0/esmodules things.

i-am-the-slime commented 2 years ago

Just got notified about the activity here. Sorry for not getting back earlier. One bad bug I had in my version of this was related to #177 . I don't know if @wclr 's version is also affected by this. So when I would be renaming a module it would produce lots of folders in output:

Module.Before Module.Befor Module.Befo Module.Bef ... Module.Afte Module.After

The autocomplete would then lead to suggesting 20 different modules (19 of which wrong) for an import from that module.

nwolverson commented 2 years ago

@i-am-the-slime I observed the same thing with another LS, I think possibly Haskell?

i-am-the-slime commented 2 years ago

I don't know, the Haskell LS only ever works for about 20 minutes for me and I use it every 2 years or so. In the PS case, I think @wclr is right here: https://github.com/nwolverson/purescript-language-server/issues/177#issuecomment-1155003404

wclr commented 2 years ago

So when I would be renaming a module it would produce lots of folders in output:

Right, when updating the module name with FS involved it creates all those intermediate names in the output, which is not nice of course. Also, the compiler keeps each such (intermediate/stale) module name in cache-db.json and actually they never(!) got removed from there, not even while on a full build.

I will write up my proposal in the corresponding issue.

@i-am-the-slime you may also try compiler's PR, it is free from those problems.

i-am-the-slime commented 2 years ago

@wclr I'll try it if you rebase/merge master! Also I think we should get the PR through or agree on an alternative solution. I hate these age old PRs.

wclr commented 2 years ago

@i-am-the-slime I've opened new PR https://github.com/purescript/purescript/pull/4362, reflecting in the description the latest version of actual changes.

@nwolverson probably now you could review it more closely and say what you think about the proposed solution.

i-am-the-slime commented 2 years ago

@wclr Very nice. I guess I also have to build my own version of the language server to make use of this, then?

wclr commented 2 years ago

@i-am-the-slime certanly, from this branch (or the current master as it was merged but not released), and set purescript settings diagnosticsOnType, diagnosticsOnOpen to true.