Skip to content

Commit 4753a20

Browse files
committed
a command runs on a stack sized for the inference it does
`run`, `build`, `transpile` and `compile` check on the thread they were dispatched to rather than through the rayon pool, and that thread got whatever stack the platform starts a process with — 1 MiB on windows, where checking a file that nests deeply enough overflowed it
1 parent d5cafee commit 4753a20

3 files changed

Lines changed: 147 additions & 9 deletions

File tree

crates/basedpython/Cargo.lock

Lines changed: 73 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/ty/src/lib.rs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,30 @@ where
5757
.context("Failed to read CLI arguments from file")?;
5858
let args = Cli::parse_from(args);
5959

60-
match args.command {
60+
// type inference recurses with the shape of the program it is checking, so
61+
// how deep a file it can survive is decided by the stack it runs on. the
62+
// rayon pool asks for `STACK_SIZE`, but the thread a process starts on gets a
63+
// platform default — 1 MiB on windows — and the commands that check on the
64+
// calling thread rather than through the pool (`run`, `build`, `transpile`,
65+
// `compile`) were overflowing it there. so the whole command runs on a thread
66+
// this codebase has sized for the job, wherever it is dispatched to
67+
std::thread::scope(|scope| {
68+
let command = std::thread::Builder::new()
69+
.stack_size(STACK_SIZE)
70+
.spawn_scoped(scope, || run_command(args.command))
71+
.context("failed to start the worker thread")?;
72+
match command.join() {
73+
Ok(status) => status,
74+
// the panic has already been reported by the default hook; carrying
75+
// it across the join keeps the process behaving as if it never moved
76+
// off the starting thread
77+
Err(payload) => std::panic::resume_unwind(payload),
78+
}
79+
})
80+
}
81+
82+
fn run_command(command: Command) -> anyhow::Result<ExitStatus> {
83+
match command {
6184
Command::Server => run_server().map(|()| ExitStatus::Success),
6285
Command::Check(check_args) => run_check(check_args),
6386
Command::Version { output_format } => Ok(by_commands::cmd_version_by(output_format)),

crates/ty/tests/by_e2e.rs

Lines changed: 50 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,30 @@ fn run_executes_module() {
294294
);
295295
}
296296

297+
/// inference recurses with the shape of the expression it is checking, and `run`
298+
/// checks on the thread it was dispatched to rather than through the rayon pool.
299+
/// on the stack a process starts with — 1 MiB on windows — a file like this one
300+
/// overflowed before that thread was sized for the work
301+
#[test]
302+
fn run_checks_a_deeply_nested_expression() {
303+
let dir = tempfile::tempdir().expect("tempdir");
304+
let terms = vec!["1"; 2000].join(" + ");
305+
fs::write(dir.path().join("main.by"), format!("print({terms})\n")).unwrap();
306+
307+
let output = Command::new(env!("CARGO_BIN_EXE_by"))
308+
.args(["run", "main"])
309+
.current_dir(dir.path())
310+
.output()
311+
.expect("failed to spawn by");
312+
313+
assert!(
314+
output.status.success(),
315+
"by run failed:\n{}",
316+
String::from_utf8_lossy(&output.stderr)
317+
);
318+
assert_eq!(String::from_utf8_lossy(&output.stdout).trim(), "2000");
319+
}
320+
297321
#[test]
298322
fn run_force_unwrap_yields_inner_value() {
299323
// `Some(x)` lowers to the `Optional(x)` wrapper; force-unwrapping it must
@@ -1780,17 +1804,36 @@ fn build_writes_a_sourcemap_beside_the_generated_python() {
17801804
let out = dir.path().join("out");
17811805
let map = fs::read_to_string(out.join("_by_sourcemap.py")).expect("sourcemap module");
17821806

1783-
// the paths in the map are the ones the build ran against, so a symlinked
1784-
// temp dir (`/tmp` on macOS) is spelled resolved there and has to be here
1785-
let resolved = fs::canonicalize(&out).expect("out directory");
1786-
let py_key = format!("{:?}", resolved.join("main.py").to_string_lossy());
1807+
// read the keys out of the file rather than rebuilding them: the build
1808+
// spells a path the way the system handed it over, which is neither the
1809+
// test's `dir.path()` (a symlink under `/tmp` on macOS) nor its canonical
1810+
// form (a `\\?\` path with the long directory name on windows)
1811+
let first_key_of = |table: &str| {
1812+
let (_, body) = map
1813+
.split_once(&format!("{table} = {{\n"))
1814+
.unwrap_or_else(|| panic!("no {table} table:\n{map}"));
1815+
let entry = body.lines().next().expect("an entry");
1816+
entry
1817+
.trim()
1818+
.split_once(": ")
1819+
.unwrap_or_else(|| panic!("no key in {table}:\n{map}"))
1820+
.0
1821+
.to_owned()
1822+
};
1823+
1824+
let mapped = first_key_of("SOURCEMAP");
17871825
assert!(
1788-
map.contains(&format!("SOURCEMAP = {{\n {py_key}: (")),
1826+
mapped.ends_with("main.py\""),
17891827
"the generated module should be mapped by its own path:\n{map}"
17901828
);
1829+
assert_eq!(
1830+
mapped,
1831+
first_key_of("DIGESTS"),
1832+
"both tables key the same generated file:\n{map}"
1833+
);
17911834
assert!(
1792-
map.contains(&format!(" {py_key}: {{\"by\": \"sha256:")),
1793-
"and digested under the same key:\n{map}"
1835+
map.contains(&format!("{mapped}: {{\"by\": \"sha256:")),
1836+
"the entry should carry a digest of each side:\n{map}"
17941837
);
17951838
// the runner shim belongs to `by run`; a build output is not an entry point
17961839
assert!(

0 commit comments

Comments
 (0)