From f0d64f1058f3010f1799c46334957978b0fd3033 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 15:01:24 -0700 Subject: [PATCH 1/5] Name a class written from the top level as Ruby does, with --experimental-parser The experimental parser joined a class or module's name to the namespaces around it with `::`, so `class ::Taco` was defined as `::::Taco`, and `class ::Burrito` inside `module Outer` as `::Outer::::Burrito`. The constants assigned in them got the same prefix, as `::::Taco::FILLING`. No reference resolves to a name with `::::`, so references to these constants were missed. The experimental resolver built its guesses the same way, so `Shell` written inside `class ::Taco` was looked up as `::::Taco::Shell` and then `::Shell`, and a `::Taco::Shell` that another pack defines was missed too. `nest_constant_name` starts the name again from the top level at a namespace that begins with `::`, as Ruby does. The experimental parser names its definitions with it, and the experimental resolver makes its guesses with it, so inside `module Outer; class ::Foo`, `Bar` is `::Foo::Bar`, then `::Outer::Bar`, then `::Bar`, the order of Ruby's `Module.nesting`. The packwerk parser and the Zeitwerk resolver keep `combine_namespace_with_constant_name` and `get_definition_from`, which match packwerk. packwerk names `class ::Taco` `::::Taco` among a file's local definitions too, and its constant_resolver looks `Shell` up as `::Taco::Shell` against names that have no leading `::`, so it misses the same references. The CHANGELOG says so. The cache key includes pks's version, so a release doesn't serve the old names from the cache. --- CHANGELOG.md | 13 +++++ src/packs/parsing/ruby/experimental.rs | 24 +++++++++ .../ruby/experimental/constant_resolver.rs | 39 +++++++++++++-- src/packs/parsing/ruby/experimental/parser.rs | 49 +++++++++---------- .../parsing/ruby/namespace_calculator.rs | 17 +++++++ tests/list_references_test.rs | 28 +++++++++++ 6 files changed, 140 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d673253b..2086026b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,19 @@ into a symlinked directory, so it no longer finds the files or packs in one. A link to a directory that contains it, such as `loop -> ..`, made pks list the same files and packs over and over, and no longer does. +### Fixes + +#### `--experimental-parser` names a class written from the top level + +With `--experimental-parser`, pks named a class or module written from the top +level, as `class ::Foo`, `::::Foo`, or `::Outer::::Foo` inside `module Outer`, +and gave the constants defined in it the same prefix. References to them were +missed, and so were references written inside one to the constants nested in +it. pks now names it `::Foo`, as Ruby does, so it may find new references and +violations. Without `--experimental-parser`, pks still looks up a constant +written inside `class ::Foo` as packwerk does, which misses the constants +nested in `::Foo` too. + ## 0.7.0 ### Features diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index ca99f0fb..611e298d 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -416,6 +416,30 @@ end } } + #[test] + fn definitions_from_the_top_level() { + // A namespace or constant written from the top level doesn't nest in + // the ones around it + for (contents, definitions) in [ + ("class ::Foo; def a; end; end", &["::Foo"][..]), + ( + "module Outer; class ::Foo; def a; end; end; end", + &["::Foo"], + ), + ( + "module Outer; class ::Foo::Bar; def a; end; end; end", + &["::Foo::Bar"], + ), + ( + "module Outer; module ::Foo; BAR = 1; end; end", + &["::Foo::BAR"], + ), + ("module Outer; ::BAR = 1; end", &["::BAR"]), + ] { + assert_eq!(definition_names(contents), definitions, "{}", contents); + } + } + #[test] fn class_definition_with_private_constant() { let contents: String = String::from( diff --git a/src/packs/parsing/ruby/experimental/constant_resolver.rs b/src/packs/parsing/ruby/experimental/constant_resolver.rs index 31632f42..f894c5d3 100644 --- a/src/packs/parsing/ruby/experimental/constant_resolver.rs +++ b/src/packs/parsing/ruby/experimental/constant_resolver.rs @@ -7,7 +7,7 @@ use std::{ use crate::packs::{ constant_resolver::{ConstantDefinition, ConstantResolver}, - parsing::ruby::namespace_calculator::combine_namespace_with_constant_name, + parsing::ruby::namespace_calculator::nest_constant_name, }; #[derive(Default, Debug)] @@ -133,10 +133,8 @@ impl ExperimentalConstantResolver { const_name: &'a str, current_namespace_path: &'a [&str], ) -> Vec { - let fully_qualified_name_guess = combine_namespace_with_constant_name( - current_namespace_path, - const_name, - ); + let fully_qualified_name_guess = + nest_constant_name(current_namespace_path, const_name); let definitions = self.constant_for_fully_qualified_name(&fully_qualified_name_guess); @@ -172,3 +170,34 @@ impl ExperimentalConstantResolver { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_in_a_namespace_from_the_top_level() { + let constants = + ["::Foo::Bar", "::Outer::Baz"].map(|name| ConstantDefinition { + fully_qualified_name: name.to_owned(), + absolute_path_of_definition: PathBuf::from("/app/foo.rb"), + }); + let resolver = ExperimentalConstantResolver::create( + constants.to_vec(), + Path::new("/app"), + &HashMap::new(), + ); + + // In `module Outer; class ::Foo; end; end`, as Ruby looks them up + for (name, resolved) in [("Bar", "::Foo::Bar"), ("Baz", "::Outer::Baz")] + { + let definitions = resolver.resolve(name, &["Outer", "::Foo"]); + let names: Vec = definitions + .unwrap_or_default() + .into_iter() + .map(|definition| definition.fully_qualified_name) + .collect(); + assert_eq!(names, [resolved], "{}", name); + } + } +} diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index 5ab0be42..e9aa4af1 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -1,14 +1,16 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ parsing::{ - ruby::parse_utils::{ - constant_name, fetch_const_name, fetch_const_path_name, - first_syntax_error, get_constant_assignment_definition, - get_definition_from, get_reference_from_active_record_association, - is_plain_call, loc_to_range, visit_constant_assignments, - ParseError, + ruby::{ + namespace_calculator::nest_constant_name, + parse_utils::{ + constant_name, fetch_const_name, fetch_const_path_name, + first_syntax_error, + get_reference_from_active_record_association, is_plain_call, + loc_to_range, visit_constant_assignments, ParseError, + }, }, - ParsedDefinition, UnresolvedReference, + ParsedDefinition, Range, UnresolvedReference, }, Configuration, ProcessedFile, }; @@ -35,18 +37,23 @@ impl ReferenceCollector<'_> { name: Result, location: &Location, ) { - let definition = get_constant_assignment_definition( - name, - location, - self.current_namespaces.to_owned(), - &self.line_col_lookup, - ); - - if let Some(definition) = definition { + if let Ok(name) = name { + let location = loc_to_range(location, &self.line_col_lookup); + let definition = self.definition(&name, location); self.definitions.push(definition); } } + /// `name`, as `Foo` or `::Foo::Bar`, defined in the current namespaces + fn definition(&self, name: &str, location: Range) -> ParsedDefinition { + let namespaces: Vec<&str> = + self.current_namespaces.iter().map(String::as_str).collect(); + ParsedDefinition { + fully_qualified_name: nest_constant_name(&namespaces, name), + location, + } + } + fn reference_constant( &mut self, name: Result, @@ -97,11 +104,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &self.line_col_lookup, ); - let definition = get_definition_from( - &namespace, - &self.current_namespaces, - &location, - ); + let definition = self.definition(&namespace, location); // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else @@ -181,11 +184,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &self.line_col_lookup, ); - let definition = get_definition_from( - &namespace, - &self.current_namespaces, - &location, - ); + let definition = self.definition(&namespace, location); // Note – is there a way to use lifetime specifiers to get rid of this and // just keep current namespaces as a vector of string references or something else diff --git a/src/packs/parsing/ruby/namespace_calculator.rs b/src/packs/parsing/ruby/namespace_calculator.rs index d3773964..2d1e2d59 100644 --- a/src/packs/parsing/ruby/namespace_calculator.rs +++ b/src/packs/parsing/ruby/namespace_calculator.rs @@ -45,6 +45,23 @@ pub fn combine_namespace_with_constant_name( format!("::{}", fully_qualified_name_vec.join("::")) } +/// The fully qualified name of `const_name` in `namespace_path`, as Ruby gives +/// it: a namespace written from the top level, as `class ::Foo`, doesn't nest +/// in the ones around it. `combine_namespace_with_constant_name` nests it +/// anyway, giving `::::Foo`, as packwerk does. +pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String { + namespace_path.iter().copied().chain([const_name]).fold( + String::new(), + |name, part| { + if part.starts_with("::") { + part.to_owned() + } else { + format!("{}::{}", name, part) + } + }, + ) +} + pub fn possible_fully_qualified_constants( namespace_path: &[&str], const_name: &str, diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 3b33403b..ea23ac47 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -362,6 +362,34 @@ enforcement_globs_ignore: Ok(()) } +#[test] +fn test_list_references_to_a_class_defined_from_the_top_level( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // The experimental parser names this `::Taco`, not `::Outer::Taco` + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "module Outer\n class ::Taco\n def self.make; end\n end\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + fs::write(fixture.path(file), "Taco.make\n")?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(file, Some((1, 0)), "::Taco"), + ] + }) + ); + + Ok(()) +} + /// A reference from `packs/foo` to a constant that `packs/bar` keeps private, /// which breaks the same two rules as `bar_from_foo`. `None` is a position in /// ERB. From a5d3d285dba8a3dd9113c9416e40806f88fe3331 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 15:06:38 -0700 Subject: [PATCH 2/5] Drop two comments that restate the code The test cases and `nest_constant_name` already say what they did. --- src/packs/parsing/ruby/experimental.rs | 2 -- src/packs/parsing/ruby/experimental/parser.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index 611e298d..fc676127 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -418,8 +418,6 @@ end #[test] fn definitions_from_the_top_level() { - // A namespace or constant written from the top level doesn't nest in - // the ones around it for (contents, definitions) in [ ("class ::Foo; def a; end; end", &["::Foo"][..]), ( diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index e9aa4af1..e52abac5 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -44,7 +44,6 @@ impl ReferenceCollector<'_> { } } - /// `name`, as `Foo` or `::Foo::Bar`, defined in the current namespaces fn definition(&self, name: &str, location: Range) -> ParsedDefinition { let namespaces: Vec<&str> = self.current_namespaces.iter().map(String::as_str).collect(); From b89106e83826f88955e0bf34137466b5b3b103ca Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 15:27:15 -0700 Subject: [PATCH 3/5] Say which references the `::::` names missed, and that todos go stale The first commit says that no reference resolves to a name with `::::`. One written inside another `class ::Taco`, as the definition is, did. The resolver built its guess the same way, so `FILLING` there resolved to `::::Taco::FILLING`. The rest were missed. That reference is now named `::Taco::FILLING`, so a violation that a package_todo.yml records under the `::::` name is reported again under the new name, and the old entry as stale, until `pks update` is run. The CHANGELOG now says so. --- CHANGELOG.md | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2086026b..6d27f257 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,12 +46,16 @@ same files and packs over and over, and no longer does. With `--experimental-parser`, pks named a class or module written from the top level, as `class ::Foo`, `::::Foo`, or `::Outer::::Foo` inside `module Outer`, -and gave the constants defined in it the same prefix. References to them were -missed, and so were references written inside one to the constants nested in -it. pks now names it `::Foo`, as Ruby does, so it may find new references and -violations. Without `--experimental-parser`, pks still looks up a constant -written inside `class ::Foo` as packwerk does, which misses the constants -nested in `::Foo` too. +and gave the constants defined in it the same prefix. A reference to one was +found only from inside another `class ::Foo` written the same way, and then +under that name, as `::::Foo::BAR`. Others were missed, as was a reference +written inside `class ::Foo` to a constant that `class Foo::Bar` defines. pks +now names them as Ruby does, `::Foo` and `::Foo::BAR`, so it may find new +references and violations. A violation that a `package_todo.yml` records under +a `::::` name is reported again under the new name, and the old entry as stale, +until you run `pks update`. Without `--experimental-parser`, pks still looks up +a constant written inside `class ::Foo` as packwerk does, which misses the +constants nested in `::Foo` too. ## 0.7.0 From 7b38103ff62d79eb652c67a9e05caee665dbd700 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 15:38:18 -0700 Subject: [PATCH 4/5] Test a module and a lookup inside a class from the top level No test caught the experimental parser naming `module ::Foo` the old way, or `nest_constant_name` starting again at the first namespace written from the top level in place of the last, and the integration test didn't reach the resolver. `definitions_from_the_top_level` now has a `module ::Foo`, and nestings with a namespace from the top level in the middle, as `module A; class ::B; module C`. A new integration test looks up `Shell` inside `module Outer; class ::Taco`, and finds `::Taco::Shell` in another pack. `nest_constant_name` builds the name in one String with a loop, in place of a fold that made a String for each part. Its doc comment says that a constant assigned from the top level starts again too, and the resolver's comment on its lookup order covers a namespace from the top level. --- src/packs/parsing/ruby/experimental.rs | 12 +++++++ .../ruby/experimental/constant_resolver.rs | 6 +++- .../parsing/ruby/namespace_calculator.rs | 29 ++++++++-------- tests/list_references_test.rs | 33 ++++++++++++++++++- 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index fc676127..2bc95bfd 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -433,6 +433,18 @@ end &["::Foo::BAR"], ), ("module Outer; ::BAR = 1; end", &["::BAR"]), + ( + "module Outer; module ::Foo; def a; end; end; end", + &["::Foo"], + ), + ( + "module A; class ::B; module C; BAR = 1; end; end; end", + &["::B::C::BAR"], + ), + ( + "module A; module ::B; module ::C; BAR = 1; end; end; end", + &["::C::BAR"], + ), ] { assert_eq!(definition_names(contents), definitions, "{}", contents); } diff --git a/src/packs/parsing/ruby/experimental/constant_resolver.rs b/src/packs/parsing/ruby/experimental/constant_resolver.rs index f894c5d3..46ca51a3 100644 --- a/src/packs/parsing/ruby/experimental/constant_resolver.rs +++ b/src/packs/parsing/ruby/experimental/constant_resolver.rs @@ -126,8 +126,12 @@ impl ExperimentalConstantResolver { // ::Foo::Boo // ::Boo // + // A namespace written from the top level starts the name again, so in + // `module Foo; class ::Bar`, the path is ['Foo', '::Bar'], and `Boo` could + // be ::Bar::Boo, ::Foo::Boo or ::Boo, the order of `Module.nesting`. + // // We need to check each of these possibilities in order, and return the first one that exists - // If none of them exist, return None + // If none of them exist, return no definitions fn resolve_traversing_namespace_path<'a>( &'a self, const_name: &'a str, diff --git a/src/packs/parsing/ruby/namespace_calculator.rs b/src/packs/parsing/ruby/namespace_calculator.rs index 2d1e2d59..e2650c9a 100644 --- a/src/packs/parsing/ruby/namespace_calculator.rs +++ b/src/packs/parsing/ruby/namespace_calculator.rs @@ -45,21 +45,22 @@ pub fn combine_namespace_with_constant_name( format!("::{}", fully_qualified_name_vec.join("::")) } -/// The fully qualified name of `const_name` in `namespace_path`, as Ruby gives -/// it: a namespace written from the top level, as `class ::Foo`, doesn't nest -/// in the ones around it. `combine_namespace_with_constant_name` nests it -/// anyway, giving `::::Foo`, as packwerk does. +/// The fully qualified name of `const_name` in `namespace_path`, as Ruby names +/// it. A name written from the top level, as `class ::Foo` or `::BAR = 1`, +/// doesn't nest in the namespaces around it. packwerk nests it anyway, naming +/// `class ::Foo` `::::Foo`, or `::Outer::::Foo` inside `module Outer`, and so +/// do the packwerk parser and the Zeitwerk resolver, which match it. pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String { - namespace_path.iter().copied().chain([const_name]).fold( - String::new(), - |name, part| { - if part.starts_with("::") { - part.to_owned() - } else { - format!("{}::{}", name, part) - } - }, - ) + let mut name = String::new(); + for part in namespace_path.iter().copied().chain([const_name]) { + if part.starts_with("::") { + name.clear(); + } else { + name.push_str("::"); + } + name.push_str(part); + } + name } pub fn possible_fully_qualified_constants( diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index ea23ac47..0d67814f 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -366,7 +366,7 @@ enforcement_globs_ignore: fn test_list_references_to_a_class_defined_from_the_top_level( ) -> Result<(), Box> { let fixture = common::Fixture::new("simple_app"); - // The experimental parser names this `::Taco`, not `::Outer::Taco` + // The experimental parser names this `::Taco`, not `::Outer::::Taco` fs::write( fixture.path("packs/bar/app/services/taco.rb"), "module Outer\n class ::Taco\n def self.make; end\n end\nend\n", @@ -390,6 +390,37 @@ fn test_list_references_to_a_class_defined_from_the_top_level( Ok(()) } +#[test] +fn test_list_references_inside_a_class_defined_from_the_top_level( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "module Taco\n class Shell\n def self.fold; end\n end\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + // As in Ruby, `Shell` here is looked up in `::Taco`, not `::Outer::::Taco` + fs::write( + fixture.path(file), + "module Outer\n class ::Taco\n Shell.fold\n end\nend\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(file, Some((3, 4)), "::Taco::Shell"), + ] + }) + ); + + Ok(()) +} + /// A reference from `packs/foo` to a constant that `packs/bar` keeps private, /// which breaks the same two rules as `bar_from_foo`. `None` is a position in /// ERB. From f0b5fb9000c5ac7e845e03205c85b7dfa92de693 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 15:38:38 -0700 Subject: [PATCH 5/5] Say that a class reopened from the top level counts as defined With `class ::String` inside `module CoreExt` now named `::String`, `expose-monkey-patches` lists it, and a reference to `String` from another pack is a reference to the pack that reopens it, as it is for a `class String` at the top level of a file. The CHANGELOG says so, and that `::BAR = 1` inside `module Outer` was `::Outer::::BAR`. Without `--experimental-parser`, a constant written inside `class ::Foo` isn't only missed: packwerk, and pks with it, can take `Bar` there for a top-level `::Bar`. The CHANGELOG says that in place of "misses". --- CHANGELOG.md | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d27f257..85c5724e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,18 +44,27 @@ same files and packs over and over, and no longer does. #### `--experimental-parser` names a class written from the top level -With `--experimental-parser`, pks named a class or module written from the top -level, as `class ::Foo`, `::::Foo`, or `::Outer::::Foo` inside `module Outer`, -and gave the constants defined in it the same prefix. A reference to one was -found only from inside another `class ::Foo` written the same way, and then -under that name, as `::::Foo::BAR`. Others were missed, as was a reference -written inside `class ::Foo` to a constant that `class Foo::Bar` defines. pks -now names them as Ruby does, `::Foo` and `::Foo::BAR`, so it may find new -references and violations. A violation that a `package_todo.yml` records under -a `::::` name is reported again under the new name, and the old entry as stale, -until you run `pks update`. Without `--experimental-parser`, pks still looks up -a constant written inside `class ::Foo` as packwerk does, which misses the -constants nested in `::Foo` too. +With `--experimental-parser`, pks gave a class or module written from the top +level, as `class ::Foo`, the name `::::Foo`, or `::Outer::::Foo` inside +`module Outer`, and gave the constants defined in it the same prefix. A +constant assigned from the top level, as `::BAR = 1` inside `module Outer`, +was `::Outer::::BAR`. A reference to one was found only from inside another +`class ::Foo` written the same way, and then under that name, as +`::::Foo::BAR`. Others were missed, as was a reference written inside +`class ::Foo` to a constant that `class Foo::Bar` defines. pks now names them +as Ruby does, `::Foo` and `::Foo::BAR`, so it may find new references and +violations. A violation that a `package_todo.yml` records under a `::::` name +is reported again under the new name, and the old entry as stale, until you +run `pks update`. + +A class reopened this way, as `class ::String` inside `module CoreExt`, now +counts as defined in its pack, as a `class String` at the top level of a file +does. So `expose-monkey-patches` lists it, and a reference to `String` from +another pack is a reference to that pack. + +Without `--experimental-parser`, pks still looks up a constant written inside +`class ::Foo` as packwerk does, so `Bar` written there is never `::Foo::Bar`. +It's missed, or taken for a top-level `::Bar`. ## 0.7.0