diff --git a/src/packs/cli.rs b/src/packs/cli.rs index 3dd2339..3af52ab 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,7 +260,21 @@ pub fn run() -> anyhow::Result<()> { install_logger(args.debug); - let mut configuration = packs::configuration::get(&absolute_root)?; + rayon::ThreadPoolBuilder::new() + .stack_size(STACK_SIZE) + .build_global()?; + + // 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/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 155c630..7ad5fe1 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 { @@ -414,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}"); + } +} diff --git a/tests/check_test.rs b/tests/check_test.rs index 33fa4c9..54e8604 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(()) +} diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index f80f78d..3b33403 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"); diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index 8edc400..1a25f51 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(())