From 3c531d6469346303f0a5eef36557d01f4b420d64 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 12:43:51 -0700 Subject: [PATCH 1/8] Print `--print-files` progress to stderr `-p` printed "Started processing" and "Finished processing" lines to stdout, so the JSON from `check -o json` and `list-references` came after them and didn't parse. They are progress, not output, so they now go to stderr, where pks's warnings already go. --- CHANGELOG.md | 8 ++++++++ src/packs/parsing/mod.rs | 4 ++-- tests/list_references_test.rs | 19 +++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ddb384..3ea7991 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,14 @@ pks keeps no source positions in ERB, so a reference in an ERB file has a null `line` and `column`, and the file's references to one constant are listed once. `schema/list-references-output.json` describes the output. +### Fixes + +#### `--print-files` no longer breaks JSON output + +`-p` printed its "Started processing" and "Finished processing" lines to +stdout, ahead of the JSON from `check -o json` and `list-references`, so the +output didn't parse. They now go to stderr. + ## 0.6.0 ### Features diff --git a/src/packs/parsing/mod.rs b/src/packs/parsing/mod.rs index d711a92..41010d6 100644 --- a/src/packs/parsing/mod.rs +++ b/src/packs/parsing/mod.rs @@ -25,7 +25,7 @@ pub fn process_file( configuration: &Configuration, ) -> anyhow::Result { if configuration.print_files { - println!("Started processing {}", path.display()); + eprintln!("Started processing {}", path.display()); } let file_type_option = get_file_type(path); @@ -57,7 +57,7 @@ pub fn process_file( }; if configuration.print_files { - println!("Finished processing {}", path.display()); + eprintln!("Finished processing {}", path.display()); } result diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index cba80cf..6ecdf18 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -230,6 +230,25 @@ fn test_list_references_from_given_packs() -> Result<(), Box> { Ok(()) } +#[test] +fn test_list_references_prints_files_to_stderr() -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + + let output = pks(&fixture, &["--print-files", "list-references"]) + .success() + .get_output() + .clone(); + let references: Value = + serde_json::from_slice(&output.stdout).expect("output is not JSON"); + assert_eq!( + references, + json!({ "references": [bar_from_foo(), baz_from_foo()] }) + ); + assert!(String::from_utf8(output.stderr)?.contains("Started processing")); + + Ok(()) +} + #[test] fn test_list_references_rejects_an_unknown_pack() -> Result<(), Box> { From 55a1c77d743bedec7902256a46ae824f04f13327 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 12:45:49 -0700 Subject: [PATCH 2/8] Read `<%==` tags as Ruby in ERB Rails' `<%== %>` outputs without escaping, but the regex that pulls Ruby out of ERB took only one `=` after `<%`, so `<%== foo(Bar) %>` became `= foo(Bar)`. That doesn't parse. lib-ruby-parser recovers only part of the file, and the tag's references and every reference after it were missed: with `<%= SomeConcern %>`, `<%== Bar.bar %>` and `<%= Baz %>`, pks found only `::SomeConcern`. In three large Rails apps, these were the only files that failed to parse. Packwerk reads ERB with a real parser and handles the tag. --- CHANGELOG.md | 7 +++++++ src/packs/file_utils.rs | 3 ++- src/packs/parsing/erb/packwerk.rs | 33 +++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ea7991..a11aaea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,13 @@ pks keeps no source positions in ERB, so a reference in an ERB file has a null ### Fixes +#### References after a `<%==` tag are no longer missed + +pks read Rails' `<%== %>` tag, which outputs without escaping, as Ruby that +starts with `=`. That doesn't parse, so in an ERB file with one, the tag's +references and every reference after it were missed. `<%==` is now read like +`<%=`, so `pks check` can report violations it missed before. + #### `--print-files` no longer breaks JSON output `-p` printed its "Started processing" and "Finished processing" lines to diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index 929ddb6..f22889a 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -104,7 +104,8 @@ pub fn user_inputted_paths_to_absolute_filepaths( pub(crate) fn convert_erb_to_ruby_without_sourcemaps( contents: String, ) -> String { - let regex_pattern = r"(?s)<%=?-?\s*(.*?)\s*-?%>"; + // `<%==` is Rails' tag for output that isn't escaped + let regex_pattern = r"(?s)<%(?:==?)?-?\s*(.*?)\s*-?%>"; let regex = Regex::new(regex_pattern).unwrap(); let extracted_contents: Vec<&str> = regex diff --git a/src/packs/parsing/erb/packwerk.rs b/src/packs/parsing/erb/packwerk.rs index ee5c8bb..b936606 100644 --- a/src/packs/parsing/erb/packwerk.rs +++ b/src/packs/parsing/erb/packwerk.rs @@ -327,6 +327,39 @@ mod tests { ); } + #[test] + fn raw_output_tag_is_ruby() { + // Read as `= Bar`, the tag hid its own references and every one after + let contents: String = + String::from("<%= Foo %>\n<%== Bar %>\n<%= Baz %>\n"); + let configuration = Configuration::default(); + assert_eq!( + vec![ + UnresolvedReference { + name: String::from("Foo"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Bar"), + namespace_path: vec![], + location: Range::default() + }, + UnresolvedReference { + name: String::from("Baz"), + namespace_path: vec![], + location: Range::default() + } + ], + process_from_contents( + contents, + &PathBuf::from("path/to/file.rb"), + &configuration + ) + .unresolved_references + ); + } + #[test] fn literal_tag_is_text_not_ruby() { let contents: String = String::from( From fab7c33e1af3c648fd5a3e411f1222cf95656cf3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 12:53:10 -0700 Subject: [PATCH 3/8] Warn about files that don't parse When lib-ruby-parser couldn't parse a file, pks used whatever partial tree it recovered, or none, and said nothing, so `check` could pass while missing the file's references. Packwerk reports a syntax error as an offense. pks now prints a warning to stderr with the file and the first error, and its line in Ruby. ERB gets no line, since its lines are those of the Ruby pks makes from it. The error rides on `ProcessedFile` as `syntax_error`, which isn't serialized, so the cache format that pks shares with packwerk doesn't change. A file pks warns about isn't written to the cache, so the warning comes back on every run until the file is fixed, instead of only when the file is first parsed. Only the app's own files, its included files, warn. `expose-monkey-patches` also parses Ruby's and the gems' sources, which can use syntax pks can't parse and aren't the app's to fix; they stay quiet and cached. An empty file has no AST but no error either, so it doesn't warn. Three large Rails apps give no warnings under either parser, now that `<%==` is read correctly. The parser can't read some newer syntax, such as Ruby 3.2's anonymous argument forwarding, and those files warn too. --- CHANGELOG.md | 12 ++++ src/packs.rs | 4 ++ src/packs/caching/per_file_cache.rs | 2 + src/packs/parsing/erb/experimental/parser.rs | 7 +- src/packs/parsing/erb/packwerk/parser.rs | 7 +- src/packs/parsing/mod.rs | 72 +++++++++++++++++-- src/packs/parsing/ruby/experimental.rs | 12 ++++ src/packs/parsing/ruby/experimental/parser.rs | 8 ++- src/packs/parsing/ruby/packwerk/parser.rs | 6 +- src/packs/parsing/ruby/parse_utils.rs | 19 ++++- tests/expose_monkey_patches_test.rs | 30 ++++++++ tests/syntax_error_test.rs | 72 +++++++++++++++++++ 12 files changed, 237 insertions(+), 14 deletions(-) create mode 100644 tests/syntax_error_test.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index a11aaea..03cda36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,18 @@ pks keeps no source positions in ERB, so a reference in an ERB file has a null ### Fixes +#### pks warns about files it can't parse + +A file that pks couldn't parse gave it some or none of its references, and +nothing said so, so `pks check` could pass with violations in that file. pks +now warns on stderr, naming the file and its first syntax error, with the line +in a Ruby file. A file with a syntax error isn't cached, so the warning comes +back on every run until the file is fixed. + +pks's parser can't read some newer Ruby syntax, such as Ruby 3.2's anonymous +argument forwarding (`def foo(*) = bar(*)`), so files that use it get this +warning too. + #### References after a `<%==` tag are no longer missed pks read Rails' `<%== %>` tag, which outputs without escaping, as Ruby that diff --git a/src/packs.rs b/src/packs.rs index c0061bd..3047639 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -42,6 +42,7 @@ pub(crate) use self::parsing::process_files_with_cache; pub(crate) use self::parsing::ruby::experimental::get_experimental_constant_resolver; pub(crate) use self::parsing::ruby::zeitwerk::get_zeitwerk_constant_resolver; pub(crate) use self::parsing::ParsedDefinition; +pub(crate) use self::parsing::SyntaxError; pub(crate) use self::parsing::UnresolvedReference; use anyhow::bail; use cli::ColorChoice; @@ -276,6 +277,9 @@ pub struct ProcessedFile { pub absolute_path: PathBuf, pub unresolved_references: Vec, pub definitions: Vec, + /// Never cached, since a file with a syntax error isn't + #[serde(skip)] + pub syntax_error: Option, } #[derive( diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index 4621c8e..e8b7594 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -252,6 +252,7 @@ mod tests { } }], definitions: vec![], + syntax_error: None, } }; @@ -306,6 +307,7 @@ mod tests { absolute_path: path.clone(), unresolved_references: vec![], definitions: vec![], + syntax_error: None, }; let CacheResult::Miss(empty_cache_entry) = cache.get(&path)? else { diff --git a/src/packs/parsing/erb/experimental/parser.rs b/src/packs/parsing/erb/experimental/parser.rs index 62f9bd1..c728c90 100644 --- a/src/packs/parsing/erb/experimental/parser.rs +++ b/src/packs/parsing/erb/experimental/parser.rs @@ -1,6 +1,7 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ - file_utils::convert_erb_to_ruby_without_sourcemaps, parsing::Range, + file_utils::convert_erb_to_ruby_without_sourcemaps, + parsing::{Range, SyntaxError}, Configuration, ProcessedFile, UnresolvedReference, }; use std::path::Path; @@ -40,5 +41,9 @@ pub(crate) fn process_from_contents( absolute_path: path.to_path_buf(), unresolved_references: references_without_range, definitions: vec![], + syntax_error: processed_file.syntax_error.map(|error| SyntaxError { + line: None, + ..error + }), } } diff --git a/src/packs/parsing/erb/packwerk/parser.rs b/src/packs/parsing/erb/packwerk/parser.rs index 04d32c0..7236382 100644 --- a/src/packs/parsing/erb/packwerk/parser.rs +++ b/src/packs/parsing/erb/packwerk/parser.rs @@ -1,6 +1,7 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ - file_utils::convert_erb_to_ruby_without_sourcemaps, parsing::Range, + file_utils::convert_erb_to_ruby_without_sourcemaps, + parsing::{Range, SyntaxError}, Configuration, ProcessedFile, UnresolvedReference, }; use std::path::Path; @@ -40,5 +41,9 @@ pub(crate) fn process_from_contents( absolute_path: path.to_path_buf(), unresolved_references: references_without_range, definitions: vec![], + syntax_error: processed_file.syntax_error.map(|error| SyntaxError { + line: None, + ..error + }), } } diff --git a/src/packs/parsing/mod.rs b/src/packs/parsing/mod.rs index 41010d6..b682392 100644 --- a/src/packs/parsing/mod.rs +++ b/src/packs/parsing/mod.rs @@ -53,6 +53,7 @@ pub fn process_file( absolute_path: path.to_path_buf(), unresolved_references: vec![], definitions: vec![], // TODO + syntax_error: None, }) }; @@ -63,6 +64,50 @@ pub fn process_file( result } +/// The file's syntax error, if it is one of the app's own. The Ruby and gem +/// sources that `expose-monkey-patches` also reads would warn about syntax that +/// pks can't parse, which isn't the app's to fix. +fn syntax_error_to_warn_about<'a>( + processed_file: &'a ProcessedFile, + configuration: &Configuration, +) -> Option<&'a SyntaxError> { + processed_file.syntax_error.as_ref().filter(|_| { + configuration + .included_files + .contains(&processed_file.absolute_path) + }) +} + +fn warn_about_syntax_error( + path: &Path, + error: &SyntaxError, + configuration: &Configuration, +) { + let file = path + .strip_prefix(&configuration.absolute_root) + .unwrap_or(path); + let line = error + .line + .map(|line| format!(" at line {}", line)) + .unwrap_or_default(); + eprintln!( + "Warning: `{}` has a syntax error{} ({}), so pks may have missed \ + references in it.", + file.display(), + line, + error.message + ); +} + +/// The first error lib-ruby-parser reports in a file. What it can't parse, pks +/// can't find references in. +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct SyntaxError { + /// None in ERB, whose lines are those of the Ruby that pks makes from it + pub line: Option, + pub message: String, +} + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] pub struct UnresolvedReference { pub name: String, @@ -92,21 +137,34 @@ pub fn process_files_with_cache( paths .par_iter() .map(|absolute_path| -> anyhow::Result { - if is_stdin_file(absolute_path, configuration) { - process_file(absolute_path, configuration) + let processed_file = if is_stdin_file(absolute_path, configuration) + { + process_file(absolute_path, configuration)? } else { match cache.get(absolute_path)? { - CacheResult::Processed(processed_file) => { - Ok(processed_file) - } + CacheResult::Processed(processed_file) => processed_file, CacheResult::Miss(empty_cache_entry) => { let processed_file = process_file(absolute_path, configuration)?; - cache.write(&empty_cache_entry, &processed_file)?; - Ok(processed_file) + // So the warning comes back until the file is fixed + if syntax_error_to_warn_about( + &processed_file, + configuration, + ) + .is_none() + { + cache.write(&empty_cache_entry, &processed_file)?; + } + processed_file } } + }; + if let Some(error) = + syntax_error_to_warn_about(&processed_file, configuration) + { + warn_about_syntax_error(absolute_path, error, configuration); } + Ok(processed_file) }) .collect() } diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index a7bdf61..d7297ce 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -79,6 +79,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -108,6 +109,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -138,6 +140,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -168,6 +171,7 @@ mod tests { absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -194,6 +198,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -230,6 +235,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -266,6 +272,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -301,6 +308,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -336,6 +344,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -375,6 +384,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); } @@ -415,6 +425,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); @@ -456,6 +467,7 @@ end absolute_path, unresolved_references, definitions, + syntax_error: None, }; assert_eq!(expected, actual); diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index a72d4e6..997cce0 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -3,8 +3,9 @@ use crate::packs::{ parsing::{ ruby::parse_utils::{ fetch_const_const_name, fetch_const_name, fetch_node_location, - get_constant_assignment_definition, get_definition_from, - get_reference_from_active_record_association, loc_to_range, + first_syntax_error, get_constant_assignment_definition, + get_definition_from, get_reference_from_active_record_association, + loc_to_range, }, ParsedDefinition, UnresolvedReference, }, @@ -200,6 +201,7 @@ pub(crate) fn process_from_contents( let lookup = LineColLookup::new(&contents); let parser = Parser::new(contents.clone(), options); let parse_result = parser.do_parse(); + let syntax_error = first_syntax_error(&parse_result.diagnostics, &lookup); let ast_option: Option> = parse_result.ast; @@ -210,6 +212,7 @@ pub(crate) fn process_from_contents( absolute_path: path.to_owned(), unresolved_references: vec![], definitions: vec![], + syntax_error, } } }; @@ -238,5 +241,6 @@ pub(crate) fn process_from_contents( absolute_path, unresolved_references, definitions, + syntax_error, } } diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 8f50294..5184f42 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -5,7 +5,8 @@ use crate::packs::{ namespace_calculator::possible_fully_qualified_constants, parse_utils::{ fetch_const_const_name, fetch_const_name, fetch_node_location, - get_constant_assignment_definition, get_definition_from, + first_syntax_error, get_constant_assignment_definition, + get_definition_from, get_reference_from_active_record_association, loc_to_range, }, }, @@ -221,6 +222,7 @@ pub(crate) fn process_from_contents( let lookup = LineColLookup::new(&contents); let parser = Parser::new(contents.clone(), options); let parse_result = parser.do_parse(); + let syntax_error = first_syntax_error(&parse_result.diagnostics, &lookup); let ast_option: Option> = parse_result.ast; @@ -231,6 +233,7 @@ pub(crate) fn process_from_contents( absolute_path: path.to_owned(), unresolved_references: vec![], definitions: vec![], + syntax_error, } } }; @@ -311,5 +314,6 @@ pub(crate) fn process_from_contents( absolute_path, unresolved_references, definitions, + syntax_error, } } diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 9a1b130..2a0b2c1 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -1,9 +1,11 @@ use std::collections::HashSet; -use lib_ruby_parser::{nodes, Loc, Node}; +use lib_ruby_parser::{nodes, Diagnostic, Loc, Node}; use line_col::LineColLookup; -use crate::packs::parsing::{ParsedDefinition, Range, UnresolvedReference}; +use crate::packs::parsing::{ + ParsedDefinition, Range, SyntaxError, UnresolvedReference, +}; use super::inflector_shim::to_class_case; @@ -48,6 +50,19 @@ pub fn get_definition_from( } } +pub fn first_syntax_error( + diagnostics: &[Diagnostic], + lookup: &LineColLookup, +) -> Option { + diagnostics + .iter() + .find(|diagnostic| diagnostic.is_error()) + .map(|diagnostic| SyntaxError { + line: Some(lookup.get(diagnostic.loc.begin).0), + message: diagnostic.render_message(), + }) +} + pub fn loc_to_range(loc: &Loc, lookup: &LineColLookup) -> Range { let (start_row, start_col) = lookup.get(loc.begin); // There's an off-by-one difference here with packwerk let (end_row, end_col) = lookup.get(loc.end); diff --git a/tests/expose_monkey_patches_test.rs b/tests/expose_monkey_patches_test.rs index 758b738..91782e8 100644 --- a/tests/expose_monkey_patches_test.rs +++ b/tests/expose_monkey_patches_test.rs @@ -23,3 +23,33 @@ fn test_expose_monkey_patches() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_expose_monkey_patches_warns_only_about_the_apps_syntax_errors( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_monkey_patches"); + // Ruby's and gems' sources aren't the app's to fix, so they don't warn + let broken = "class Broken\n def a(\nend\n"; + std::fs::write(fixture.path("rubydir_stub/broken.rb"), broken)?; + std::fs::write(fixture.path("gemdir_stub/rails/broken.rb"), broken)?; + std::fs::write(fixture.path("app/services/broken.rb"), broken)?; + + let rubydir = fixture.path("rubydir_stub"); + let gemdir = fixture.path("gemdir_stub"); + cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(fixture.root()) + .arg("--experimental-parser") + .arg("expose-monkey-patches") + .arg(format!("--rubydir={}", rubydir.display())) + .arg(format!("--gemdir={}", gemdir.display())) + .assert() + .success() + .stderr( + "Warning: `app/services/broken.rb` has a syntax error at line 3 \ + (unexpected kEND), so pks may have missed references in it.\n", + ); + + Ok(()) +} diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs new file mode 100644 index 0000000..ed11ae8 --- /dev/null +++ b/tests/syntax_error_test.rs @@ -0,0 +1,72 @@ +use assert_cmd::cargo::cargo_bin_cmd; +use std::{error::Error, fs}; + +mod common; + +/// simple_app, with the cache on, since a file with a syntax error must warn on +/// every run rather than only when it is first parsed +fn fixture_app() -> Result> { + let fixture = common::Fixture::new("simple_app"); + let packwerk_yml = fixture.path("packwerk.yml"); + let contents = fs::read_to_string(&packwerk_yml)?; + fs::write( + &packwerk_yml, + contents.replace("cache: false", "cache: true"), + )?; + Ok(fixture) +} + +fn check_stderr( + fixture: &common::Fixture, + args: &[&str], +) -> Result> { + let output = cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(fixture.root()) + .args(args) + .arg("check") + .output()?; + Ok(String::from_utf8(output.stderr)?) +} + +#[test] +fn test_check_warns_about_a_ruby_file_with_a_syntax_error( +) -> Result<(), Box> { + // simple_app's empty `packs/baz/app/services/baz.rb` makes no warning + let warning = "Warning: `packs/foo/app/services/broken.rb` has a syntax \ + error at line 7 (unexpected kEND), so pks may have missed \ + references in it.\n"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + fs::write( + fixture.path("packs/foo/app/services/broken.rb"), + "class Broken\n def a\n Bar\n end\n\n def b(\nend\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, warning); + assert_eq!(check_stderr(&fixture, args)?, warning); + } + + Ok(()) +} + +#[test] +fn test_check_warns_about_an_erb_file_with_a_syntax_error( +) -> Result<(), Box> { + // pks keeps no ERB positions, so the warning gives no line + let warning = "Warning: `packs/foo/app/views/broken.erb` has a syntax \ + error (unexpected END_OF_INPUT), so pks may have missed \ + references in it.\n"; + for args in [&[][..], &["--experimental-parser"]] { + let fixture = fixture_app()?; + fs::write( + fixture.path("packs/foo/app/views/broken.erb"), + "<%= Bar.bar( %>\n", + )?; + + assert_eq!(check_stderr(&fixture, args)?, warning); + } + + Ok(()) +} From dd0083924b293b69befe09d6982675106529c129 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 12:57:21 -0700 Subject: [PATCH 4/8] Read only the referencing packs' files in `list-references` `--referencing-pack` filtered references after pks had parsed every included file, so asking about one pack cost as much as asking about all of them. Only a pack's own files can make references from it, so pks now passes just those to `get_all_references`, as `check ` passes its paths. The experimental parser still reads every file for definitions inside `get_all_references`, so it gains less. For two packs of a large Rails app, the output is the same as the full list limited to those packs, under both parsers, and the run took 2.6 s instead of 8.0 s with the default parser. A test checks that a file in another pack isn't read: one that doesn't parse would make pks warn. --- CHANGELOG.md | 3 ++- src/packs/reference_lister.rs | 27 +++++++++++++++++++++++++-- tests/list_references_test.rs | 21 +++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03cda36..7709328 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,8 @@ files alone can't. Unlike `check`, it exits 0 when references have violations. `--defining-pack` and `--referencing-pack` limit the list to references to or from the packs they name, and each can be given more than once. Given both, a -reference must match both. +reference must match both. With `--referencing-pack`, pks reads only those +packs' files, so it takes less time and memory in a large app. pks keeps no source positions in ERB, so a reference in an ERB file has a null `line` and `column`, and the file's references to one constant are listed once. diff --git a/src/packs/reference_lister.rs b/src/packs/reference_lister.rs index 8027e38..1f4de7c 100644 --- a/src/packs/reference_lister.rs +++ b/src/packs/reference_lister.rs @@ -6,6 +6,7 @@ use std::{ collections::HashSet, io::{BufWriter, Write}, + path::PathBuf, }; use rayon::prelude::{ParallelIterator, ParallelSlice, ParallelSliceMut}; @@ -49,8 +50,15 @@ pub(crate) fn list( let defining_packs = pack_names(configuration, defining_packs)?; let referencing_packs = pack_names(configuration, referencing_packs)?; - let all_references = - get_all_references(configuration, &configuration.included_files)?; + // Only the referencing packs' own files can reference from them + let owned_files; + let files = if referencing_packs.is_empty() { + &configuration.included_files + } else { + owned_files = files_of(configuration, &referencing_packs)?; + &owned_files + }; + let all_references = get_all_references(configuration, files)?; let mut references: Vec<&Reference> = all_references .iter() .filter(|reference| { @@ -130,6 +138,21 @@ fn pack_names<'a>( .collect() } +/// The included files that the packs own +fn files_of( + configuration: &Configuration, + packs: &HashSet<&str>, +) -> anyhow::Result> { + let mut files = HashSet::new(); + for file in &configuration.included_files { + let owner = configuration.pack_set.for_file(file)?; + if owner.is_some_and(|pack| packs.contains(pack.name.as_str())) { + files.insert(file.clone()); + } + } + Ok(files) +} + /// No packs given means every pack. fn selected(packs: &HashSet<&str>, pack: &str) -> bool { packs.is_empty() || packs.contains(pack) diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 6ecdf18..9069023 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -249,6 +249,27 @@ fn test_list_references_prints_files_to_stderr() -> Result<(), Box> { Ok(()) } +#[test] +fn test_list_references_from_given_packs_reads_only_their_files( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // pks warns about a file that doesn't parse, but only if it reads it + fs::write(fixture.path("packs/bar/app/services/broken.rb"), "def a(\n")?; + + assert_eq!( + list_references( + &fixture, + &["list-references", "--referencing-pack", "packs/foo"] + ), + json!({ "references": [bar_from_foo(), baz_from_foo()] }) + ); + pks(&fixture, &["list-references"]) + .success() + .stderr(predicates::str::contains("broken.rb")); + + Ok(()) +} + #[test] fn test_list_references_rejects_an_unknown_pack() -> Result<(), Box> { From bb568e553e861c8ba98636f6b442e66fd5bf4bf3 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 13:01:06 -0700 Subject: [PATCH 5/8] Share test helpers, and keep `RUST_LOG` out of tests that check stderr pks's logger reads `RUST_LOG` even without `--debug`, so a developer with `RUST_LOG=debug` set saw every test that expects an empty stderr fail: 7 in `rm_test.rs`, all of `list_references_test.rs` and 5 in `check_test.rs`. CI doesn't set it, so only local runs were affected. `rm_test.rs` and `list_references_test.rs` each had the same `pks()` helper, and `list_references_test.rs` and `check_test.rs` each had a schema validator. Both move to `tests/common`, where `pks()` removes `RUST_LOG`; the new syntax error tests use it too. The `check_test.rs` tests run against the fixtures in place rather than through `Fixture`, so those five remove it themselves. With `RUST_LOG=debug`, the suite now fails only on `test_gitignore_negation_patterns`, which depends on the global gitignore rather than on logging. --- tests/check_test.rs | 25 +++++++++------------ tests/common/mod.rs | 35 +++++++++++++++++++++++++++++ tests/expose_monkey_patches_test.rs | 28 +++++++++++------------ tests/list_references_test.rs | 33 ++------------------------- tests/rm_test.rs | 11 ++------- tests/syntax_error_test.rs | 10 ++------- 6 files changed, 66 insertions(+), 76 deletions(-) diff --git a/tests/check_test.rs b/tests/check_test.rs index 5416d5e..33fa4c9 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -1,24 +1,11 @@ use assert_cmd::cargo::cargo_bin_cmd; -use jsonschema::Validator; use predicates::prelude::*; use std::{error::Error, fs}; mod common; fn validate_check_output_schema(json_value: &serde_json::Value) { - let schema_str = fs::read_to_string("schema/check-output.json") - .expect("Failed to read schema file"); - let schema: serde_json::Value = - serde_json::from_str(&schema_str).expect("Schema should be valid JSON"); - let validator = - Validator::new(&schema).expect("Schema should be valid JSON Schema"); - if !validator.is_valid(json_value) { - let errors: Vec = validator - .iter_errors(json_value) - .map(|e| format!(" - {}", e)) - .collect(); - panic!("JSON output does not match schema:\n{}", errors.join("\n")); - } + common::validate_schema("schema/check-output.json", json_value); } pub fn stripped_output(output: Vec) -> String { @@ -70,6 +57,8 @@ fn test_check_with_privacy_dependency_error_template_overrides( #[test] fn test_check_with_private_constants() -> Result<(), Box> { let output = cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/privacy_violations_with_private_constants") .arg("check") @@ -735,6 +724,8 @@ fn test_check_contents_does_not_warn_about_existing_file_that_is_not_included( ) -> Result<(), Box> { // package.yml exists but is not in the include globs. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check-contents") @@ -753,6 +744,8 @@ fn test_check_does_not_warn_about_existing_files_that_are_not_included( ) -> Result<(), Box> { // What a hook passing every staged file sends alongside the Ruby files. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -789,6 +782,8 @@ fn test_check_warns_about_other_spelling_of_included_file( fn test_check_does_not_warn_when_every_path_matches_an_included_file( ) -> Result<(), Box> { cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") @@ -983,6 +978,8 @@ fn test_check_ignores_polymorphic_association() -> Result<(), Box> { // `:event` matches the private `::Event` in packs/baz, but a polymorphic // association names no class. An empty stderr means the path matched. cargo_bin_cmd!("pks") + // `RUST_LOG` makes pks log to stderr, which this test expects empty + .env_remove("RUST_LOG") .arg("--project-root") .arg("tests/fixtures/app_with_rails_relationships") .arg("check") diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 1a8d88f..8fcf448 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -1,3 +1,5 @@ +use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; +use jsonschema::Validator; use std::{ fs, path::{Path, PathBuf}, @@ -62,6 +64,39 @@ impl Fixture { } } +/// Runs `pks` against the fixture. `RUST_LOG` would make pks log to stderr, +/// which tests often expect to be empty, so it is removed. +#[allow(dead_code)] +pub fn pks(fixture: &Fixture, args: &[&str]) -> Assert { + cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(fixture.root()) + .args(args) + .assert() +} + +/// Panics, listing every error, unless `output` matches the schema +#[allow(dead_code)] +pub fn validate_schema(schema_path: &str, output: &serde_json::Value) { + let schema = fs::read_to_string(schema_path) + .unwrap_or_else(|e| panic!("Could not read {}: {}", schema_path, e)); + let schema: serde_json::Value = + serde_json::from_str(&schema).expect("Schema should be valid JSON"); + let validator = + Validator::new(&schema).expect("Schema should be valid JSON Schema"); + let errors: Vec = validator + .iter_errors(output) + .map(|e| format!(" - {}", e)) + .collect(); + assert!( + errors.is_empty(), + "JSON output does not match {}:\n{}", + schema_path, + errors.join("\n") + ); +} + fn copy_dir_recursive(from: &Path, to: &Path) -> std::io::Result<()> { fs::create_dir_all(to)?; for entry in fs::read_dir(from)? { diff --git a/tests/expose_monkey_patches_test.rs b/tests/expose_monkey_patches_test.rs index 91782e8..dbe9dce 100644 --- a/tests/expose_monkey_patches_test.rs +++ b/tests/expose_monkey_patches_test.rs @@ -36,20 +36,20 @@ fn test_expose_monkey_patches_warns_only_about_the_apps_syntax_errors( let rubydir = fixture.path("rubydir_stub"); let gemdir = fixture.path("gemdir_stub"); - cargo_bin_cmd!("pks") - .env_remove("RUST_LOG") - .arg("--project-root") - .arg(fixture.root()) - .arg("--experimental-parser") - .arg("expose-monkey-patches") - .arg(format!("--rubydir={}", rubydir.display())) - .arg(format!("--gemdir={}", gemdir.display())) - .assert() - .success() - .stderr( - "Warning: `app/services/broken.rb` has a syntax error at line 3 \ - (unexpected kEND), so pks may have missed references in it.\n", - ); + common::pks( + &fixture, + &[ + "--experimental-parser", + "expose-monkey-patches", + &format!("--rubydir={}", rubydir.display()), + &format!("--gemdir={}", gemdir.display()), + ], + ) + .success() + .stderr( + "Warning: `app/services/broken.rb` has a syntax error at line 3 \ + (unexpected kEND), so pks may have missed references in it.\n", + ); Ok(()) } diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 9069023..1d3c61d 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -1,21 +1,10 @@ -use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; -use jsonschema::Validator; use pretty_assertions::assert_eq; use serde_json::{json, Value}; use std::{error::Error, fs}; mod common; -fn pks(fixture: &common::Fixture, args: &[&str]) -> Assert { - cargo_bin_cmd!("pks") - // `RUST_LOG` makes pks log to stderr, which `list_references` expects - // to be empty - .env_remove("RUST_LOG") - .arg("--project-root") - .arg(fixture.root()) - .args(args) - .assert() -} +use common::pks; /// Runs pks with `args`, which include `list-references`, and returns its /// output after checking it against the schema. @@ -27,28 +16,10 @@ fn list_references(fixture: &common::Fixture, args: &[&str]) -> Value { .stdout .clone(); let output = serde_json::from_slice(&stdout).expect("output is not JSON"); - validate_schema(&output); + common::validate_schema("schema/list-references-output.json", &output); output } -fn validate_schema(output: &Value) { - let schema = fs::read_to_string("schema/list-references-output.json") - .expect("Failed to read schema file"); - let schema: Value = - serde_json::from_str(&schema).expect("Schema should be valid JSON"); - let validator = - Validator::new(&schema).expect("Schema should be valid JSON Schema"); - let errors: Vec = validator - .iter_errors(output) - .map(|e| format!(" - {}", e)) - .collect(); - assert!( - errors.is_empty(), - "JSON output does not match schema:\n{}", - errors.join("\n") - ); -} - // In simple_app, `packs/foo` references `::Bar`, which is private to // `packs/bar` and which it doesn't depend on, and `::Baz`, which it depends on. diff --git a/tests/rm_test.rs b/tests/rm_test.rs index 604e85f..45f3ce7 100644 --- a/tests/rm_test.rs +++ b/tests/rm_test.rs @@ -1,10 +1,11 @@ -use assert_cmd::{assert::Assert, cargo::cargo_bin_cmd}; use predicates::prelude::*; use pretty_assertions::assert_eq; use std::{collections::BTreeMap, error::Error, fs, path::Path}; mod common; +use common::pks; + // In the fixture, `packs/old` is about to be removed and every other pack // still names it: the root and `packs/foo` depend on it, `packs/bar` and // `packs/baz` list it in `visible_to`, `packs/baz` ignores it as a dependency, @@ -12,14 +13,6 @@ mod common; // No code references `Old` any more, so those todo entries are stale, as they // are once the last use of a pack is deleted. -fn pks(fixture: &common::Fixture, args: &[&str]) -> Assert { - cargo_bin_cmd!("pks") - .arg("--project-root") - .arg(fixture.root()) - .args(args) - .assert() -} - fn read(fixture: &common::Fixture, path: &str) -> String { fs::read_to_string(fixture.path(path)) .unwrap_or_else(|e| panic!("Could not read {}: {}", path, e)) diff --git a/tests/syntax_error_test.rs b/tests/syntax_error_test.rs index ed11ae8..a7be7e1 100644 --- a/tests/syntax_error_test.rs +++ b/tests/syntax_error_test.rs @@ -1,4 +1,3 @@ -use assert_cmd::cargo::cargo_bin_cmd; use std::{error::Error, fs}; mod common; @@ -20,13 +19,8 @@ fn check_stderr( fixture: &common::Fixture, args: &[&str], ) -> Result> { - let output = cargo_bin_cmd!("pks") - .env_remove("RUST_LOG") - .arg("--project-root") - .arg(fixture.root()) - .args(args) - .arg("check") - .output()?; + let args = [args, &["check"]].concat(); + let output = common::pks(fixture, &args).get_output().clone(); Ok(String::from_utf8(output.stderr)?) } From 0c5b9f6279ed434f3dc2da90fba1b2cb063b173e Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 13:03:56 -0700 Subject: [PATCH 6/8] Share list-references' reference selection with `rm` `rm`'s search for references into the pack it removes filtered, sorted and deduplicated the references the way `list-references` does, with its own copy of each step. `between_packs` now holds the filter and the sort, and `rm` asks it for the references to one pack from the others, then keeps one copy of each, as `list-references` merges them. It and `identity` move to `reference_extractor`, which both commands already use, rather than one command reaching into the other's module. The order `rm` prints references in is unchanged, since `identity` only adds the defining pack, which is the same for all of them. Nothing tested that `rm` lists a reference once when several of the pack's files define its constant, so a new test does, under the experimental parser. It passes before and after this change and fails without the deduplication. --- src/packs/reference_extractor.rs | 50 +++++++++++++++++++++++++++++++- src/packs/reference_lister.rs | 42 ++++----------------------- src/packs/remover.rs | 42 +++++++++------------------ tests/rm_test.rs | 20 +++++++++++++ 4 files changed, 87 insertions(+), 67 deletions(-) diff --git a/src/packs/reference_extractor.rs b/src/packs/reference_extractor.rs index de969d3..706178e 100644 --- a/src/packs/reference_extractor.rs +++ b/src/packs/reference_extractor.rs @@ -1,6 +1,8 @@ use std::{collections::HashSet, path::PathBuf}; -use rayon::prelude::{IntoParallelRefIterator, ParallelIterator}; +use rayon::prelude::{ + IntoParallelRefIterator, ParallelIterator, ParallelSliceMut, +}; use tracing::debug; use crate::packs::{ @@ -87,3 +89,49 @@ pub(crate) fn get_all_references( references } + +/// The references from one pack to a constant that another pack defines, +/// limited to the given packs and sorted by `identity`, which puts the copies +/// of a reference, one for each file that defines its constant, next to each +/// other. +pub(crate) fn between_packs<'a>( + references: &'a [Reference], + defining_packs: &HashSet<&str>, + referencing_packs: &HashSet<&str>, +) -> Vec<&'a Reference> { + let mut references: Vec<&Reference> = references + .iter() + .filter(|reference| { + let Some(defining) = reference.defining_pack_name.as_deref() else { + return false; // A constant no pack defines, such as a gem's + }; + let referencing = reference.referencing_pack_name.as_str(); + defining != referencing + && selected(defining_packs, defining) + && selected(referencing_packs, referencing) + }) + .collect(); + // The copies give the same output in any order, so an unstable sort will do + references.par_sort_unstable_by(|a, b| identity(a).cmp(&identity(b))); + references +} + +/// No packs given means every pack. +fn selected(packs: &HashSet<&str>, pack: &str) -> bool { + packs.is_empty() || packs.contains(pack) +} + +/// What sets a reference apart from the others. The copies of a reference made +/// for each file that defines its constant share it. So do all of an ERB file's +/// references to one constant, since they are all at line 0. +pub(crate) fn identity( + reference: &Reference, +) -> (&str, usize, usize, &str, Option<&str>) { + ( + &reference.relative_referencing_file, + reference.source_location.line, + reference.source_location.column, + &reference.constant_name, + reference.defining_pack_name.as_deref(), + ) +} diff --git a/src/packs/reference_lister.rs b/src/packs/reference_lister.rs index 1f4de7c..1c9726c 100644 --- a/src/packs/reference_lister.rs +++ b/src/packs/reference_lister.rs @@ -9,13 +9,13 @@ use std::{ path::PathBuf, }; -use rayon::prelude::{ParallelIterator, ParallelSlice, ParallelSliceMut}; +use rayon::prelude::{ParallelIterator, ParallelSlice}; use serde::Serialize; use super::{ - checker::{get_checkers, reference::Reference}, + checker::get_checkers, checker_configuration::CheckerType, - reference_extractor::get_all_references, + reference_extractor::{between_packs, get_all_references, identity}, Configuration, }; @@ -59,22 +59,8 @@ pub(crate) fn list( &owned_files }; let all_references = get_all_references(configuration, files)?; - let mut references: Vec<&Reference> = all_references - .iter() - .filter(|reference| { - let Some(defining) = reference.defining_pack_name.as_deref() else { - return false; // A constant no pack defines, such as a gem's - }; - let referencing = reference.referencing_pack_name.as_str(); - defining != referencing - && selected(&defining_packs, defining) - && selected(&referencing_packs, referencing) - }) - .collect(); - // The order the schema promises. It also puts the copies of a reference - // next to each other, and as they give the same output in any order, an - // unstable sort will do. - references.par_sort_unstable_by(|a, b| identity(a).cmp(&identity(b))); + let references = + between_packs(&all_references, &defining_packs, &referencing_packs); let checkers = get_checkers(configuration); let recorded_violations = &configuration.pack_set.all_violations; @@ -152,21 +138,3 @@ fn files_of( } Ok(files) } - -/// No packs given means every pack. -fn selected(packs: &HashSet<&str>, pack: &str) -> bool { - packs.is_empty() || packs.contains(pack) -} - -/// What sets a reference apart from the others. The copies of a reference made -/// for each file that defines its constant share it. So do all of an ERB file's -/// references to one constant, since they are all at line 0. -fn identity(reference: &Reference) -> (&str, usize, usize, &str, Option<&str>) { - ( - &reference.relative_referencing_file, - reference.source_location.line, - reference.source_location.column, - &reference.constant_name, - reference.defining_pack_name.as_deref(), - ) -} diff --git a/src/packs/remover.rs b/src/packs/remover.rs index 2a68697..8dd0c35 100644 --- a/src/packs/remover.rs +++ b/src/packs/remover.rs @@ -7,7 +7,7 @@ use super::{ get_zeitwerk_constant_resolver, pack::{write_pack_to_disk, Pack}, package_todo, - reference_extractor::get_all_references, + reference_extractor::{between_packs, get_all_references, identity}, Configuration, }; @@ -65,7 +65,9 @@ fn refuse_if_in_use( configuration: &Configuration, pack: &Pack, ) -> anyhow::Result<()> { - let references = references_from_other_packs(configuration, pack)?; + let all_references = + get_all_references(configuration, &configuration.included_files)?; + let references = references_from_other_packs(&all_references, pack); for reference in &references { println!( "{}:{}:{} references {}", @@ -171,34 +173,16 @@ fn nested_packs(configuration: &Configuration, pack: &Pack) -> Vec { nested } -fn references_from_other_packs( - configuration: &Configuration, +fn references_from_other_packs<'a>( + references: &'a [Reference], pack: &Pack, -) -> anyhow::Result> { - let mut references: Vec = - get_all_references(configuration, &configuration.included_files)? - .into_iter() - .filter(|reference| { - reference.referencing_pack_name != pack.name - && reference.defining_pack_name.as_deref() - == Some(pack.name.as_str()) - }) - .collect(); - - references.sort_by(|a, b| location(a).cmp(&location(b))); - // A constant defined in several of the pack's files yields one per file. - references.dedup_by(|a, b| location(a) == location(b)); - - Ok(references) -} - -fn location(reference: &Reference) -> (&str, usize, usize, &str) { - ( - &reference.relative_referencing_file, - reference.source_location.line, - reference.source_location.column, - &reference.constant_name, - ) +) -> Vec<&'a Reference> { + let defining_packs = HashSet::from([pack.name.as_str()]); + between_packs(references, &defining_packs, &HashSet::new()) + // A constant defined in several of the pack's files yields one per file + .chunk_by(|a, b| identity(a) == identity(b)) + .map(|copies| copies[0]) + .collect() } /// Serializes the whole pack, as `add-dependency` does, so the file loses its diff --git a/tests/rm_test.rs b/tests/rm_test.rs index 45f3ce7..61d6768 100644 --- a/tests/rm_test.rs +++ b/tests/rm_test.rs @@ -139,6 +139,26 @@ fn test_rm_refuses_a_pack_other_packs_reference() -> Result<(), Box> Ok(()) } +#[test] +fn test_rm_lists_a_reference_once_when_several_files_define_its_constant( +) -> Result<(), Box> { + let fixture = common::Fixture::new("app_with_obsolete_pack"); + reference_old_from_baz(&fixture); + // The experimental parser resolves `::Old` to each file that defines it + fs::create_dir_all(fixture.path("packs/old/app/models"))?; + fs::write( + fixture.path("packs/old/app/models/old.rb"), + "module Old\n def self.other; end\nend\n", + )?; + + pks(&fixture, &["--experimental-parser", "rm", "packs/old"]) + .code(2) + .stdout("packs/baz/app/services/baz.rb:3:4 references ::Old\n") + .stderr(predicate::str::contains("Found 1 reference")); + + Ok(()) +} + #[test] fn test_rm_force_removes_a_pack_other_packs_reference( ) -> Result<(), Box> { From 773578ac437fdb8037d8dc118279decbb55bf87a Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 13:06:00 -0700 Subject: [PATCH 7/8] Have Dependabot update Rust dependencies and actions monthly #71 needed `rayon`'s requirement raised from 1.7.0 to 1.10.0 by hand, for `par_chunk_by`. Nothing else raises a requirement, so a new API is found missing only when the build breaks. This takes the bundler and github-actions setup from rubyatscale/packs-specification: monthly, all updates in one group per ecosystem, with a 7-day cooldown. Cargo.lock isn't tracked, and Dependabot's default strategy leaves a requirement alone when it already allows the new release, as `1.7.0` allows 1.12. `versioning-strategy: increase` raises the requirement to the newest release instead, so Cargo.toml keeps up with what CI builds. dist generates release.yml and pins its actions in dist-workspace.toml, so the actions update leaves that file out. --- .github/dependabot.yml | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..82377e9 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,28 @@ +version: 2 +updates: + - package-ecosystem: "cargo" + directory: "/" + schedule: + interval: "monthly" + # Cargo.lock isn't tracked, so raise the minimum versions in Cargo.toml + # even when they already allow the new release + versioning-strategy: "increase" + groups: + cargo: + patterns: + - "*" + cooldown: + default-days: 7 + - package-ecosystem: "github-actions" + directory: "/" + # dist generates release.yml and pins its actions in dist-workspace.toml + exclude-paths: + - ".github/workflows/release.yml" + schedule: + interval: "monthly" + groups: + github-actions: + patterns: + - "*" + cooldown: + default-days: 7 From 8637fbd10b92d2568571aed9bf5df53760b67583 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 12 Oct 2026 13:06:52 -0700 Subject: [PATCH 8/8] Bump version to 0.7.0 `pks list-references` is a new command, and pre-1.0 a new feature wants a minor bump. Once this merges, auto-release.yml tags and releases v0.7.0, since Cargo.toml's version has no tag yet. Retitle `## Unreleased` to `## 0.7.0`, the heading dist takes the release notes from. --- CHANGELOG.md | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7709328..aac1686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 0.7.0 ### Features diff --git a/Cargo.toml b/Cargo.toml index 6a4fd7a..892c495 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,7 +2,7 @@ [package] name = "pks" -version = "0.6.0" +version = "0.7.0" edition = "2021" description = "Welcome! Please see https://github.com/rubyatscale/pks for more information!" license = "MIT"