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

lsp: Update LSP linting to run incrementally after file change #1146

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

charlieegan3
Copy link
Member

@charlieegan3 charlieegan3 commented Sep 27, 2024

Fixes to #1133

This PR is a large refactoring of the language server diagnostics loops. Changes:

  • when the contents of a file with aggregate violations changes, this no longer triggers a full workspace lint
  • when the contents of any file changes, the aggregate rule state is now cached on the server and the results of this update, in addition to the full global state, is used to perform a linting run of all files but only linting for aggregate rules.
  • new tests have been added to cover this functionality and the underlying functionality which these server changes depend on in the linter.
  • server tests moved to new files since they are very long.
  • increased timeouts for go test when running in go test -race https://go.dev/doc/articles/race_detector#Runtime_Overheads
  • updated the cache to allow partial updating of diagnostics based on the relevant rules.

@charlieegan3 charlieegan3 force-pushed the agg-wkspce branch 2 times, most recently from 3ac2d56 to 16ec12c Compare September 30, 2024 11:42
@charlieegan3 charlieegan3 changed the title lsp: Update LSP linting to incrementally lint after aggregate violation state change lsp: Update LSP linting to incrementally lint after file change Oct 1, 2024
@charlieegan3 charlieegan3 changed the title lsp: Update LSP linting to incrementally lint after file change lsp: Update LSP linting to run incrementally after file change Oct 1, 2024
@charlieegan3 charlieegan3 force-pushed the agg-wkspce branch 7 times, most recently from 01c6d60 to 4f74263 Compare October 2, 2024 10:19
Copy link
Member

@anderseknert anderseknert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me! Only complaint would be all the changes not directly related to solving the issue, but they are also good changes, so why not! Might have been better off in separate PRs though :)

I'm excited to try this out!

Reason string
// OverwriteAggregates for a workspace is only run once at start up. All
// later updates to aggregate state is made as files are changed.
OverwriteAggregates bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@charlieegan3 charlieegan3 force-pushed the agg-wkspce branch 4 times, most recently from d965c5e to dfd0b39 Compare October 2, 2024 13:58
@charlieegan3 charlieegan3 marked this pull request as ready for review October 2, 2024 13:58
@charlieegan3 charlieegan3 marked this pull request as draft October 2, 2024 14:01
@charlieegan3 charlieegan3 marked this pull request as ready for review October 2, 2024 15:57
@anderseknert
Copy link
Member

I've tried this now and it's a huge improvement 👏 No more lag or even freezes, but most things just show up right away, and the aggregate violations a little later. This is great!

@charlieegan3
Copy link
Member Author

aggregate violations a little later

Yeah, I actually think that we are in a position to correct that, but I'd need to do some more testing. This is because for a per-file run, only the non agg rules are run to improve performance. Since we know that a full workspace agg rule run will take place immediately after with the updated aggregate state.

I think the solution is to provide all aggregate data but for the current file, and then run all rules in the per file lint run. It should be simple enough, but something I think might approached in another PR....

This makes a change to how workspace lint runs are run. Now, when a file
with aggregate violations is changed, a full workspace lint will still
run but only for the aggregate rules the file has. This will also be
done using cached intermediate aggregate data.
@charlieegan3 charlieegan3 merged commit 67162e6 into StyraInc:main Oct 3, 2024
4 checks passed
@charlieegan3 charlieegan3 deleted the agg-wkspce branch October 3, 2024 16:10
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

Successfully merging this pull request may close these issues.

3 participants