Use Result for failed text document retrieval in LSP requests

[dhruvmanila]

Summary

Ref: astral-sh/ruff-vscode#644 (comment)

Test Plan

Not sure how to test this as this is mainly to get more context on the panic that the server is raising.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

This is great.

We should consider propagating the error instead of unwrapping, to prevent the LSP from crashing

[MichaReiser]

I never fully understood this in the LSP but could we propagate the error instead of unwrapping here?

Suggested change

	.map_err(|err| anyhow::anyhow!("Failed to get text document for the format request: {err}"))
	.unwrap();
	.context("Failed to get text document for the format request"))
	.unwrap();

I think that should be sufficient. anyhow preserves the entire error-chain

[dhruvmanila]

Yeah, I thought about doing this but then we would still have the problem that the message will only be logged if the user has turned on server messages via ruff.trace.server: "messages":

ruff/crates/ruff_server/src/server/api.rs

Lines 197 to 214 in 4ce0c42

	/// Sends back a response to the server using a [`Responder`].
	fn respond<Req>(
	id: server::RequestId,
	result: crate::server::Result<
	<<Req as traits::RequestHandler>::RequestType as lsp_types::request::Request>::Result,
	>,
	responder: &Responder,
	) where
	Req: traits::RequestHandler,
	{
	if let Err(err) = &result {
	tracing::error!("An error occurred with result ID {id}: {err}");
	show_err_msg!("Ruff encountered a problem. Check the logs for more details.");
	}
	if let Err(err) = responder.respond(id, result) {
	tracing::error!("Failed to send response: {err}");
	}
	}

[dhruvmanila]

I think I'll want to club the change that you're suggesting along with the logging infrastructure change that I'm planning for later.

[MichaReiser]

That makes sense. I would still prefer propagating because there's a workaround to get logs, but there's no workaround for a crashing ruff server.

[dhruvmanila]

My main concern here is that the user will only see the notification but no log messages. Then, the user would need to turn on server messages which would restart the server and will loose the state in which the panic occurs. So, the user would need to wait until the next time the error occurs to provide us the logs.

[dhruvmanila]

Using anyhow's Context requires cloning the Url because I can't use the reference because of lifetime issues.

[MichaReiser]

Can you use with_context

[dhruvmanila]

Same issue:

error[E0521]: borrowed data escapes outside of function
  --> crates/ruff_server/src/server/api/requests/format.rs:65:25
   |
64 |   pub(super) fn format_document(snapshot: &DocumentSnapshot) -> Result<super::FormatResponse> {
   |                                 --------  - let's call the lifetime of this reference `'1`
   |                                 |
   |                                 `snapshot` is a reference that is only valid in the function body
65 |       let text_document = snapshot
   |  _________________________^
66 | |         .query()
   | |                ^
   | |                |
   | |________________`snapshot` escapes the function body here
   |                  argument requires that `'1` must outlive `'static`

error[E0521]: borrowed data escapes outside of function
  --> crates/ruff_server/src/server/api/requests/format_range.rs:33:25
   |
30 |       snapshot: &DocumentSnapshot,
   |       --------  - let's call the lifetime of this reference `'1`
   |       |
   |       `snapshot` is a reference that is only valid in the function body
...
33 |       let text_document = snapshot
   |  _________________________^
34 | |         .query()
   | |                ^
   | |                |
   | |________________`snapshot` escapes the function body here
   |                  argument requires that `'1` must outlive `'static`

error[E0521]: borrowed data escapes outside of function
  --> crates/ruff_server/src/server/api/requests/hover.rs:34:20
   |
30 |       snapshot: &DocumentSnapshot,
   |       --------  - let's call the lifetime of this reference `'1`
   |       |
   |       `snapshot` is a reference that is only valid in the function body
...
34 |       let document = snapshot
   |  ____________________^
35 | |         .query()
   | |                ^
   | |                |
   | |________________`snapshot` escapes the function body here
   |                  argument requires that `'1` must outlive `'static`

For more information about this error, try `rustc --explain E0521`.
error: could not compile `ruff_server` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `ruff_server` (lib test) due to 3 previous errors

[MichaReiser]

Oh I see now. I think cloning is fine. It only happens in the error case and it's uncommon that errors have lifetime bounds (e.g. it prevents users from propagating errors, as you've seen here)