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

Send $/progress messages on build file parsing #2141

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

sphaerophoria
Copy link
Contributor

@sphaerophoria sphaerophoria commented Jan 10, 2025

Initial build processing can be slow. It can be desirable to know when
the initial processing is finished to ensure that LSP queries are
processing the entire code base

Implement LSP protocol for this: $/progress.
Message flow is as follows

* Client publishes that it supports the window/workDoneProgress/create
  command on initialize
* Something triggers a build file invalidation
* Server requests that the client creates a work done progress report
* Server sends the initial work done progress
* Server sends the final work done progress

Note that this commit does not attempt to send intermediate reports.
AFAICT this would involve some fairly complex hooking of the build
runner that probably isn't worth it

Implementation details:
* Followed pattern of DiagnosticsCollection. Give DocumentStore a handle
  to the thread safe transport, and send progress messages directly from
  the build worker thread
* In some cases multiple builds can happen in parallel. We need to
  ensure we aren't double starting the same progress message. Fold them
  all builds into one

(EDIT: Updated for most recent commit message)

Example of progress report in vs code
Screenshot_20250109_213649

Apologies if you don't want drive by PRs like this, let me know if there's anything I can do to make this easier on you :)

@sphaerophoria
Copy link
Contributor Author

I've stolen some logs from clangd which somewhat validate the set of messages we're sending (more than the feature working in vs code). Note that we don't send the intermediate report, so the message/percentage stuff is not necessary. This is supported by notes in the spec

/**
	 * Optional, more detailed associated progress message. Contains
	 * complementary information to the `title`.
	 *
	 * Examples: "3/25 files", "project/src/module2", "node_modules/some_dep".
	 * If unset, the previous progress message (if any) is still valid.
	 */
	message?: string;

	/**
	 * Optional progress percentage to display (value 100 is considered 100%).
	 * If not provided infinite progress is assumed and clients are allowed
	 * to ignore the `percentage` value in subsequent report notifications.
	 *
	 * The value should be steadily rising. Clients are free to ignore values
	 * that are not following this rule. The value range is [0, 100].
	 */
	percentage?: [uinteger](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#uinteger);
V[05:00:57.713] >>> {"id":0,"jsonrpc":"2.0","method":"window/workDoneProgress/create","params":{"token":"backgroundIndexProgress"}}
V[05:00:57.834] >>> {"jsonrpc":"2.0","method":"$/progress","params":{"token":"backgroundIndexProgress","value":{"kind":"begin","percentage":0,"title":"indexing"}}}
V[05:00:57.834] >>> {"jsonrpc":"2.0","method":"$/progress","params":{"token":"backgroundIndexProgress","value":{"kind":"report","message":"0/1","percentage":0}}}
V[05:01:10.996] >>> {"jsonrpc":"2.0","method":"$/progress","params":{"token":"backgroundIndexProgress","value":{"kind":"report","message":"152/2665","percentage":5}}}
V[05:04:47.141] >>> {"jsonrpc":"2.0","method":"$/progress","params":{"token":"backgroundIndexProgress","value":{"kind":"report","message":"2664/2665","percentage":99}}}
V[05:04:48.587] >>> {"jsonrpc":"2.0","method":"$/progress","params":{"token":"backgroundIndexProgress","value":{"kind":"end"}}}

@sphaerophoria
Copy link
Contributor Author

sphaerophoria commented Jan 10, 2025

I should also note, this does not trigger for the build on save path. A similar set of actions could be performed, but I didn't need it for what I'm working on. If you feel that its important to do it while we're here, I can implement that as well. I think we'd probably want a separate set of messages + in progress tracking var

@travisstaloch
Copy link
Contributor

This addresses and possibly closes #1201

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

Overall this change looks great, there is just one request that can simplify the code.

  • DocumentStore owns the build file invalidation, but Server owns the message serialization/sending.

Unless it is necessary to get the response from requests, It is perfectly fine to send messages without going through Server.zig. For example, diagnostics are send independent of the main server loop. This means that this PR can be simplified by moving the added logic into the documenstore with the following fields:

diagnostics_collection: *DiagnosticsCollection,
+ builds_in_progress: std.atomic.Value(i32) = .init(0),
+ transport: ?lsp.Transport = null,
+ supports_work_done_progress: bool = false,

The transport field should be null until setTransport has been called.

This should make the WorkerNotifier unnecessary. I would also suggest to rebase your branch because I recently simplified the message serialization logic in Server.zig which you can use as well. See c942ce7

Apologies if you don't want drive by PRs like this, let me know if there's anything I can do to make this easier on you :)

Well... It not a drive by PR if you previously watched some videos where someones talks about sending a PR to ZLS. :)

FYI: Not sure if you already encountered this but ZLS's symbol references has an issue with how it collects references across multiple files. This can cause missing references being reported in your code map.

@sphaerophoria
Copy link
Contributor Author

Ah brilliant, I'll make those changes. I was wondering if I was missing something.

FYI: Not sure if you already encountered this but ZLS's symbol references has an #1071 with how it collects references across multiple files. This can cause missing references being reported in your code map.

Ah interesting. I've definitely noticed some odd behavior. It seems to work much better if I've opened every file before retrieving references. I don't know how I manage to find references to a module from main given that bug report. The code seems to line up with what was said, but its not matching behavior I'm seeing. Either way, thanks for the heads up :)

Initial build processing can be slow. It can be desirable to know when
the initial processing is finished to ensure that LSP queries are
processing the entire code base

Implement LSP protocol for this: $/progress.
Message flow is as follows

* Client publishes that it supports the window/workDoneProgress/create
  command on initialize
* Something triggers a build file invalidation
* Server requests that the client creates a work done progress report
* Server sends the initial work done progress
* Server sends the final work done progress

Note that this commit does not attempt to send intermediate reports.
AFAICT this would involve some fairly complex hooking of the build
runner that probably isn't worth it

Implementation details:
* Followed pattern of DiagnosticsCollection. Give DocumentStore a handle
  to the thread safe transport, and send progress messages directly from
  the build worker thread
* In some cases multiple builds can happen in parallel. We need to
  ensure we aren't double starting the same progress message. Fold them
  all builds into one
@sphaerophoria
Copy link
Contributor Author

Looking at the build failure, it looks to be an environment issue related to starting kcov, not an actual reduction in coverage caused by my PR. I am probably adding any useful info here, but officially noting that I'm not planning on making any changes :P

@Techatrix
Copy link
Member

Looking at the build failure, it looks to be an environment issue related to starting kcov, not an actual reduction in coverage caused by my PR.

I issue should be resolved by ce11ba2.

Copy link
Member

@Techatrix Techatrix left a comment

Choose a reason for hiding this comment

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

looks great!

@Techatrix Techatrix merged commit 80ac2bc into zigtools:master Jan 17, 2025
3 of 4 checks passed
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