From 5c778c0c1f85182bc37557e8957634e2882ba8e4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 17:56:32 -0700 Subject: [PATCH 1/4] Give the parsing threads 16 MiB of stack Prism parses recursively, as the walk of its tree does, so each level of nesting in a file takes stack. A rayon thread has 2 MiB by default, which with Prism holds only about 1,400 to 2,500 levels of nested calls, hashes, arrays or blocks, where lib-ruby-parser held 16,000 or more. Past that, the whole run aborts with a stack overflow. Prism stops nesting after 10,000 levels and reports an error in the file. 16 MiB holds that many levels of each construct tried, where 8 MiB doesn't for arrays, parentheses, `!`, splats or indexes. A thread's stack is reserved, not committed, so peak memory on two large Rails apps is unchanged. A debug build takes more than ten times the stack per level, so the test's 500 levels overflow 2 MiB there, but not 16 MiB. --- src/packs/cli.rs | 7 +++++++ tests/list_references_test.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/src/packs/cli.rs b/src/packs/cli.rs index 3dd23390..4e271c7b 100644 --- a/src/packs/cli.rs +++ b/src/packs/cli.rs @@ -254,6 +254,13 @@ pub fn run() -> anyhow::Result<()> { install_logger(args.debug); + // Prism and the walk of its tree recurse, so a few thousand levels of + // nesting overflow a thread's default 2 MiB stack, which aborts pks. 16 MiB + // holds the 10,000 levels after which Prism stops nesting. + rayon::ThreadPoolBuilder::new() + .stack_size(16 * 1024 * 1024) + .build_global()?; + let mut configuration = packs::configuration::get(&absolute_root)?; if args.print_files { diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index f80f78dc..3b33403b 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -439,6 +439,38 @@ fn test_list_references_in_anonymous_argument_forwarding( Ok(()) } +#[test] +fn test_list_references_in_a_deeply_nested_file() -> Result<(), Box> +{ + // Prism and the walk of its tree recurse, and in a debug build this nesting + // overflows a thread's default 2 MiB stack + let file = "packs/foo/app/services/nested.rb"; + let depth = 500; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path(file), + format!("x = {}Bar{}\n", "[".repeat(depth), "]".repeat(depth)), + )?; + + assert_eq!( + list_references( + &fixture, + &[args, &["list-references", "--defining-pack", "packs/bar"]] + .concat() + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(file, Some((1, 4 + depth as u64)), "::Bar"), + ] + }) + ); + } + + Ok(()) +} + #[test] fn test_list_references_at_the_start_of_a_file() -> Result<(), Box> { let fixture = common::Fixture::new("simple_app"); From 3ed58d6394b36610259c0f19c5513ba2fcf7cc51 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 17:57:56 -0700 Subject: [PATCH 2/4] Don't print the panic that reading a Prism message recovers from pks catches the panic that `Diagnostic::message` raises on a message that isn't UTF-8, but Rust's default panic hook had already printed it, with a path into the cargo registry that ruby-prism was built from, so the run looked as if it had crashed. A hook now leaves out a panic raised while a message is read, on that thread only, and passes any other to the hook it replaced. --- src/packs/parsing/ruby/parse_utils.rs | 22 ++++++++++++++++++++-- tests/syntax_error_test.rs | 4 ++-- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 155c6305..151dff0f 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::{cell::Cell, collections::HashSet, sync::Once}; use line_col::LineColLookup; use ruby_prism::{ @@ -63,7 +63,25 @@ pub fn first_syntax_error( /// a quote that ends mid-character, as one can when a magic comment names an /// encoding other than UTF-8. The crate gives no other way to read a message. fn error_message(error: &Diagnostic) -> Option { - std::panic::catch_unwind(|| error.message().to_owned()).ok() + // The default hook would print that panic, which pks recovers from + static QUIET_HOOK: Once = Once::new(); + QUIET_HOOK.call_once(|| { + let hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + if !READING_MESSAGE.get() { + hook(info); + } + })); + }); + + READING_MESSAGE.set(true); + let message = std::panic::catch_unwind(|| error.message().to_owned()).ok(); + READING_MESSAGE.set(false); + message +} + +thread_local! { + static READING_MESSAGE: Cell = const { Cell::new(false) }; } pub fn loc_to_range(loc: &Location, lookup: &LineColLookup) -> Range { diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index 8edc4002..1a25f512 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -54,7 +54,7 @@ fn test_check_warns_about_a_ruby_file_with_a_syntax_error( fn test_check_warns_about_an_error_whose_message_isnt_utf_8( ) -> Result<(), Box> { // Read as Windows-31J, the heredoc's name ends mid-character in Prism's - // message. The panic pks recovers from prints before the warning. + // message let warning = "Warning: `packs/foo/app/services/sjis.rb` has a syntax \ error at line 2 (its message isn't UTF-8), so pks may have \ missed references in it.\n"; @@ -70,7 +70,7 @@ fn test_check_warns_about_an_error_whose_message_isnt_utf_8( .code(1) .get_output() .clone(); - assert!(String::from_utf8(output.stderr)?.ends_with(warning)); + assert_eq!(String::from_utf8(output.stderr)?, warning); } Ok(()) From 8264d285528f2caf6c4e225a0861645c81ee2b94 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 11:05:18 -0700 Subject: [PATCH 3/4] Run the command on a thread with the pool's stack rayon runs a `par_iter` that won't split on the thread that calls it, so a command with one file to parse, as `check ` and `check-contents` have, parsed it on the main thread. That thread has 8 MiB on macOS and Linux and 1 MiB on Windows, so in a release build a file nested about 9,990 deep still aborted pks. `cli::run` now runs the command on a thread with the same 16 MiB as the pool's. It keeps the global pool and spawns a thread, rather than running the command inside a pool with `install`, so that the command doesn't hold one of the pool's threads. With `RAYON_NUM_THREADS=1`, jwalk would wait a second for the busy pool and then walk nothing. The comment on the stack size now says it holds 10,000 levels in a release build, where Prism stops nesting most constructs. A debug build overflows 16 MiB at about 1,350 levels, and nested `case`/`in` patterns and long method chains go past Prism's cap. The new test runs `check` on one file nested 1,000 deep, which overflows an 8 MiB thread in a debug build. --- src/packs/cli.rs | 25 +++++++++++++++++++------ tests/check_test.rs | 25 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/packs/cli.rs b/src/packs/cli.rs index 4e271c7b..3af52abe 100644 --- a/src/packs/cli.rs +++ b/src/packs/cli.rs @@ -3,7 +3,7 @@ use crate::packs; use crate::packs::file_utils::get_absolute_path; use clap::{Parser, Subcommand, ValueEnum}; use clap_derive::Args; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tracing::debug; use super::logger::install_logger; @@ -246,6 +246,12 @@ impl Args { } } +/// Prism and the walk of its tree recurse, so a few thousand levels of nesting +/// overflow a thread's default 2 MiB stack, which aborts pks. In a release +/// build, 16 MiB holds the 10,000 levels after which Prism stops nesting most +/// constructs. +const STACK_SIZE: usize = 16 * 1024 * 1024; + pub fn run() -> anyhow::Result<()> { let args = Args::parse(); let absolute_root = args.absolute_project_root().map_err(|e| { @@ -254,14 +260,21 @@ pub fn run() -> anyhow::Result<()> { install_logger(args.debug); - // Prism and the walk of its tree recurse, so a few thousand levels of - // nesting overflow a thread's default 2 MiB stack, which aborts pks. 16 MiB - // holds the 10,000 levels after which Prism stops nesting. rayon::ThreadPoolBuilder::new() - .stack_size(16 * 1024 * 1024) + .stack_size(STACK_SIZE) .build_global()?; - let mut configuration = packs::configuration::get(&absolute_root)?; + // rayon parses a lone file, as `check-contents` has, on the thread that + // asks rather than the pool's, so the command gets as much stack too + std::thread::Builder::new() + .stack_size(STACK_SIZE) + .spawn(move || run_command(args, &absolute_root))? + .join() + .unwrap_or_else(|panic| std::panic::resume_unwind(panic)) +} + +fn run_command(args: Args, absolute_root: &Path) -> anyhow::Result<()> { + let mut configuration = packs::configuration::get(absolute_root)?; if args.print_files { configuration.print_files = true; diff --git a/tests/check_test.rs b/tests/check_test.rs index 33fa4c9f..54e86041 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -1664,3 +1664,28 @@ fn test_check_with_color_never() -> Result<(), Box> { common::teardown(); Ok(()) } + +#[test] +fn test_check_a_deeply_nested_file() -> Result<(), Box> { + // pks parses a lone file on the thread that runs the command, rather than + // the pool's. In a debug build, a thread's default 8 MiB on macOS and + // Linux overflows at about 700 levels. + let file = "packs/foo/app/services/nested.rb"; + let depth = 1000; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path(file), + format!("x = {}Bar{}\n", "[".repeat(depth), "]".repeat(depth)), + )?; + + common::pks(&fixture, &[args, &["check", file]].concat()) + .code(1) + .stdout(predicate::str::contains(format!( + "{file}:1:{}\nPrivacy violation", + 4 + depth + ))); + } + + Ok(()) +} From 276555644e77e87366799603a35191907d699970 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 11:05:47 -0700 Subject: [PATCH 4/4] Test that the panic hook still prints other panics A hook that swallowed every panic, or a flag left set after reading a message, passed the tests, since none of them checked that a panic still prints. The new test reads a message that panics and then panics itself, in a child of the test binary since the hook is the process's, and checks that only the second panic printed. --- src/packs/parsing/ruby/parse_utils.rs | 37 +++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 151dff0f..7ad5fe16 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -432,3 +432,40 @@ macro_rules! visit_constant_assignments { }; } pub(crate) use visit_constant_assignments; + +#[cfg(test)] +mod tests { + use std::process::Command; + + use super::error_message; + + const CHILD: &str = "PKS_TEST_PANIC_HOOK_CHILD"; + + // The panic hook is the process's, so the test runs in a child of the + // test binary + #[test] + fn test_a_panic_after_a_recovered_one_is_still_printed() { + if std::env::var_os(CHILD).is_some() { + // Read as Windows-31J, the heredoc's name ends mid-character + let parsed = ruby_prism::parse( + "# encoding: windows-31j\nx = <<~あ\n".as_bytes(), + ); + let error = parsed.errors().next().expect("a syntax error"); + assert_eq!(error_message(&error), None); + panic!("an unrelated panic"); + } + + let output = Command::new(std::env::current_exe().unwrap()) + .args([ + "test_a_panic_after_a_recovered_one_is_still_printed", + "--nocapture", + ]) + .env(CHILD, "1") + .output() + .unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!(stderr.contains("an unrelated panic"), "{stderr}"); + assert!(!stderr.contains("allows only UTF-8"), "{stderr}"); + } +}