Give the parsing threads more stack, and quiet a recovered Prism panic

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;

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<String> {
-    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<bool> = 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}");
+    }
+}

tests/check_test.rs

@@ -1664,3 +1664,28 @@ fn test_check_with_color_never() -> Result<(), Box<dyn Error>> {
     common::teardown();
     Ok(())
 }
+
+#[test]
+fn test_check_a_deeply_nested_file() -> Result<(), Box<dyn Error>> {
+    // 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(())
+}

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<dyn Error>>
+{
+    // 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<dyn Error>> {
     let fixture = common::Fixture::new("simple_app");

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<dyn Error>> {
     // 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(())