Skip to content

Commit 211d905

Browse files
committed
by/transpile runs detached too, and both requests are tested through the server
`transpileForBuild` was fixed by running the transpile on a thread that has attached no database, and its own message said the hole belongs to the transpiler rather than to that request — "any other server request that reaches a type-directed transpile has the same hole". There is one, in the next file over. `by/transpile` builds a database on both of its forward paths: the fragment path unconditionally, and the whole-document path whenever a pre-pass rewrote the source. Both panicked with the same "Cannot change database mid-query", caught by the handler wrapper and returned to the editor as a fault in the server. The thread hop is now one implementation that both handlers call rather than two spellings of it, and the module holding it says what it is: a workaround for the transpiler constructing databases mid-call, which is also why `by_stage::emit`'s transpile loop cannot be parallelised. Removing it means handing a type-aware pass a database instead of letting it build one. Wrapping the transpiler's own database reads in `attach_allow_change` was tried first, and is not enough: a salsa field accessor attaches, so there is no choke point to wrap, and wrapping the reads that were easy to find left the rest still panicking. A partial fix that looks total is worse than a named workaround. The tests are the other half of this. Neither request had any, and the ones covering the same operations call the library directly — the path that has nothing attached, and so the one path that never reproduced the bug. These go through a real server, fail without the change, and are the first coverage either request has had. Also: the panic is now caught inside the thread rather than off its join handle. The hook that captures a panic rather than printing it is armed per thread, so catching it on the join still printed a raw backtrace to the log on the way out. Also fixes two lints the picked commit left — a redundant clone, and a `match` that AGENTS.md wants written as `let...else`.
1 parent 4a2c492 commit 211d905

6 files changed

Lines changed: 216 additions & 38 deletions

File tree

‎crates/ty_server/src/server/api/requests.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ mod code_action;
1818
mod code_lens;
1919
mod completion;
2020
mod data_flow;
21+
mod detached_transpile;
2122
mod diagnostic;
2223
mod doc_highlights;
2324
mod document_symbols;
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
//! Running a transpile from a request handler.
2+
//!
3+
//! The transpiler builds databases of its own — a single-file one holding the source
4+
//! it is lowering, and the project rebuilt over that source when a pre-pass rewrote
5+
//! it. Reading a salsa field attaches the database it belongs to, and attaching a
6+
//! second one while a first is attached is a panic: "Cannot change database
7+
//! mid-query". Every request handler runs inside `salsa::attach(&db, …)`, so every
8+
//! one of those reads is a panic here.
9+
//!
10+
//! Attachment is per thread, so a thread that has attached nothing is a thread the
11+
//! nested build is legal on — which is the state the `by` command line runs in, and
12+
//! why none of this reproduces there. That is also why it went unnoticed: the tests
13+
//! that cover these same operations call the library directly.
14+
//!
15+
//! This is a workaround and worth naming as one. The nesting belongs to the
16+
//! transpiler, not to any request, so the fix that would remove this is for a
17+
//! type-aware pass to be handed a database rather than construct one. Until then
18+
//! every caller that already holds one attached has to arrange not to be — the same
19+
//! reason `by_stage::emit`'s transpile loop cannot be parallelised.
20+
21+
use std::panic::AssertUnwindSafe;
22+
23+
use ruff_db::panic::catch_unwind;
24+
25+
/// Run `op` on a thread that has attached no database.
26+
///
27+
/// `Err` when it panicked, which the caller reports rather than propagates: a client
28+
/// has one shape to read, and a request that got no answer at all leaves whatever it
29+
/// was about to do with the result in an unknown state.
30+
///
31+
/// The panic is caught *inside* the thread rather than read off its join handle. The
32+
/// hook that captures a panic rather than printing it is armed per thread, so one
33+
/// caught only by the join would still have printed a raw backtrace to the server's
34+
/// log on the way out. Caught here it is logged the way the rest of the server logs
35+
/// a panicking handler, and the caller is told only that there is nothing to return.
36+
pub(super) fn transpile_detached<R: Send>(op: impl FnOnce() -> R + Send) -> Result<R, ()> {
37+
let caught =
38+
std::thread::scope(|scope| scope.spawn(|| catch_unwind(AssertUnwindSafe(op))).join());
39+
match caught {
40+
Ok(Ok(value)) => Ok(value),
41+
Ok(Err(panic)) => {
42+
tracing::error!("the transpiler panicked: {panic}");
43+
Err(())
44+
}
45+
// the thread unwound past the catch, which leaves nothing to report but the
46+
// same answer: there is no result
47+
Err(_) => {
48+
tracing::error!("the transpiler's thread failed");
49+
Err(())
50+
}
51+
}
52+
}

‎crates/ty_server/src/server/api/requests/transpile.rs‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ use by_transforms::Config;
1616
use lsp_types::{LspRequestMethod, MessageDirection, Request, TextDocumentIdentifier, Uri};
1717
use ty_project::{Db as _, ProjectDatabase};
1818

19+
use super::detached_transpile::transpile_detached;
1920
use crate::server::api::traits::{
2021
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
2122
};
@@ -116,20 +117,33 @@ impl BackgroundDocumentRequestHandler for TranspileRequestHandler {
116117
// document that was asked about — that is the reason for answering here, since cross-module
117118
// types resolve — and the single-file path for a fragment, which has no module to resolve
118119
// against however it is transpiled.
120+
// Both forward paths run on a thread that has attached no database — see
121+
// [`super::detached_transpile`]. Every forward transpile builds a database of its
122+
// own, and doing that under this handler's attached one panics; the reverse
123+
// direction is purely syntactic and needs no database at all, so it stays here.
124+
let owned = db.dyn_clone();
119125
let response = if params.reverse {
120126
match by_transforms::reverse_transpile(source, &config) {
121127
Ok(out) => TranspileResponse::generated(out),
122128
Err(error) => TranspileResponse::failed(error),
123129
}
124-
} else if params.source.is_some() {
125-
match by_transforms::transpile(source, &config) {
126-
Ok(out) => TranspileResponse::generated(out),
127-
Err(error) => TranspileResponse::failed(error),
128-
}
129130
} else {
130-
match by_transforms::transpile_typed(db, file, &config, None) {
131-
Ok(out) => TranspileResponse::generated(out),
132-
Err(error) => TranspileResponse::failed(error.to_string()),
131+
let fragment = params.source.is_some();
132+
let transpiled = transpile_detached(move || {
133+
if fragment {
134+
by_transforms::transpile(source, &config)
135+
} else {
136+
by_transforms::transpile_typed(&*owned, file, &config, None)
137+
.map_err(|error| error.to_string())
138+
}
139+
});
140+
match transpiled {
141+
Ok(Ok(out)) => TranspileResponse::generated(out),
142+
Ok(Err(error)) => TranspileResponse::failed(error),
143+
// A panic is an answer here for the same reason a failure is: source that
144+
// does not lower is an ordinary state for a file being edited, and a client
145+
// renders an error response as a fault in the server.
146+
Err(()) => TranspileResponse::failed("the transpiler panicked".to_owned()),
133147
}
134148
};
135149

‎crates/ty_server/src/server/api/requests/transpile_for_build.rs‎

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
use std::borrow::Cow;
3333

3434
use by_stage::restage::{Restage, restage_one};
35+
36+
use super::detached_transpile::transpile_detached;
3537
use lsp_types::{LspRequestMethod, MessageDirection, Request, TextDocumentIdentifier, Uri};
3638
use ty_project::ProjectDatabase;
3739

@@ -107,38 +109,24 @@ impl BackgroundDocumentRequestHandler for TranspileForBuildRequestHandler {
107109
return Ok(None);
108110
};
109111

110-
// **On a thread of its own, and that is not an optimisation.** Every request handler runs
111-
// inside `salsa::attach(&db, ...)`, and the transpiler builds a *second* database when a
112-
// pre-pass rewrites the source it is about to lower — so it can resolve the file's imports
113-
// against the real project with the rewritten text overlaid. Attaching that one while this
114-
// one is attached is "Cannot change database mid-query", which salsa panics on, and a
115-
// panicking handler is what the user sees as "ty encountered a problem".
116-
//
117-
// Attachment is per thread, so a thread that has attached nothing is a thread the nested
118-
// build is legal on — which is exactly the state the `by` command line runs this in, and
119-
// why it works there. Scoped so the borrow of `db` is the same one this function has.
120-
// A clone rather than a borrow, because a salsa database is not `Sync`: it is cheap to
121-
// clone and a clone is how every one of this server's worker threads already holds one.
112+
// On a thread of its own, and that is not an optimisation — see
113+
// [`super::detached_transpile`] for why a transpile cannot run on a thread that
114+
// already has a database attached. A clone rather than a borrow, because a salsa
115+
// database is not `Sync`: it is cheap to clone and a clone is how every one of this
116+
// server's worker threads already holds one.
122117
let owned = db.clone();
123-
let directory = params.build_directory.clone();
118+
let directory = params.build_directory;
124119
let file = path.as_std_path().to_path_buf();
125-
let restaged = std::thread::scope(|scope| {
126-
scope
127-
.spawn(move || restage_one(&owned, &directory, &file))
128-
.join()
129-
});
130-
131-
let restaged = match restaged {
132-
Ok(restaged) => restaged,
133-
// The transpile panicked rather than failed. Reported as a refusal for the reason an
134-
// error is: a client has one shape to read, and a debugger that got no answer at all
135-
// would leave the tree it is about to write into in an unknown state.
136-
Err(_) => {
137-
return Ok(Some(Restage::Refused(by_stage::restage::Refusal {
138-
refused: "the transpiler panicked while re-staging this file".to_owned(),
139-
diagnostics: Vec::new(),
140-
})));
141-
}
120+
let restaged = transpile_detached(move || restage_one(&owned, &directory, &file));
121+
122+
// A panic rather than a failure is reported as a refusal for the reason an error is:
123+
// a client has one shape to read, and a debugger that got no answer at all would
124+
// leave the tree it is about to write into in an unknown state.
125+
let Ok(restaged) = restaged else {
126+
return Ok(Some(Restage::Refused(by_stage::restage::Refusal {
127+
refused: "the transpiler panicked while re-staging this file".to_owned(),
128+
diagnostics: Vec::new(),
129+
})));
142130
};
143131

144132
match restaged {

‎crates/ty_server/tests/e2e/main.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ mod pull_diagnostics;
4646
mod rename;
4747
mod semantic_tokens;
4848
mod signature_help;
49+
mod transpile;
4950
mod type_hierarchy;
5051
mod will_rename_files;
5152
mod workspace_folders;
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
//! The `by/transpile` and `by/transpileForBuild` requests, exercised through the
2+
//! server rather than through the library they call.
3+
//!
4+
//! That distinction is the whole reason these exist. The transpiler builds a
5+
//! database of its own whenever it lowers rewritten source, and a request handler
6+
//! runs with the project database already attached — so the nested one is a second
7+
//! attachment on the same thread, which salsa refuses with "Cannot change database
8+
//! mid-query". Nothing about it reproduces from the command line, where nothing is
9+
//! attached, and the `crates/ty` tests that cover the same operations therefore
10+
//! passed throughout. Only a request does it.
11+
12+
use anyhow::Result;
13+
use lsp_types::{LspRequestMethod, MessageDirection, Request, TextDocumentIdentifier, Uri};
14+
use ruff_db::system::SystemPath;
15+
16+
use crate::TestServerBuilder;
17+
18+
#[derive(Debug, serde::Deserialize, serde::Serialize)]
19+
#[serde(rename_all = "camelCase")]
20+
struct TranspileParams {
21+
text_document: TextDocumentIdentifier,
22+
#[serde(skip_serializing_if = "std::ops::Not::not")]
23+
reverse: bool,
24+
#[serde(skip_serializing_if = "Option::is_none")]
25+
source: Option<String>,
26+
}
27+
28+
#[derive(Debug, serde::Deserialize, serde::Serialize)]
29+
#[serde(rename_all = "camelCase")]
30+
struct TranspileResponse {
31+
#[serde(default)]
32+
source: Option<String>,
33+
#[serde(default)]
34+
error: Option<String>,
35+
}
36+
37+
enum Transpile {}
38+
39+
impl Request for Transpile {
40+
type Params = TranspileParams;
41+
type Result = Option<TranspileResponse>;
42+
const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("by/transpile");
43+
const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer;
44+
}
45+
46+
fn params(uri: &Uri, source: Option<&str>) -> TranspileParams {
47+
TranspileParams {
48+
text_document: TextDocumentIdentifier { uri: uri.clone() },
49+
reverse: false,
50+
source: source.map(str::to_owned),
51+
}
52+
}
53+
54+
/// The fragment path: `source` is supplied, so the whole transpile runs over text
55+
/// rather than the project, and it builds an in-memory database to check that text
56+
/// against. That database is queried while the project's is attached.
57+
#[test]
58+
fn transpiling_a_fragment_answers_rather_than_panicking() -> Result<()> {
59+
let workspace_root = SystemPath::new("src");
60+
let main = SystemPath::new("src/main.by");
61+
let content = "def f(a: int?) -> int:\n return a ?? 0\n";
62+
63+
let mut server = TestServerBuilder::new()?
64+
.with_workspace(workspace_root, None)?
65+
.with_file(main, content)?
66+
.build()
67+
.wait_until_workspaces_are_initialized();
68+
69+
server.open_text_document(main, content, 1);
70+
let uri = server.file_uri(main);
71+
72+
let response = server
73+
.send_request_await::<Transpile>(params(&uri, Some(content)))
74+
.expect("the server answered nothing at all");
75+
76+
assert!(
77+
response.error.is_none(),
78+
"transpile failed: {:?}",
79+
response.error
80+
);
81+
let generated = response.source.expect("no generated source");
82+
assert!(
83+
generated.contains("def f("),
84+
"unexpected output:\n{generated}"
85+
);
86+
Ok(())
87+
}
88+
89+
/// The whole-document path, which goes through the project database — and then
90+
/// still builds one of its own, because a pre-pass rewrote the source it lowers.
91+
/// `enum class` is such a pre-pass.
92+
#[test]
93+
fn transpiling_a_document_answers_rather_than_panicking() -> Result<()> {
94+
let workspace_root = SystemPath::new("src");
95+
let main = SystemPath::new("src/main.by");
96+
let content = "enum class Colour:\n case Red\n case Green\n\nlet chosen: Colour = Red\n";
97+
98+
let mut server = TestServerBuilder::new()?
99+
.with_workspace(workspace_root, None)?
100+
.with_file(main, content)?
101+
.build()
102+
.wait_until_workspaces_are_initialized();
103+
104+
server.open_text_document(main, content, 1);
105+
let uri = server.file_uri(main);
106+
107+
let response = server
108+
.send_request_await::<Transpile>(params(&uri, None))
109+
.expect("the server answered nothing at all");
110+
111+
assert!(
112+
response.error.is_none(),
113+
"transpile failed: {:?}",
114+
response.error
115+
);
116+
let generated = response.source.expect("no generated source");
117+
assert!(
118+
generated.contains("Colour"),
119+
"unexpected output:\n{generated}"
120+
);
121+
Ok(())
122+
}

0 commit comments

Comments
 (0)