From c0811048f90bfe2eaf4f84feed933764aab03571 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:08:54 -0700 Subject: [PATCH 1/9] Take no parent for `Outer::`, as packwerk does packwerk names `class ::Burrito` inside `module Outer` `::Outer::::Burrito`, and so does pks. When no file defines a constant, both look for it in the file of its parent, which for `Outer::::Burrito` is `Outer::`. packwerk takes the parent with Ruby's `String#split`, which drops the empty name at the end, so `Outer::` has no parent, and packwerk finds no constant. Rust's `split` keeps it, so pks went on to `Outer`, and reported `::Outer::::Burrito` against the pack that defines `Outer`. In `module Aa; module Bb; class ::Cee`, packwerk goes from `Aa::Bb::` to `Aa`, where pks stopped at `Aa::Bb`. The Zeitwerk resolver now drops the empty names at the end too. --- .../ruby/zeitwerk/constant_resolver.rs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs index 6afaeac8..c7d088f7 100644 --- a/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs +++ b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs @@ -111,7 +111,13 @@ impl ZeitwerkConstantResolver { // end // Therefore, we take the given const_name, remove the last part of the fully qualified name, and try again. // In this case, we'd try to resolve `::Foo::Bar` instead of `::Foo::Bar::Boo` - let split_const = const_name.split("::").collect::>(); + let mut split_const = + const_name.split("::").collect::>(); + // As in packwerk, whose `String#split` drops the empty names + // at the end, so `Outer::::Foo`'s parent `Outer::` has none + while split_const.last() == Some(&"") { + split_const.pop(); + } if split_const.len() <= 1 { return None; } @@ -209,3 +215,36 @@ impl ZeitwerkConstantResolver { None } } + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use super::*; + + #[test] + fn resolve_a_name_from_the_top_level_in_a_namespace() { + let resolver = ZeitwerkConstantResolver::create( + [ + ("::Outer", "/app/outer.rb"), + ("::Aa", "/app/aa.rb"), + ("::Aa::Bb", "/app/aa/bb.rb"), + ] + .map(|(name, path)| ConstantDefinition { + fully_qualified_name: name.to_owned(), + absolute_path_of_definition: PathBuf::from(path), + }) + .to_vec(), + ); + + // As in packwerk, `Outer::` has no parent, and `Aa::Bb::`'s is `Aa` + assert_eq!(resolver.resolve("::Outer::::Burrito", &[]), None); + assert_eq!( + resolver.resolve("::Aa::Bb::::Cee", &[]), + Some(vec![ConstantDefinition { + fully_qualified_name: "::Aa::Bb::::Cee".to_owned(), + absolute_path_of_definition: PathBuf::from("/app/aa.rb"), + }]) + ); + } +} From bc0ad33b490ffecdf9a57220c7dc8ddb3368d722 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:09:17 -0700 Subject: [PATCH 2/9] Count a constant after `class ::Taco` as packwerk does packwerk names `class ::Taco` at the top level of a file `::::Taco` among the file's own definitions, so a `Taco` or `::Taco` written later in the file, which it looks for as `::Taco`, isn't one of them, and is a reference. pks looked for `::` and the name, `::::Taco`, as well, so it took them for the file's own and dropped them. The packwerk parser now looks for the name alone. Only a definition written from the top level, at the top level of a file, has a name that starts with `::::`, so nothing else changes. --- src/packs/parsing/ruby/packwerk/parser.rs | 6 ++--- tests/list_references_test.rs | 31 +++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 4779339c..00c8df4b 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -318,10 +318,8 @@ fn process( let possible_constants = possible_fully_qualified_constants(&namespace_path, &r.name); for constant_name in possible_constants { - if let Some(location) = definition_to_location_map - .get(&constant_name) - .or(definition_to_location_map - .get(&format!("::{}", constant_name))) + if let Some(location) = + definition_to_location_map.get(&constant_name) { let reference_is_definition = location.start_row == r.location.start_row diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 7a1b8954..77a1946a 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -421,6 +421,37 @@ fn test_list_references_inside_a_class_defined_from_the_top_level( Ok(()) } +#[test] +fn test_list_references_after_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"), + "class Taco\n def self.make; end\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + // As in packwerk, which names this class `::::Taco`, the `Taco` and + // `::Taco` after it aren't the file's own + fs::write( + fixture.path(file), + "class ::Taco\n def self.local; end\nend\nTaco.make\n::Taco.make\n", + )?; + + assert_eq!( + list_references(&fixture, &["list-references"]), + json!({ + "references": [ + bar_from_foo(), + baz_from_foo(), + to_bar_pack(file, Some((4, 0)), "::Taco"), + to_bar_pack(file, Some((5, 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 a81a24794e538b87911c80931d60d87cc6476ca9 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:09:36 -0700 Subject: [PATCH 3/9] Look up an association from its model's name, with --experimental-parser Rails looks up the class of `has_many :tacos` from the name of the model that declares it: in `Outer::Foo`, as `Outer::Foo::Taco`, `Outer::Taco`, then `Taco`. The experimental parser gave the reference the namespaces it's written in, so in `class Outer::Foo` pks looked for `::Outer::Foo::Taco`, then `::Taco`, and in `module Outer; class ::Foo` for `::Foo::Taco`, `::Outer::Taco`, then `::Taco`. The reference now has the namespaces of the model's name, `["Outer", "Foo"]` and `["Foo"]`, so the resolver looks where Rails does. Without `--experimental-parser`, pks looks it up as packwerk does, as before. --- src/packs/parsing/ruby/experimental/parser.rs | 20 +++++++++- tests/list_references_test.rs | 40 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index e52abac5..c20b252a 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -53,6 +53,23 @@ impl ReferenceCollector<'_> { } } + /// Rails looks up an association's class from its model's name, so these + /// are that name's: `["Foo"]` in `module Outer; class ::Foo`, and + /// `["Outer", "Foo"]` in `class Outer::Foo`. + fn model_namespaces(&self) -> Vec { + let Some((model, namespaces)) = self.current_namespaces.split_last() + else { + return vec![]; + }; + let namespaces: Vec<&str> = + namespaces.iter().map(String::as_str).collect(); + nest_constant_name(&namespaces, model) + .split("::") + .skip(1) + .map(str::to_owned) + .collect() + } + fn reference_constant( &mut self, name: Result, @@ -143,7 +160,8 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &self.custom_associations, ); - if let Some(association_reference) = association_reference { + if let Some(mut association_reference) = association_reference { + association_reference.namespace_path = self.model_namespaces(); self.references.push(association_reference); } } diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 77a1946a..2aa259d0 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -452,6 +452,46 @@ fn test_list_references_after_a_class_defined_from_the_top_level( Ok(()) } +#[test] +fn test_list_references_from_associations_by_the_models_name( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "class Taco\n def self.make; end\nend\n\ + module Outer\n class Taco\n def self.make; end\n end\nend\n", + )?; + // Rails looks `:tacos` up from the model's name, as `Outer::Foo::Taco`, + // `Outer::Taco`, then `Taco` from `Outer::Foo`, and as `Foo::Taco`, then + // `Taco` from `Foo`, which `class ::Foo` names whatever surrounds it + let compact = "packs/foo/app/services/foo_compact.rb"; + fs::write( + fixture.path(compact), + "class Outer::Foo\n has_many :tacos\nend\n", + )?; + let top_level = "packs/foo/app/services/foo_top_level.rb"; + fs::write( + fixture.path(top_level), + "module Outer\n class ::Foo\n has_many :tacos\n end\nend\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(compact, Some((2, 2)), "::Outer::Taco"), + to_bar_pack(top_level, Some((3, 4)), "::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 e9a60929c6c8e4dae3347b460899ee4bc178d07f Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:14:57 -0700 Subject: [PATCH 4/9] Name a constant written through `Object` as Ruby does, with --experimental-parser The constants of `Object` are the top level's, so `class Object::Bar` inside `module CoreExt` defines `::Bar`, as does `class Bar` inside a `class Object` at the top level of a file. The experimental parser named them `::CoreExt::Object::Bar` and `::Object::Bar`. `nest_constant_name` now drops a leading `::Object` from a name, and the experimental parser takes a class, module or constant defined as `Object::Bar` for `::Bar`. So both are named `::Bar`, and the experimental resolver finds `::Bar` for a reference written `::Object::Bar`. For a reference written `Object::Bar`, the resolver looks `Object` up as Ruby does, so inside `module CoreExt` it tries `::CoreExt::Object::Bar` before `::Bar`. The parser can't look it up for a definition, so it takes it that no namespace defines an `Object` of its own. A `class Object` inside a namespace is still the namespace's own, as Ruby makes it. --- src/packs/parsing/ruby/experimental.rs | 26 +++++++ .../ruby/experimental/constant_resolver.rs | 75 +++++++++++++++---- src/packs/parsing/ruby/experimental/parser.rs | 20 ++++- .../parsing/ruby/namespace_calculator.rs | 6 ++ 4 files changed, 107 insertions(+), 20 deletions(-) diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index 2bc95bfd..bd3d14f5 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -450,6 +450,32 @@ end } } + #[test] + fn definitions_in_object() { + for (contents, definitions) in [ + ( + "module CoreExt; class Object::Bar; def a; end; end; end", + &["::Bar"][..], + ), + ("class Object; class Bar; def a; end; end; end", &["::Bar"]), + ( + "module CoreExt; Object::BAR = 1; ::Object::BAZ = 1; end", + &["::BAR", "::BAZ"], + ), + ( + "module CoreExt; class ::Object; BAR = 1; end; end", + &["::BAR"], + ), + // A class written `Object` in a namespace is the namespace's own + ( + "module CoreExt; class Object; BAR = 1; end; end", + &["::CoreExt::Object::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 46ca51a3..1ef3d0df 100644 --- a/src/packs/parsing/ruby/experimental/constant_resolver.rs +++ b/src/packs/parsing/ruby/experimental/constant_resolver.rs @@ -179,29 +179,72 @@ impl ExperimentalConstantResolver { 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(), + fn resolved_names( + constants: &[&str], + name: &str, + namespace_path: &[&str], + ) -> Vec { + let constants = constants + .iter() + .map(|constant| ConstantDefinition { + fully_qualified_name: (*constant).to_owned(), absolute_path_of_definition: PathBuf::from("/app/foo.rb"), - }); - let resolver = ExperimentalConstantResolver::create( - constants.to_vec(), + }) + .collect(); + ExperimentalConstantResolver::create( + constants, Path::new("/app"), &HashMap::new(), - ); + ) + .resolve(name, namespace_path) + .unwrap_or_default() + .into_iter() + .map(|definition| definition.fully_qualified_name) + .collect() + } + #[test] + fn resolve_in_a_namespace_from_the_top_level() { // 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); + assert_eq!( + resolved_names( + &["::Foo::Bar", "::Outer::Baz"], + name, + &["Outer", "::Foo"] + ), + [resolved], + "{}", + name + ); + } + } + + #[test] + fn resolve_through_object() { + for (constants, name, resolved) in [ + (&["::Bar"][..], "Object::Bar", "::Bar"), + (&["::Bar"], "::Object::Bar", "::Bar"), + // `Object` is looked up as Ruby looks up any constant + ( + &["::Bar", "::CoreExt::Object::Bar"], + "Object::Bar", + "::CoreExt::Object::Bar", + ), + ( + &["::Bar", "::CoreExt::Object::Bar"], + "::Object::Bar", + "::Bar", + ), + ] { + assert_eq!( + resolved_names(constants, name, &["CoreExt"]), + [resolved], + "{} among {:?}", + name, + constants + ); } } } diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index c20b252a..dfcedba6 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -39,7 +39,7 @@ impl ReferenceCollector<'_> { ) { if let Ok(name) = name { let location = loc_to_range(location, &self.line_col_lookup); - let definition = self.definition(&name, location); + let definition = self.definition(&defined_name(name), location); self.definitions.push(definition); } } @@ -110,7 +110,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { return; } - let namespace = namespace_result.unwrap(); + let namespace = defined_name(namespace_result.unwrap()); if let Some(inner) = node.superclass() { self.visit(&inner); @@ -194,8 +194,9 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { visit_constant_assignments!(); fn visit_module_node(&mut self, node: &ModuleNode<'pr>) { - let namespace = - fetch_const_name(&node.constant_path()).unwrap_or("".to_owned()); + let namespace = defined_name( + fetch_const_name(&node.constant_path()).unwrap_or("".to_owned()), + ); let location = loc_to_range( &node.constant_path().location(), &self.line_col_lookup, @@ -250,6 +251,17 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } } +/// The name of a class, module or constant as defined, with `Object::Foo` taken +/// for `::Foo`. Ruby looks `Object` up as it does any constant, so a namespace +/// could define an `Object` of its own, but pks can't look it up here, and takes +/// it that none does. +fn defined_name(name: String) -> String { + match name.strip_prefix("Object::") { + Some(name) => format!("::{}", name), + None => name, + } +} + pub(crate) fn process_from_path( path: &Path, configuration: &Configuration, diff --git a/src/packs/parsing/ruby/namespace_calculator.rs b/src/packs/parsing/ruby/namespace_calculator.rs index e2650c9a..8d4f0c20 100644 --- a/src/packs/parsing/ruby/namespace_calculator.rs +++ b/src/packs/parsing/ruby/namespace_calculator.rs @@ -50,6 +50,9 @@ pub fn combine_namespace_with_constant_name( /// 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. +/// +/// The constants of `Object` are the top level's, so `::Object::Foo` is +/// `::Foo`, as is `Foo` inside a `class Object` at the top level. pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String { let mut name = String::new(); for part in namespace_path.iter().copied().chain([const_name]) { @@ -60,6 +63,9 @@ pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String { } name.push_str(part); } + while name.starts_with("::Object::") { + name.replace_range(.."::Object".len(), ""); + } name } From 410937e45c6e12c300a48a7b60cb237f1be59819 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 16:15:18 -0700 Subject: [PATCH 5/9] Say what the follow-ups to #82 change in the CHANGELOG The association lookup and the names written through `Object` change what `--experimental-parser` finds. Without it, pks now matches packwerk around a name written from the top level, so `pks check` may report a violation it didn't, or no longer report one, and leave a todo stale. --- CHANGELOG.md | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85c5724e..989c7349 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,45 @@ 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`. +#### `--experimental-parser` looks up an association from its model's name + +With `--experimental-parser`, pks looked up the class of an association, as +`has_many :tacos`, as Ruby looks up a constant written where it's declared. +Rails looks it up from the model's name: in `class Outer::Foo`, as +`Outer::Foo::Taco`, `Outer::Taco`, then `Taco`, and in `class ::Foo` inside +`module Outer`, as `Foo::Taco`, then `Taco`. pks now does too, so in +`class Outer::Foo` it finds `Outer::Taco` where it found `Taco`, and in +`class ::Foo` it no longer takes `Outer::Taco` for the class. + +#### `--experimental-parser` names a constant written through `Object` + +The constants of `Object` are the top level's, so `class Object::Bar` inside +`module CoreExt` defines `::Bar`, as does `class Bar` inside a `class Object` +at the top level of a file. With `--experimental-parser`, pks named these +`::CoreExt::Object::Bar` and `::Object::Bar`. It now names them `::Bar`, +taking it that no namespace defines an `Object` of its own. A reference +written `::Object::Bar`, or `Object::Bar` where no namespace around it defines +an `Object`, now resolves to `::Bar` too. A `class Object` inside a namespace +is still the namespace's own, as in Ruby. + +#### pks finds what packwerk finds around a name written from the top level + +Without `--experimental-parser`, pks differed from packwerk in two cases, and +now matches it: + +- packwerk names `class ::Burrito` inside `module Outer` `::Outer::::Burrito`, + and finds no constant for it. pks took it for a reference to `Outer`, in the + pack that defines `Outer`. And where packwerk takes `::Aa::Bb::::Cee`, in + `module Aa; module Bb; class ::Cee`, for a reference to `Aa`, pks took it for + one to `Aa::Bb`. +- After `class ::Taco` or `::Taco = 1` at the top level of a file, packwerk + counts a `Taco` or `::Taco` written later in the file as a reference, since + it names the definition `::::Taco`. pks took it for the file's own. + +So `pks check` may report a violation it didn't, or no longer report one, and a +`package_todo.yml` entry for one it no longer reports is stale until you run +`pks update`. + ## 0.7.0 ### Features From 15b614c4d226b1cc0de801c49f8d63aac99ccf78 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:24:58 -0700 Subject: [PATCH 6/9] Try `::Taco` first for `has_many :tacos` in a model named `Taco` ActiveRecord's `_klass` (reflection.rb) looks up the class of `belongs_to :company` in a model whose name ends in `Company` as `::Company` first, and from the model's name only if that isn't defined. a81a247 gave the reference the namespaces of the model's name, so with `--experimental-parser`, in `class Payroll::Company`, pks looked for `::Payroll::Company::Company`, then `::Payroll::Company`, and found the model itself, where it had found `::Company`. So it missed a violation it used to report. In `module Payroll; class Company`, it found the model itself before a81a247 too. When the association's class has the model's own last name, the reference is now to `::Company`, so pks finds what Rails does in both. Where no `::Company` is defined, Rails goes on to find `Payroll::Company::Company` or the model itself, and pks finds nothing, so it misses a reference only where a `Payroll::Company::Company` is defined. A test also covers a model in `module Outer; class Foo` and in `class Object::Foo`. In a module, as a concern, pks can't know the model that includes it, and takes the module's name, and `model_namespaces` now says so. --- CHANGELOG.md | 5 ++ src/packs/parsing/ruby/experimental/parser.rs | 13 ++++- tests/list_references_test.rs | 56 ++++++++++++++++++- 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 989c7349..2c130e15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,11 @@ Rails looks it up from the model's name: in `class Outer::Foo`, as `class Outer::Foo` it finds `Outer::Taco` where it found `Taco`, and in `class ::Foo` it no longer takes `Outer::Taco` for the class. +When the class has the model's own name, as `has_many :tacos` in `class Taco` +inside `module Outer`, Rails tries `::Taco` first, and pks now does too, where +it found the model itself. A `package_todo.yml` entry for a violation that +moves is stale until you run `pks update`. + #### `--experimental-parser` names a constant written through `Object` The constants of `Object` are the top level's, so `class Object::Bar` inside diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index dfcedba6..2ebcf49f 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -55,7 +55,8 @@ impl ReferenceCollector<'_> { /// Rails looks up an association's class from its model's name, so these /// are that name's: `["Foo"]` in `module Outer; class ::Foo`, and - /// `["Outer", "Foo"]` in `class Outer::Foo`. + /// `["Outer", "Foo"]` in `class Outer::Foo`. In a module, as a concern's + /// `included` block, pks can't know the model, and takes the module's name. fn model_namespaces(&self) -> Vec { let Some((model, namespaces)) = self.current_namespaces.split_last() else { @@ -161,7 +162,15 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { ); if let Some(mut association_reference) = association_reference { - association_reference.namespace_path = self.model_namespaces(); + let namespaces = self.model_namespaces(); + // For a class with the model's own name, as `has_many :tacos` + // in `Outer::Taco`, Rails tries `::Taco` first (`_klass` in + // reflection.rb) + if namespaces.last() == Some(&association_reference.name) { + association_reference.name = + format!("::{}", association_reference.name); + } + association_reference.namespace_path = namespaces; self.references.push(association_reference); } } diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 2aa259d0..ef9126d8 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -461,9 +461,9 @@ fn test_list_references_from_associations_by_the_models_name( "class Taco\n def self.make; end\nend\n\ module Outer\n class Taco\n def self.make; end\n end\nend\n", )?; - // Rails looks `:tacos` up from the model's name, as `Outer::Foo::Taco`, - // `Outer::Taco`, then `Taco` from `Outer::Foo`, and as `Foo::Taco`, then - // `Taco` from `Foo`, which `class ::Foo` names whatever surrounds it + // Rails looks `:tacos` up from the model's name: as `Outer::Foo::Taco`, + // `Outer::Taco`, then `Taco` in `Outer::Foo`, and as `Foo::Taco`, then + // `Taco` in `class ::Foo`, which names the model `Foo` inside any module let compact = "packs/foo/app/services/foo_compact.rb"; fs::write( fixture.path(compact), @@ -474,6 +474,14 @@ fn test_list_references_from_associations_by_the_models_name( fixture.path(top_level), "module Outer\n class ::Foo\n has_many :tacos\n end\nend\n", )?; + // When the class has the model's own name, Rails tries `::Taco` first, so + // these don't find the model itself + let own_name = "packs/foo/app/services/foo_own_name.rb"; + fs::write( + fixture.path(own_name), + "class Payroll::Taco\n belongs_to :taco\nend\n\ + module Billing\n class Taco\n belongs_to :taco\n end\nend\n", + )?; assert_eq!( list_references( @@ -484,6 +492,8 @@ fn test_list_references_from_associations_by_the_models_name( "references": [ bar_from_foo(), to_bar_pack(compact, Some((2, 2)), "::Outer::Taco"), + to_bar_pack(own_name, Some((2, 2)), "::Taco"), + to_bar_pack(own_name, Some((6, 4)), "::Taco"), to_bar_pack(top_level, Some((3, 4)), "::Taco"), ] }) @@ -492,6 +502,46 @@ fn test_list_references_from_associations_by_the_models_name( Ok(()) } +#[test] +fn test_list_references_from_associations_in_namespaced_models( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "class Taco\n def self.make; end\nend\n\ + module Outer\n class Taco\n def self.make; end\n end\nend\n\ + module CoreExt\n class Taco\n def self.make; end\n end\nend\n", + )?; + let nested = "packs/foo/app/services/foo_nested.rb"; + fs::write( + fixture.path(nested), + "module Outer\n class Foo\n has_many :tacos\n end\nend\n", + )?; + // `class Object::Foo` is `::Foo` wherever it's written, so Rails looks + // `:tacos` up as `Foo::Taco`, then `Taco`, not as `CoreExt::Taco` + let through_object = "packs/foo/app/services/foo_through_object.rb"; + fs::write( + fixture.path(through_object), + "module CoreExt\n class Object::Foo\n has_many :tacos\n end\nend\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(nested, Some((3, 4)), "::Outer::Taco"), + to_bar_pack(through_object, Some((3, 4)), "::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 3a69fb189de710e5e43b11d481b96836468b7059 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 17:26:41 -0700 Subject: [PATCH 7/9] Correct what the follow-ups to #82 say, and test what they change bc0ad33 said that only a definition written from the top level, at the top level of a file, has a name that starts with `::::`, so nothing else changes. That isn't so. The constants defined in such a class, as `::::Taco::Shell` in `class ::Taco; class Shell`, start with it too, as does the `::::Taco` pks takes from `class ::Taco::Shell`, and a `Taco` written inside `class ::Taco` was dropped the same way. pks now counts each of these as a reference, as packwerk 3.3.1 does, and the CHANGELOG says so. Tests cover a `Taco` inside `class ::Taco`, `::Taco = 1` at the top level of a file, and `class ::Burrito` in `module Outer` and `class ::Cee` in `module Aa; module Bb`, from the file to the references pks lists. e9a6092 said that for a reference written `Object::Bar`, the resolver looks `Object` up as Ruby does. Inside `module CoreExt` it tries `::CoreExt::Object::Bar` first, as Ruby would, but where `CoreExt` defines an `Object` with no `Bar`, Ruby raises a NameError, and pks goes on to `::Bar`, as it does for `Foo::Bar` when `CoreExt::Foo` has no `Bar`. New tests name `module Object::Bar`, the constants in `class Object::Bar`, `::Object::Object::BAR` and `ObjectSpace::Bar`, and find constants defined through `Object` from another pack. Comments now say that only the experimental parser and resolver drop a leading `::Object`, and that the empty names come off the end as in constant_resolver. --- CHANGELOG.md | 16 ++- src/packs/parsing/ruby/experimental.rs | 14 ++ .../ruby/experimental/constant_resolver.rs | 6 + src/packs/parsing/ruby/experimental/parser.rs | 10 +- .../parsing/ruby/namespace_calculator.rs | 6 +- .../ruby/zeitwerk/constant_resolver.rs | 5 +- tests/list_references_test.rs | 121 +++++++++++++++++- 7 files changed, 158 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c130e15..a71c90bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,14 +97,16 @@ is still the namespace's own, as in Ruby. Without `--experimental-parser`, pks differed from packwerk in two cases, and now matches it: -- packwerk names `class ::Burrito` inside `module Outer` `::Outer::::Burrito`, - and finds no constant for it. pks took it for a reference to `Outer`, in the - pack that defines `Outer`. And where packwerk takes `::Aa::Bb::::Cee`, in - `module Aa; module Bb; class ::Cee`, for a reference to `Aa`, pks took it for - one to `Aa::Bb`. +- In `module Outer; class ::Burrito`, packwerk names the class + `::Outer::::Burrito` and finds no constant for it. pks took it for a + reference to `Outer`, in the pack that defines `Outer`. In + `module Aa; module Bb; class ::Cee`, packwerk takes `::Aa::Bb::::Cee` for a + reference to `Aa`, where pks took it for one to `Aa::Bb`. - After `class ::Taco` or `::Taco = 1` at the top level of a file, packwerk - counts a `Taco` or `::Taco` written later in the file as a reference, since - it names the definition `::::Taco`. pks took it for the file's own. + counts a `Taco` or `::Taco` written anywhere else in the file, inside the + class too, as a reference, since it names the definition `::::Taco`. pks took + it for the file's own, as it did a constant defined in the class, as + `Taco::Shell` after `class ::Taco; class Shell`. So `pks check` may report a violation it didn't, or no longer report one, and a `package_todo.yml` entry for one it no longer reports is stale until you run diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index bd3d14f5..79af6f29 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -466,6 +466,20 @@ end "module CoreExt; class ::Object; BAR = 1; end; end", &["::BAR"], ), + ("module CoreExt; ::Object::Object::BAR = 1; end", &["::BAR"]), + ( + "module CoreExt; module Object::Bar; BAZ = 1; end; end", + &["::Bar::BAZ"], + ), + ( + "module CoreExt; class Object::Bar; BAZ = 1; end; end", + &["::Bar::BAZ"], + ), + ("class Object; def a; end; end", &["::Object"]), + ( + "class ObjectSpace::Bar; def a; end; end", + &["::ObjectSpace::Bar"], + ), // A class written `Object` in a namespace is the namespace's own ( "module CoreExt; class Object; BAR = 1; end; end", diff --git a/src/packs/parsing/ruby/experimental/constant_resolver.rs b/src/packs/parsing/ruby/experimental/constant_resolver.rs index 1ef3d0df..3611d450 100644 --- a/src/packs/parsing/ruby/experimental/constant_resolver.rs +++ b/src/packs/parsing/ruby/experimental/constant_resolver.rs @@ -226,6 +226,12 @@ mod tests { for (constants, name, resolved) in [ (&["::Bar"][..], "Object::Bar", "::Bar"), (&["::Bar"], "::Object::Bar", "::Bar"), + (&["::Bar"], "::Object::Object::Bar", "::Bar"), + ( + &["::ObjectSpace::Bar"], + "::ObjectSpace::Bar", + "::ObjectSpace::Bar", + ), // `Object` is looked up as Ruby looks up any constant ( &["::Bar", "::CoreExt::Object::Bar"], diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index 2ebcf49f..c26e809f 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -260,13 +260,13 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } } -/// The name of a class, module or constant as defined, with `Object::Foo` taken -/// for `::Foo`. Ruby looks `Object` up as it does any constant, so a namespace -/// could define an `Object` of its own, but pks can't look it up here, and takes -/// it that none does. +/// The name of a class, module or constant as defined, with `Object::Foo` +/// taken for `::Foo`. Ruby looks `Object` up as it does any constant, so a +/// namespace could define an `Object` of its own, but pks can't look it up +/// here, and takes it that none does. fn defined_name(name: String) -> String { match name.strip_prefix("Object::") { - Some(name) => format!("::{}", name), + Some(rest) => format!("::{}", rest), None => name, } } diff --git a/src/packs/parsing/ruby/namespace_calculator.rs b/src/packs/parsing/ruby/namespace_calculator.rs index 8d4f0c20..6d9b6154 100644 --- a/src/packs/parsing/ruby/namespace_calculator.rs +++ b/src/packs/parsing/ruby/namespace_calculator.rs @@ -51,8 +51,10 @@ pub fn combine_namespace_with_constant_name( /// `class ::Foo` `::::Foo`, or `::Outer::::Foo` inside `module Outer`, and so /// do the packwerk parser and the Zeitwerk resolver, which match it. /// -/// The constants of `Object` are the top level's, so `::Object::Foo` is -/// `::Foo`, as is `Foo` inside a `class Object` at the top level. +/// The constants of `Object` are the top level's, so this names +/// `::Object::Foo` `::Foo`, as it does `Foo` inside a `class Object` at the top +/// level. packwerk, the packwerk parser and the Zeitwerk resolver, which don't +/// use this, name both `::Object::Foo`. pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String { let mut name = String::new(); for part in namespace_path.iter().copied().chain([const_name]) { diff --git a/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs index c7d088f7..27281be5 100644 --- a/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs +++ b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs @@ -113,8 +113,9 @@ impl ZeitwerkConstantResolver { // In this case, we'd try to resolve `::Foo::Bar` instead of `::Foo::Bar::Boo` let mut split_const = const_name.split("::").collect::>(); - // As in packwerk, whose `String#split` drops the empty names - // at the end, so `Outer::::Foo`'s parent `Outer::` has none + // As in packwerk's constant_resolver, whose `String#split` + // drops the empty names at the end, so `Outer::::Foo`'s + // parent `Outer::` has none while split_const.last() == Some(&"") { split_const.pop(); } diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index ef9126d8..4712530f 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -421,6 +421,50 @@ fn test_list_references_inside_a_class_defined_from_the_top_level( Ok(()) } +#[test] +fn test_list_references_to_a_class_defined_from_the_top_level_in_a_namespace( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + for (path, contents) in [ + ("outer.rb", "module Outer\nend\n"), + ("aa.rb", "module Aa\nend\n"), + ("aa/bb.rb", "module Aa::Bb\nend\n"), + ] { + let path = fixture.path(&format!("packs/bar/app/services/{}", path)); + fs::create_dir_all(path.parent().unwrap())?; + fs::write(path, contents)?; + } + // As in packwerk, `::Outer::::Burrito` has no parent, so it isn't a + // reference to `Outer`, and the parent of `::Aa::Bb::::Cee` is `::Aa`, not + // `::Aa::Bb` + let outer = "packs/foo/app/services/foo_outer.rb"; + fs::write( + fixture.path(outer), + "module Outer\n class ::Burrito\n end\nend\n", + )?; + let aa = "packs/foo/app/services/foo_aa.rb"; + fs::write( + fixture.path(aa), + "module Aa\n module Bb\n class ::Cee\n end\n end\nend\n", + )?; + + assert_eq!( + list_references(&fixture, &["list-references"]), + json!({ + "references": [ + bar_from_foo(), + baz_from_foo(), + to_bar_pack(aa, Some((1, 7)), "::Aa"), + to_bar_pack(aa, Some((2, 9)), "::Aa::Bb"), + to_bar_pack(aa, Some((3, 10)), "::Aa::Bb::::Cee"), + to_bar_pack(outer, Some((1, 7)), "::Outer"), + ] + }) + ); + + Ok(()) +} + #[test] fn test_list_references_after_a_class_defined_from_the_top_level( ) -> Result<(), Box> { @@ -430,11 +474,12 @@ fn test_list_references_after_a_class_defined_from_the_top_level( "class Taco\n def self.make; end\nend\n", )?; let file = "packs/foo/app/services/foo_taco.rb"; - // As in packwerk, which names this class `::::Taco`, the `Taco` and - // `::Taco` after it aren't the file's own + // As in packwerk, which names this class `::::Taco`, the `Taco` inside it + // and the `Taco` and `::Taco` after it aren't the file's own fs::write( fixture.path(file), - "class ::Taco\n def self.local; end\nend\nTaco.make\n::Taco.make\n", + "class ::Taco\n def self.local; end\n Taco.make\nend\n\ + Taco.make\n::Taco.make\n", )?; assert_eq!( @@ -443,8 +488,37 @@ fn test_list_references_after_a_class_defined_from_the_top_level( "references": [ bar_from_foo(), baz_from_foo(), - to_bar_pack(file, Some((4, 0)), "::Taco"), + to_bar_pack(file, Some((3, 2)), "::Taco"), to_bar_pack(file, Some((5, 0)), "::Taco"), + to_bar_pack(file, Some((6, 0)), "::Taco"), + ] + }) + ); + + Ok(()) +} + +#[test] +fn test_list_references_after_a_constant_assigned_from_the_top_level( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "class Taco\n def self.make; end\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + // As in packwerk, which names this constant `::::Taco`, the `Taco` and + // `::Taco` after it aren't the file's own + fs::write(fixture.path(file), "::Taco = 1\nTaco.make\n::Taco.make\n")?; + + assert_eq!( + list_references(&fixture, &["list-references"]), + json!({ + "references": [ + bar_from_foo(), + baz_from_foo(), + to_bar_pack(file, Some((2, 0)), "::Taco"), + to_bar_pack(file, Some((3, 0)), "::Taco"), ] }) ); @@ -542,6 +616,45 @@ fn test_list_references_from_associations_in_namespaced_models( Ok(()) } +#[test] +fn test_list_references_to_constants_defined_through_object( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // The experimental parser names these `::Taco`, `::Salsa` and `::GUAC`, + // not `::CoreExt::Object::Taco` and so on + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "module CoreExt\n class Object::Taco\n def self.make; end\n end\n\ + \n module Object::Salsa\n def self.pour; end\n end\n\ + \n Object::GUAC = 1\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + fs::write( + fixture.path(file), + "Taco.make\n::Object::Salsa.pour\nObject::GUAC\n", + )?; + let template = "packs/foo/app/views/foo_taco.erb"; + fs::write(fixture.path(template), "<%= ::Object::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"), + to_bar_pack(file, Some((2, 0)), "::Salsa"), + to_bar_pack(file, Some((3, 0)), "::GUAC"), + to_bar_pack(template, None, "::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 00ccc82306f8fa9e7d0eb97c55b316e5e9fd6c95 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 20:10:19 -0700 Subject: [PATCH 8/9] Look up associations and `Object` from the whole app, with --experimental-parser With `--experimental-parser`, two things that pks looked up from one file depend on the rest of the app, so they now wait until every file is read. Rails looks up the class of an association in a module, as a concern's `included` block, from the name of each model that includes the module. a81a247 and 15b614c took the module's name for the model's. pks now records the `include` and `prepend` calls in each class and module, and looks up the class from the name of each class that includes the module, directly or through other modules, so one association can reference a class for each. Where no class includes the module, the module's name stands in, as before. When the class has the model's own name and no `::Taco` is defined, Rails goes on to look it up from the model's name, as `Outer::Taco::Taco`; 15b614c looked for `::Taco` only. From the model's name, Rails takes only a class of the very name it looks for (`compute_type` in inheritance.rb), and pks now does too. In a controller, inherited_resources looks up the class of `belongs_to` from the controller's namespaces, and one given as `class_name:` from the top level, so pks no longer looks in the controller itself, and an ActiveRecord model whose name ends in `Controller` is looked up as any other. `class Object::Charge` in `module Payments` defines `::Payments::Object::Charge` where the app has a `Payments::Object`, even one that defines nothing. e9a6092 took it for `::Charge` whatever the app defines. Where some namespace has its own `Object`, pks now reads a file that defines a constant through `Object` in a namespace again, knowing which namespaces do, and names it as Ruby does. Since `check-contents` reads its file from stdin, which can be read only once, pks keeps what it read there. Separately, Ruby finds a constant of `Kernel` or `BasicObject`, which `Object` includes and inherits from, from anywhere, after the top level's own, and the experimental resolver now looks there last. --- CHANGELOG.md | 39 ++- src/packs.rs | 35 +- src/packs/caching/per_file_cache.rs | 2 + src/packs/file_utils.rs | 12 +- src/packs/monkey_patch_detection.rs | 16 +- src/packs/parsing/erb/experimental/parser.rs | 14 +- src/packs/parsing/erb/packwerk/parser.rs | 1 + src/packs/parsing/mod.rs | 35 +- src/packs/parsing/ruby/experimental.rs | 302 +++++++++++++++++- .../ruby/experimental/constant_resolver.rs | 19 +- src/packs/parsing/ruby/experimental/parser.rs | 187 ++++++++--- src/packs/parsing/ruby/packwerk/parser.rs | 1 + src/packs/parsing/ruby/parse_utils.rs | 11 + src/packs/reference_extractor.rs | 72 ++--- tests/check_test.rs | 32 ++ tests/list_references_test.rs | 231 ++++++++++++++ 16 files changed, 882 insertions(+), 127 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a71c90bc..5f70f5b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,20 +77,43 @@ Rails looks it up from the model's name: in `class Outer::Foo`, as `class ::Foo` it no longer takes `Outer::Taco` for the class. When the class has the model's own name, as `has_many :tacos` in `class Taco` -inside `module Outer`, Rails tries `::Taco` first, and pks now does too, where -it found the model itself. A `package_todo.yml` entry for a violation that -moves is stale until you run `pks update`. +inside `module Outer`, Rails tries `::Taco` first, and where there's none, looks +it up from the model's name, as `Outer::Taco::Taco`. pks now does too, where it +found the model itself. + +In a module, as a concern's `included` block, Rails looks up the class from the +name of each model that includes the module, directly or through other +modules. pks now finds those models from the `include` and `prepend` calls in +the app's classes and modules, and looks up the class from each one's name, so +one association can reference a class for each model. Where no class in the +app includes the module, pks looks it up from the module's name, as before. In +a controller, inherited_resources looks up the class of `belongs_to :taco` from +the controller's namespaces, and one given as `class_name:` from the top level, +and pks now does too, where it looked in the controller itself first. + +A `package_todo.yml` entry for a violation that moves is stale until you run +`pks update`. #### `--experimental-parser` names a constant written through `Object` The constants of `Object` are the top level's, so `class Object::Bar` inside `module CoreExt` defines `::Bar`, as does `class Bar` inside a `class Object` at the top level of a file. With `--experimental-parser`, pks named these -`::CoreExt::Object::Bar` and `::Object::Bar`. It now names them `::Bar`, -taking it that no namespace defines an `Object` of its own. A reference -written `::Object::Bar`, or `Object::Bar` where no namespace around it defines -an `Object`, now resolves to `::Bar` too. A `class Object` inside a namespace -is still the namespace's own, as in Ruby. +`::CoreExt::Object::Bar` and `::Object::Bar`. It now names them as Ruby does: +`::Bar`, or, where the app defines an `Object` in a namespace around it, as +`CoreExt::Object`, `::CoreExt::Object::Bar`. A reference written +`::Object::Bar` now resolves to `::Bar` too, as does one written `Object::Bar` +where no namespace around it defines an `Object`. A `class Object` inside a +namespace is still the namespace's own, as in Ruby. + +#### `--experimental-parser` finds the constants of `Kernel` and `BasicObject` + +`Object` includes `Kernel` and inherits from `BasicObject`, so Ruby finds a +constant that either defines, as `class BasicObject::Foo` or `FOO = 1` in +`module Kernel`, from anywhere, as `Foo` or `::Foo`, after the top level's own. +With `--experimental-parser`, pks now does too, except where Rails looks up the +class of an association from its model's name, which takes only a class of the +very name it looks for. #### pks finds what packwerk finds around a name written from the top level diff --git a/src/packs.rs b/src/packs.rs index 30476397..9fa7eab7 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -39,10 +39,12 @@ pub(crate) use self::checker::Violation; use self::creator::CreateResult; pub(crate) use self::pack_set::PackSet; 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::experimental::process_files as process_files_experimental; pub(crate) use self::parsing::ruby::zeitwerk::get_zeitwerk_constant_resolver; +pub(crate) use self::parsing::Inclusion; pub(crate) use self::parsing::ParsedDefinition; pub(crate) use self::parsing::SyntaxError; +pub(crate) use self::parsing::UnresolvedAssociation; pub(crate) use self::parsing::UnresolvedReference; use anyhow::bail; use cli::ColorChoice; @@ -272,11 +274,28 @@ pub fn delete_cache(configuration: Configuration) { } } -#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq)] +#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Default)] pub struct ProcessedFile { pub absolute_path: PathBuf, pub unresolved_references: Vec, pub definitions: Vec, + /// With `--experimental-parser`, the associations in a class or module, + /// whose references depend on the rest of the app + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub associations: Vec, + /// With `--experimental-parser` + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub inclusions: Vec, + /// With `--experimental-parser`, whether the file defines a constant + /// through `Object` inside a namespace, as `class Object::Bar` in + /// `module CoreExt`, which is named for the namespace's own `Object` where + /// the app defines one + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub object_in_a_namespace: bool, + /// With `--experimental-parser`, the namespaces' own `Object`s that the + /// file opens, as `::CoreExt::Object`, whatever they define + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub namespace_objects: Vec, /// Never cached, since a file with a syntax error isn't #[serde(skip)] pub syntax_error: Option, @@ -295,17 +314,11 @@ pub(crate) fn list_definitions( ambiguous: bool, ) -> anyhow::Result<()> { let constant_resolver = if configuration.experimental_parser { - let processed_files: Vec = process_files_with_cache( + process_files_experimental( &configuration.included_files, - configuration.get_cache(), configuration, - )?; - - get_experimental_constant_resolver( - &configuration.absolute_root, - &processed_files, - &configuration.ignored_definitions, - ) + )? + .1 } else { if ambiguous { bail!("Ambiguous mode is not supported for the Zeitwerk parser"); diff --git a/src/packs/caching/per_file_cache.rs b/src/packs/caching/per_file_cache.rs index e8b7594b..d64185ab 100644 --- a/src/packs/caching/per_file_cache.rs +++ b/src/packs/caching/per_file_cache.rs @@ -253,6 +253,7 @@ mod tests { }], definitions: vec![], syntax_error: None, + ..ProcessedFile::default() } }; @@ -308,6 +309,7 @@ mod tests { unresolved_references: vec![], definitions: vec![], syntax_error: None, + ..ProcessedFile::default() }; let CacheResult::Miss(empty_cache_entry) = cache.get(&path)? else { diff --git a/src/packs/file_utils.rs b/src/packs/file_utils.rs index f22889a6..b2cc897a 100644 --- a/src/packs/file_utils.rs +++ b/src/packs/file_utils.rs @@ -3,6 +3,7 @@ use std::{ fs, io, io::Read, path::{Path, PathBuf}, + sync::OnceLock, }; use crate::packs::Configuration; @@ -138,10 +139,17 @@ pub fn file_read_contents( configuration: &Configuration, ) -> anyhow::Result { if is_stdin_file(path, configuration) { - Ok(io::read_to_string(io::stdin()).context(format!( + // Kept, since stdin can be read only once, and a file can be read more + // than once + static STDIN_CONTENTS: OnceLock = OnceLock::new(); + if let Some(contents) = STDIN_CONTENTS.get() { + return Ok(contents.clone()); + } + let contents = io::read_to_string(io::stdin()).context(format!( "Failed to read contents of {} from stdin", path.to_string_lossy() - ))?) + ))?; + Ok(STDIN_CONTENTS.get_or_init(|| contents).clone()) } else { fs::read_to_string(path).context(format!( "Failed to read contents of {}", diff --git a/src/packs/monkey_patch_detection.rs b/src/packs/monkey_patch_detection.rs index 61da32c9..bb439254 100644 --- a/src/packs/monkey_patch_detection.rs +++ b/src/packs/monkey_patch_detection.rs @@ -5,8 +5,7 @@ use tracing::debug; use crate::packs::{ constant_resolver::ConstantDefinition, file_utils::glob_ruby_files_in_dirs, - get_experimental_constant_resolver, process_files_with_cache, - ProcessedFile, + process_files_experimental, }; use super::Configuration; @@ -28,17 +27,8 @@ pub fn expose_monkey_patches( included_files.extend(other_files_to_parse); - let processed_files: Vec = process_files_with_cache( - &included_files, - configuration.get_cache(), - configuration, - )?; - - let constant_resolver = get_experimental_constant_resolver( - &configuration.absolute_root, - &processed_files, - &configuration.ignored_definitions, - ); + let (_, constant_resolver) = + process_files_experimental(&included_files, configuration)?; let constant_definition_map = constant_resolver .fully_qualified_constant_name_to_constant_definition_map(); diff --git a/src/packs/parsing/erb/experimental/parser.rs b/src/packs/parsing/erb/experimental/parser.rs index 8039d2f5..8b8e0a83 100644 --- a/src/packs/parsing/erb/experimental/parser.rs +++ b/src/packs/parsing/erb/experimental/parser.rs @@ -2,7 +2,7 @@ use crate::packs::file_utils::file_read_contents; use crate::packs::{ file_utils::convert_erb_to_ruby_without_sourcemaps, parsing::{Range, SyntaxError}, - Configuration, ProcessedFile, UnresolvedReference, + Configuration, ProcessedFile, UnresolvedAssociation, UnresolvedReference, }; use std::path::Path; @@ -37,13 +37,25 @@ pub(crate) fn process_from_contents( }) .collect(); + let associations = processed_file + .associations + .into_iter() + .map(|association| UnresolvedAssociation { + location: Range::default(), + ..association + }) + .collect(); + ProcessedFile { absolute_path: path.to_path_buf(), unresolved_references: references_without_range, definitions: vec![], + associations, + inclusions: processed_file.inclusions, syntax_error: processed_file.syntax_error.map(|error| SyntaxError { line: None, ..error }), + ..ProcessedFile::default() } } diff --git a/src/packs/parsing/erb/packwerk/parser.rs b/src/packs/parsing/erb/packwerk/parser.rs index a07e609e..4cc44626 100644 --- a/src/packs/parsing/erb/packwerk/parser.rs +++ b/src/packs/parsing/erb/packwerk/parser.rs @@ -45,5 +45,6 @@ pub(crate) fn process_from_contents( line: None, ..error }), + ..ProcessedFile::default() } } diff --git a/src/packs/parsing/mod.rs b/src/packs/parsing/mod.rs index b95645a6..cfe771db 100644 --- a/src/packs/parsing/mod.rs +++ b/src/packs/parsing/mod.rs @@ -51,9 +51,7 @@ pub fn process_file( // method should make sure we never get any files we can't handle. Ok(ProcessedFile { absolute_path: path.to_path_buf(), - unresolved_references: vec![], - definitions: vec![], // TODO - syntax_error: None, + ..ProcessedFile::default() }) }; @@ -115,6 +113,37 @@ pub struct UnresolvedReference { pub location: Range, } +/// An association, as `has_many :tacos`, in a class or module. Rails looks +/// up its class from the name of the model that declares it, which in a +/// module, as a concern's `included` block, is that of each model that +/// includes the module, so the experimental parser leaves it to be looked up +/// once every file is read. +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] +pub struct UnresolvedAssociation { + /// The class's name, as `Taco` + pub name: String, + /// The fully qualified name of the class or module it's declared in + pub model: String, + pub in_module: bool, + /// `belongs_to`, which in a controller is inherited_resources' + pub belongs_to: bool, + /// Whether it names its class, as `class_name: "Taco"` + pub names_its_class: bool, + pub location: Range, +} + +/// `include Taggable` or `prepend Taggable` in the class or module `includer` +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone)] +pub struct Inclusion { + /// The fully qualified name of the class or module that includes it + pub includer: String, + pub includer_is_module: bool, + /// The module's name as written, as `Taggable` + pub name: String, + /// The namespaces it's written in + pub namespace_path: Vec, +} + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Default)] pub struct Range { pub start_row: usize, diff --git a/src/packs/parsing/ruby/experimental.rs b/src/packs/parsing/ruby/experimental.rs index 79af6f29..3da4c745 100644 --- a/src/packs/parsing/ruby/experimental.rs +++ b/src/packs/parsing/ruby/experimental.rs @@ -2,17 +2,255 @@ mod constant_resolver; pub(crate) mod parser; use self::constant_resolver::ExperimentalConstantResolver; +use self::parser::process_from_path_with_objects; +use super::namespace_calculator::nest_constant_name; use crate::packs::{ constant_resolver::{ConstantDefinition, ConstantResolver}, - ProcessedFile, + file_utils::{get_file_type, SupportedFileType}, + process_files_with_cache, Configuration, ProcessedFile, + UnresolvedAssociation, UnresolvedReference, }; -use rayon::prelude::{IntoParallelIterator, ParallelIterator}; +use rayon::prelude::{ + IntoParallelIterator, IntoParallelRefMutIterator, ParallelIterator, +}; use std::{ collections::{HashMap, HashSet}, path::{Path, PathBuf}, }; +/// Reads the files, and the resolver for their constants. A constant written +/// through a namespace's `Object` and the class of an association depend on +/// the rest of the app, so they wait until every file is read. +pub fn process_files( + paths: &HashSet, + configuration: &Configuration, +) -> anyhow::Result<(Vec, Box)> +{ + let mut processed_files = process_files_with_cache( + paths, + configuration.get_cache(), + configuration, + )?; + name_through_objects(&mut processed_files, configuration)?; + let constant_resolver = get_experimental_constant_resolver( + &configuration.absolute_root, + &processed_files, + &configuration.ignored_definitions, + ); + resolve_associations(&mut processed_files, constant_resolver.as_ref()); + Ok((processed_files, constant_resolver)) +} + +/// `class Object::Bar` in `module Payments` defines `::Payments::Object::Bar` +/// where the app defines a `Payments::Object`, and `::Bar` otherwise, so a +/// file that defines a constant that way is read again, knowing the +/// namespaces' `Object`s that the others define. That can name another +/// `Object`, so it goes on until the names stop changing. +fn name_through_objects( + processed_files: &mut [ProcessedFile], + configuration: &Configuration, +) -> anyhow::Result<()> { + let mut objects = HashSet::new(); + // A few times at most, in case the names of two `Object`s depend on each + // other's + for _ in 0..8 { + let found = objects_in_namespaces(processed_files); + if found == objects { + break; + } + objects = found; + processed_files + .par_iter_mut() + .filter(|processed_file| { + processed_file.object_in_a_namespace + && get_file_type(&processed_file.absolute_path) + == Some(SupportedFileType::Ruby) + }) + .try_for_each(|processed_file| -> anyhow::Result<()> { + *processed_file = process_from_path_with_objects( + &processed_file.absolute_path, + configuration, + &objects, + )?; + Ok(()) + })?; + } + Ok(()) +} + +/// The namespaces' own `Object`s, as `::Payments::Object`, that the files +/// open, or define a constant in +fn objects_in_namespaces(processed_files: &[ProcessedFile]) -> HashSet { + let opened = processed_files + .iter() + .flat_map(|processed_file| processed_file.namespace_objects.clone()); + let defined_in = processed_files + .iter() + .flat_map(|processed_file| &processed_file.definitions) + .flat_map(|definition| { + let name = definition.fully_qualified_name.as_str(); + name.match_indices("::Object") + .filter_map(move |(start, part)| { + let end = start + part.len(); + let whole = + end == name.len() || name[end..].starts_with("::"); + // `::Object` itself is the top level's + (start > 0 && whole).then(|| name[..end].to_owned()) + }) + }); + opened.chain(defined_in).collect() +} + +/// Turns each association into a reference, looked up as Rails does from the +/// name of its model. In a module, as a concern's `included` block, the +/// models are the classes that include it, directly or through other modules, +/// and where none does, the module's own name stands in for theirs. +fn resolve_associations( + processed_files: &mut [ProcessedFile], + constant_resolver: &(dyn ConstantResolver + Send + Sync), +) { + let includers = includers(processed_files, constant_resolver); + processed_files.par_iter_mut().for_each(|processed_file| { + for association in std::mem::take(&mut processed_file.associations) { + let models = if association.in_module { + models_including(&association.model, &includers) + } else { + vec![association.model.clone()] + }; + for model in models { + let Some(reference) = association_reference( + &association, + &model, + constant_resolver, + ) else { + continue; + }; + if !processed_file.unresolved_references.contains(&reference) { + processed_file.unresolved_references.push(reference); + } + } + } + }); +} + +/// For each module, the classes and modules that include or prepend it, and +/// whether each is a module +fn includers( + processed_files: &[ProcessedFile], + constant_resolver: &(dyn ConstantResolver + Send + Sync), +) -> HashMap> { + let mut includers: HashMap> = HashMap::new(); + for inclusion in processed_files + .iter() + .flat_map(|processed_file| &processed_file.inclusions) + { + let namespace_path: Vec<&str> = inclusion + .namespace_path + .iter() + .map(String::as_str) + .collect(); + let modules: HashSet = constant_resolver + .resolve(&inclusion.name, &namespace_path) + .unwrap_or_default() + .into_iter() + .map(|definition| definition.fully_qualified_name) + .collect(); + for module in modules { + let includer = + (inclusion.includer.clone(), inclusion.includer_is_module); + let module_includers = includers.entry(module).or_default(); + if !module_includers.contains(&includer) { + module_includers.push(includer); + } + } + } + includers +} + +/// The classes that include `module`, directly or through other modules, or +/// the module itself where none does +fn models_including( + module: &str, + includers: &HashMap>, +) -> Vec { + let mut models: Vec = vec![]; + let mut seen = HashSet::from([module.to_owned()]); + let mut modules = vec![module.to_owned()]; + while let Some(module) = modules.pop() { + for (includer, is_module) in + includers.get(&module).into_iter().flatten() + { + if *is_module { + if seen.insert(includer.clone()) { + modules.push(includer.clone()); + } + } else if !models.contains(includer) { + models.push(includer.clone()); + } + } + } + if models.is_empty() { + models.push(module.to_owned()); + } + models.sort(); + models +} + +/// The reference that `association` in `model` makes, if Rails finds its +/// class, as it looks it up +fn association_reference( + association: &UnresolvedAssociation, + model: &str, + constant_resolver: &(dyn ConstantResolver + Send + Sync), +) -> Option { + let name = &association.name; + let mut namespaces: Vec<&str> = model.split("::").skip(1).collect(); + let reference = |name: String, namespace_path: Vec| { + Some(UnresolvedReference { + name, + namespace_path, + location: association.location.clone(), + }) + }; + let model_name = namespaces.last().copied(); + if association.belongs_to + && model_name.is_some_and(|model| model.ends_with("Controller")) + { + // inherited_resources takes a class it's given from the top level, + // and looks up one it isn't from the controller's namespaces + if association.names_its_class { + return reference(name.clone(), vec![]); + } + namespaces.pop(); + let namespace_path = + namespaces.into_iter().map(str::to_owned).collect(); + return reference(name.clone(), namespace_path); + } + if model_name == Some(name.as_str()) { + // For a class with the model's own name, as in `Outer::Taco`, Rails + // tries `::Taco` first (`_klass` in reflection.rb), which finds a + // constant of `Kernel` or `BasicObject` too + let top_level = format!("::{}", name); + let defined = constant_resolver + .resolve(&top_level, &[]) + .is_some_and(|definitions| !definitions.is_empty()); + if defined { + return reference(top_level, vec![]); + } + } + // Then from the model's name, as `Outer::Foo::Taco`, `Outer::Taco`, then + // `Taco` in `Outer::Foo`, taking only a constant of that very name + // (`compute_type` in inheritance.rb) + let definitions = constant_resolver + .fully_qualified_constant_name_to_constant_definition_map(); + (0..=namespaces.len()) + .rev() + .map(|depth| nest_constant_name(&namespaces[..depth], name)) + .find(|candidate| definitions.contains_key(candidate)) + .and_then(|candidate| reference(candidate, vec![])) +} + pub fn get_experimental_constant_resolver( absolute_root: &Path, processed_files: &Vec, @@ -49,11 +287,59 @@ pub fn get_experimental_constant_resolver( mod tests { use std::path::PathBuf; + use std::collections::HashMap; + use crate::packs::parsing::ruby::experimental::parser::process_from_contents; use crate::packs::parsing::{ParsedDefinition, Range}; use crate::packs::{Configuration, ProcessedFile, UnresolvedReference}; use pretty_assertions::assert_eq; + #[test] + fn objects_in_namespaces_of_definitions() { + let processed_file = ProcessedFile { + definitions: [ + "::Object", + "::Payments::Object", + "::Billing::Object::Charge", + "::Aa::Object::Bb::Object", + "::Cc::ObjectSpace", + ] + .map(|name| ParsedDefinition { + fully_qualified_name: name.to_owned(), + location: Range::default(), + }) + .to_vec(), + ..ProcessedFile::default() + }; + let mut objects: Vec = + super::objects_in_namespaces(&[processed_file]) + .into_iter() + .collect(); + objects.sort(); + assert_eq!( + objects, + [ + "::Aa::Object", + "::Aa::Object::Bb::Object", + "::Billing::Object", + "::Payments::Object", + ] + ); + } + + #[test] + fn models_including_modules_that_include_each_other() { + let includers = HashMap::from([ + ("::A".to_owned(), vec![("::B".to_owned(), true)]), + ( + "::B".to_owned(), + vec![("::A".to_owned(), true), ("::Foo".to_owned(), false)], + ), + ]); + assert_eq!(super::models_including("::A", &includers), ["::Foo"]); + assert_eq!(super::models_including("::C", &includers), ["::C"]); + } + #[test] fn trivial_case() { let contents: String = String::from("Foo"); @@ -80,6 +366,7 @@ mod tests { unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -110,6 +397,7 @@ mod tests { unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -141,6 +429,7 @@ mod tests { unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -172,6 +461,7 @@ mod tests { unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -199,6 +489,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -236,6 +527,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -273,6 +565,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -309,6 +602,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -345,6 +639,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -526,6 +821,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -567,6 +863,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); @@ -609,6 +906,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); diff --git a/src/packs/parsing/ruby/experimental/constant_resolver.rs b/src/packs/parsing/ruby/experimental/constant_resolver.rs index 3611d450..bf999db9 100644 --- a/src/packs/parsing/ruby/experimental/constant_resolver.rs +++ b/src/packs/parsing/ruby/experimental/constant_resolver.rs @@ -36,7 +36,24 @@ impl ConstantResolver for ExperimentalConstantResolver { (namespace_path, fully_or_partially_qualified_constant) }; - Some(self.resolve_traversing_namespace_path(const_name, namespace_path)) + let definitions = + self.resolve_traversing_namespace_path(const_name, namespace_path); + if !definitions.is_empty() { + return Some(definitions); + } + // Then, as the top level's constants are `Object`'s, those of its + // ancestors, as `BasicObject::Foo` for `Foo`, from anywhere + let top_level = nest_constant_name(&[], const_name); + for ancestor in ["::Kernel", "::BasicObject"] { + let definitions = self.constant_for_fully_qualified_name(&format!( + "{}{}", + ancestor, top_level + )); + if !definitions.is_empty() { + return Some(definitions); + } + } + Some(vec![]) } fn fully_qualified_constant_name_to_constant_definition_map( diff --git a/src/packs/parsing/ruby/experimental/parser.rs b/src/packs/parsing/ruby/experimental/parser.rs index c26e809f..588c367d 100644 --- a/src/packs/parsing/ruby/experimental/parser.rs +++ b/src/packs/parsing/ruby/experimental/parser.rs @@ -7,10 +7,12 @@ use crate::packs::{ 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, + loc_to_range, names_its_class, visit_constant_assignments, + ParseError, }, }, - ParsedDefinition, Range, UnresolvedReference, + Inclusion, ParsedDefinition, Range, UnresolvedAssociation, + UnresolvedReference, }, Configuration, ProcessedFile, }; @@ -20,15 +22,25 @@ use ruby_prism::{ CallTargetNode, ClassNode, ConstantPathNode, ConstantReadNode, DefNode, Location, ModuleNode, Visit, }; -use std::path::Path; +use std::{collections::HashSet, path::Path}; struct ReferenceCollector<'a> { pub references: Vec, pub definitions: Vec, + pub associations: Vec, + pub inclusions: Vec, pub current_namespaces: Vec, + /// Whether each of `current_namespaces` is a module + pub modules: Vec, pub line_col_lookup: LineColLookup<'a>, pub behavioral_change_in_namespace: bool, pub custom_associations: Vec, + /// The namespaces' own `Object`s that the app defines, as + /// `::Payments::Object` + pub objects: &'a HashSet, + pub object_in_a_namespace: bool, + /// The namespaces' own `Object`s that the file opens + pub namespace_objects: Vec, } impl ReferenceCollector<'_> { @@ -39,7 +51,8 @@ impl ReferenceCollector<'_> { ) { if let Ok(name) = name { let location = loc_to_range(location, &self.line_col_lookup); - let definition = self.definition(&defined_name(name), location); + let name = self.defined_name(name); + let definition = self.definition(&name, location); self.definitions.push(definition); } } @@ -53,22 +66,92 @@ impl ReferenceCollector<'_> { } } - /// Rails looks up an association's class from its model's name, so these - /// are that name's: `["Foo"]` in `module Outer; class ::Foo`, and - /// `["Outer", "Foo"]` in `class Outer::Foo`. In a module, as a concern's - /// `included` block, pks can't know the model, and takes the module's name. - fn model_namespaces(&self) -> Vec { - let Some((model, namespaces)) = self.current_namespaces.split_last() - else { - return vec![]; + /// The name of a class, module or constant as defined. Ruby looks up the + /// `Object` of `Object::Foo` from the innermost namespace out, so it's a + /// namespace's own where the app defines one, and otherwise `::Object`, + /// whose constants are the top level's, so `Object::Foo` is `::Foo`. + fn defined_name(&mut self, name: String) -> String { + let Some(rest) = name.strip_prefix("Object::") else { + return name; }; + let namespaces: Vec<&str> = + self.current_namespaces.iter().map(String::as_str).collect(); + if !namespaces.is_empty() { + self.object_in_a_namespace = true; + } + for depth in (1..=namespaces.len()).rev() { + let object = nest_constant_name(&namespaces[..depth], "Object"); + if self.objects.contains(&object) { + return format!("{}::{}", object, rest); + } + } + format!("::{}", rest) + } + + /// The fully qualified name of the class or module the code is in + fn namespace_name(&self) -> Option { + let (last, namespaces) = self.current_namespaces.split_last()?; let namespaces: Vec<&str> = namespaces.iter().map(String::as_str).collect(); - nest_constant_name(&namespaces, model) - .split("::") - .skip(1) - .map(str::to_owned) - .collect() + Some(nest_constant_name(&namespaces, last)) + } + + /// Rails looks up an association's class from its model's name, so in a + /// class or module it waits for the rest of the app + fn associate(&mut self, node: &CallNode, reference: UnresolvedReference) { + match (self.namespace_name(), self.modules.last()) { + (Some(model), Some(&in_module)) + if !reference.name.starts_with("::") => + { + self.associations.push(UnresolvedAssociation { + name: reference.name, + model, + in_module, + belongs_to: node.name().as_slice() == b"belongs_to", + names_its_class: names_its_class(node), + location: reference.location, + }); + } + _ => self.references.push(UnresolvedReference { + namespace_path: vec![], + ..reference + }), + } + } + + /// Opens a class or module, noting one that is a namespace's own + /// `Object`, as `class Object` in `module CoreExt`, whether or not it + /// defines anything + fn open_namespace(&mut self, namespace: String, module: bool) { + self.current_namespaces.push(namespace); + self.modules.push(module); + if let Some(name) = self.namespace_name() { + if name.ends_with("::Object") && name != "::Object" { + self.namespace_objects.push(name); + } + } + } + + /// `include Taggable` or `prepend Taggable` in a class or module + fn include(&mut self, node: &CallNode) { + let (Some(includer), Some(&includer_is_module)) = + (self.namespace_name(), self.modules.last()) + else { + return; + }; + let Some(arguments) = node.arguments() else { + return; + }; + for argument in arguments.arguments().iter() { + if let Ok(name) = fetch_const_name(&argument) { + self.inclusions.push(Inclusion { + includer: includer.clone(), + includer_is_module, + name, + namespace_path: self.current_namespaces.clone(), + }); + } + } } fn reference_constant( @@ -111,7 +194,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { return; } - let namespace = defined_name(namespace_result.unwrap()); + let namespace = self.defined_name(namespace_result.unwrap()); if let Some(inner) = node.superclass() { self.visit(&inner); @@ -126,7 +209,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { // 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 // more efficient? - self.current_namespaces.push(namespace); + self.open_namespace(namespace, false); // Each time we open up a new class/module, we reset the behavioral change flag let previous_behavioral_change = self.behavioral_change_in_namespace; @@ -145,6 +228,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { self.behavioral_change_in_namespace = previous_behavioral_change; self.current_namespaces.pop(); + self.modules.pop(); } fn visit_call_node(&mut self, node: &CallNode<'pr>) { @@ -161,17 +245,14 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &self.custom_associations, ); - if let Some(mut association_reference) = association_reference { - let namespaces = self.model_namespaces(); - // For a class with the model's own name, as `has_many :tacos` - // in `Outer::Taco`, Rails tries `::Taco` first (`_klass` in - // reflection.rb) - if namespaces.last() == Some(&association_reference.name) { - association_reference.name = - format!("::{}", association_reference.name); - } - association_reference.namespace_path = namespaces; - self.references.push(association_reference); + if let Some(association_reference) = association_reference { + self.associate(node, association_reference); + } + + if node.receiver().is_none() + && matches!(node.name().as_slice(), b"include" | b"prepend") + { + self.include(node); } } ruby_prism::visit_call_node(self, node); @@ -203,7 +284,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { visit_constant_assignments!(); fn visit_module_node(&mut self, node: &ModuleNode<'pr>) { - let namespace = defined_name( + let namespace = self.defined_name( fetch_const_name(&node.constant_path()).unwrap_or("".to_owned()), ); let location = loc_to_range( @@ -216,7 +297,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { // 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 // more efficient? - self.current_namespaces.push(namespace); + self.open_namespace(namespace, true); // Each time we open up a new class/module, we reset the behavioral change flag let previous_behavioral_change = self.behavioral_change_in_namespace; @@ -235,6 +316,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { self.behavioral_change_in_namespace = previous_behavioral_change; self.current_namespaces.pop(); + self.modules.pop(); } fn visit_constant_read_node(&mut self, node: &ConstantReadNode<'pr>) { @@ -260,33 +342,33 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } } -/// The name of a class, module or constant as defined, with `Object::Foo` -/// taken for `::Foo`. Ruby looks `Object` up as it does any constant, so a -/// namespace could define an `Object` of its own, but pks can't look it up -/// here, and takes it that none does. -fn defined_name(name: String) -> String { - match name.strip_prefix("Object::") { - Some(rest) => format!("::{}", rest), - None => name, - } +pub(crate) fn process_from_path( + path: &Path, + configuration: &Configuration, +) -> anyhow::Result { + process_from_path_with_objects(path, configuration, &HashSet::new()) } -pub(crate) fn process_from_path( +/// With `objects`, the namespaces' own `Object`s that the app defines +pub(crate) fn process_from_path_with_objects( path: &Path, configuration: &Configuration, + objects: &HashSet, ) -> anyhow::Result { let contents = file_read_contents(path, configuration)?; - Ok(process_from_contents(contents, path, configuration)) + // Rails runs a Builder template, as it does an ERB one, as a method body + let template = path.extension().is_some_and(|ext| ext == "builder"); + Ok(process(contents, path, configuration, template, objects)) } +#[cfg(test)] pub(crate) fn process_from_contents( contents: String, path: &Path, configuration: &Configuration, ) -> ProcessedFile { - // Rails runs a Builder template, as it does an ERB one, as a method body let template = path.extension().is_some_and(|ext| ext == "builder"); - process(contents, path, configuration, template) + process(contents, path, configuration, template, &HashSet::new()) } /// For the Ruby that pks makes from an ERB template @@ -295,7 +377,7 @@ pub(crate) fn process_from_template_contents( path: &Path, configuration: &Configuration, ) -> ProcessedFile { - process(contents, path, configuration, true) + process(contents, path, configuration, true, &HashSet::new()) } fn process( @@ -303,6 +385,7 @@ fn process( path: &Path, configuration: &Configuration, template: bool, + objects: &HashSet, ) -> ProcessedFile { let lookup = LineColLookup::new(&contents); let parse_result = ruby_prism::parse(contents.as_bytes()); @@ -311,11 +394,17 @@ fn process( let mut collector = ReferenceCollector { references: vec![], - current_namespaces: vec![], definitions: vec![], + associations: vec![], + inclusions: vec![], + current_namespaces: vec![], + modules: vec![], line_col_lookup: lookup, behavioral_change_in_namespace: false, custom_associations: configuration.custom_associations.clone(), + objects, + object_in_a_namespace: false, + namespace_objects: vec![], }; collector.visit(&parse_result.node()); @@ -333,6 +422,10 @@ fn process( absolute_path, unresolved_references, definitions, + associations: collector.associations, + inclusions: collector.inclusions, + object_in_a_namespace: collector.object_in_a_namespace, + namespace_objects: collector.namespace_objects, syntax_error, } } diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 00c8df4b..95377e9a 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -345,5 +345,6 @@ fn process( unresolved_references, definitions, syntax_error, + ..ProcessedFile::default() } } diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 7ad5fe16..42f8e619 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -212,6 +212,17 @@ pub fn get_reference_from_active_record_association( } } +/// Whether an association names its class, as `class_name: "Taco"` +pub fn names_its_class(node: &CallNode) -> bool { + node.arguments().is_some_and(|arguments| { + arguments.arguments().iter().any(|argument| { + argument.as_keyword_hash_node().is_some_and(|kwargs| { + extract_class_name_from_kwargs(&kwargs).is_some() + }) + }) + }) +} + fn extract_class_name_from_kwargs(kwargs: &KeywordHashNode) -> Option { for pair_node in kwargs.elements().iter() { if let Some(pair) = pair_node.as_assoc_node() { diff --git a/src/packs/reference_extractor.rs b/src/packs/reference_extractor.rs index 706178ef..ac1bd099 100644 --- a/src/packs/reference_extractor.rs +++ b/src/packs/reference_extractor.rs @@ -6,7 +6,7 @@ use rayon::prelude::{ use tracing::debug; use crate::packs::{ - get_experimental_constant_resolver, get_zeitwerk_constant_resolver, + get_zeitwerk_constant_resolver, process_files_experimental, process_files_with_cache, ProcessedFile, }; @@ -16,46 +16,40 @@ pub(crate) fn get_all_references( configuration: &Configuration, absolute_paths: &HashSet, ) -> anyhow::Result> { - let cache = configuration.get_cache(); - debug!("Getting unresolved references (using cache if possible)"); - let (constant_resolver, processed_files_to_check) = if configuration - .experimental_parser - { - // The experimental parser needs *all* processed files to get definitions - let all_processed_files: Vec = process_files_with_cache( - &configuration.included_files, - cache, - configuration, - )?; - - let constant_resolver = get_experimental_constant_resolver( - &configuration.absolute_root, - &all_processed_files, - &configuration.ignored_definitions, - ); - - let processed_files_to_check = all_processed_files - .into_iter() - .filter(|processed_file| { - absolute_paths.contains(&processed_file.absolute_path) - }) - .collect(); - - (constant_resolver, processed_files_to_check) - } else { - let processed_files: Vec = - process_files_with_cache(absolute_paths, cache, configuration)?; - - // The zeitwerk constant resolver doesn't look at processed files to get definitions - let constant_resolver = get_zeitwerk_constant_resolver( - &configuration.pack_set, - &configuration.constant_resolver_configuration(), - ); - - (constant_resolver, processed_files) - }; + let (constant_resolver, processed_files_to_check) = + if configuration.experimental_parser { + // The experimental parser needs *all* processed files to get definitions + let (all_processed_files, constant_resolver) = + process_files_experimental( + &configuration.included_files, + configuration, + )?; + + let processed_files_to_check = all_processed_files + .into_iter() + .filter(|processed_file| { + absolute_paths.contains(&processed_file.absolute_path) + }) + .collect(); + + (constant_resolver, processed_files_to_check) + } else { + let processed_files: Vec = process_files_with_cache( + absolute_paths, + configuration.get_cache(), + configuration, + )?; + + // The zeitwerk constant resolver doesn't look at processed files to get definitions + let constant_resolver = get_zeitwerk_constant_resolver( + &configuration.pack_set, + &configuration.constant_resolver_configuration(), + ); + + (constant_resolver, processed_files) + }; debug!("Turning unresolved references into fully qualified references"); let references: anyhow::Result> = processed_files_to_check diff --git a/tests/check_test.rs b/tests/check_test.rs index 54e86041..53c49111 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -1689,3 +1689,35 @@ fn test_check_a_deeply_nested_file() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_check_contents_of_a_file_read_twice_with_experimental_parser( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // `Payments` defines its own `Object`, so pks reads the file written + // through it again once it's read the others, and has to keep what it + // read from stdin + fs::write( + fixture.path("packs/baz/app/services/payments_object.rb"), + "module Payments\n class Object\n end\nend\n", + )?; + let file = "packs/foo/app/services/foo_charge.rb"; + fs::write(fixture.path(file), "module Payments\nend\n")?; + + cargo_bin_cmd!("pks") + .env_remove("RUST_LOG") + .arg("--project-root") + .arg(fixture.root()) + .args(["--experimental-parser", "check-contents", file]) + .write_stdin( + "module Payments\n class Object::Charge\n def self.make\n \ + Bar\n end\n end\nend\n", + ) + .assert() + .code(1) + .stdout(predicate::str::contains(format!( + "{}:4:6\nDependency violation: `::Bar`", + file + ))); + Ok(()) +} diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 4712530f..498e2136 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -655,6 +655,237 @@ fn test_list_references_to_constants_defined_through_object( Ok(()) } +#[test] +fn test_list_references_from_associations_in_concerns( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "class Taco\n def self.make; end\nend\n\ + module Outer\n class Taco\n def self.make; end\n end\nend\n\ + module Billing\n class Taco\n def self.make; end\n end\nend\n", + )?; + // Rails looks `:tacos` up from the name of each model that includes the + // concern, `Plain` and `Billing::Invoice`, which includes `Trackable` + // through `Billing::Auditable`. With no model, the concern's name stands + // in for one. + let taggable = "packs/foo/app/services/foo_taggable.rb"; + let trackable = "packs/foo/app/services/foo_trackable.rb"; + let unused = "packs/foo/app/services/foo_unused.rb"; + for (file, contents) in [ + ( + taggable, + "module Outer::Taggable\n extend ActiveSupport::Concern\n\n \ + included do\n has_many :tacos\n end\nend\n", + ), + ( + trackable, + "module Trackable\n extend ActiveSupport::Concern\n\n \ + included do\n belongs_to :taco\n end\nend\n", + ), + ( + unused, + "module Outer\n module Unused\n \ + extend ActiveSupport::Concern\n\n included do\n \ + has_many :tacos\n end\n end\nend\n", + ), + ( + "packs/foo/app/services/foo_auditable.rb", + "module Billing\n module Auditable\n \ + extend ActiveSupport::Concern\n include Trackable\n end\n\ + end\n", + ), + ( + "packs/foo/app/services/foo_invoice.rb", + "module Billing\n class Invoice\n include Outer::Taggable\n \ + include Auditable\n end\nend\n", + ), + ( + "packs/foo/app/services/foo_plain.rb", + "class Plain\n include Outer::Taggable\nend\n", + ), + ] { + fs::write(fixture.path(file), contents)?; + } + // An association in a template too, which has no position + let template = "packs/foo/app/views/foo_tacos.erb"; + fs::write( + fixture.path(template), + "<% class Plain; has_many :tacos; end %>\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(taggable, Some((5, 4)), "::Billing::Taco"), + to_bar_pack(taggable, Some((5, 4)), "::Taco"), + to_bar_pack(trackable, Some((5, 4)), "::Billing::Taco"), + to_bar_pack(unused, Some((6, 6)), "::Outer::Taco"), + to_bar_pack(template, None, "::Taco"), + ] + }) + ); + + Ok(()) +} + +#[test] +fn test_list_references_from_associations_named_for_their_model( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "module Outer\n class Taco\n class Taco\n \ + def self.make; end\n end\n end\nend\n\ + module Billing\n class Taco\n def self.make; end\n end\n \ + class Salsa\n def self.pour; end\n end\nend\n\ + class Salsa\n def self.pour; end\nend\n", + )?; + // With no `::Taco`, Rails goes on to look `:tacos` up from the model's + // name, and finds `Outer::Taco::Taco` + let own_name = "packs/foo/app/services/foo_own_name.rb"; + fs::write( + fixture.path(own_name), + "class Outer::Taco\n has_many :tacos\nend\n", + )?; + // inherited_resources looks up `belongs_to :taco` from the controller's + // namespace, and not in the controller, and takes a class it's given from + // the top level + let controller = "packs/foo/app/services/foo_controller.rb"; + fs::write( + fixture.path(controller), + "class Billing::TacosController\n class Taco\n def self.local; end\n \ + end\n\n belongs_to :taco\n belongs_to :sauce, class_name: \"Salsa\"\n\ + end\n", + )?; + // ActiveRecord's `has_many` in a model named `...Controller` is looked up + // from the model's name, so it finds the model's own `Taco` + fs::write( + fixture.path("packs/foo/app/services/foo_motor_controller.rb"), + "class Billing::TacoMotorController\n class Taco\n \ + def self.local; end\n end\n\n has_many :tacos\nend\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(controller, Some((6, 2)), "::Billing::Taco"), + to_bar_pack(controller, Some((7, 2)), "::Salsa"), + to_bar_pack(own_name, Some((2, 2)), "::Outer::Taco::Taco"), + ] + }) + ); + + Ok(()) +} + +#[test] +fn test_list_references_to_constants_defined_through_a_namespaces_object( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // `Payments` defines an `Object` of its own, so `class Object::Charge` in + // `Payments`, in another file, is `Payments::Object::Charge` + fs::write( + fixture.path("packs/bar/app/services/payments_object.rb"), + "module Payments\n class Object\n def self.pay; end\n end\nend\n", + )?; + fs::write( + fixture.path("packs/bar/app/services/payments_charge.rb"), + "module Payments\n class Object::Charge\n FEE = 1\n\n \ + def self.make; end\n end\nend\n", + )?; + // As does an `Object` that defines nothing + fs::write( + fixture.path("packs/bar/app/services/kitchen_object.rb"), + "module Kitchen\n class Object\n end\nend\n\ + module Kitchen\n class Object::Dish\n def self.make; end\n end\n\ + end\n", + )?; + let file = "packs/foo/app/services/foo_payments.rb"; + fs::write( + fixture.path(file), + "Payments::Object::Charge.make\nPayments::Object::Charge::FEE\n\ + ::Charge.make\nKitchen::Object::Dish.make\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(file, Some((1, 0)), "::Payments::Object::Charge"), + to_bar_pack( + file, + Some((2, 0)), + "::Payments::Object::Charge::FEE" + ), + to_bar_pack(file, Some((4, 0)), "::Kitchen::Object::Dish"), + ] + }) + ); + + Ok(()) +} + +#[test] +fn test_list_references_to_constants_of_objects_ancestors( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + // The top level's constants are `Object`'s, so those of `Kernel` and + // `BasicObject` are found from anywhere + fs::write( + fixture.path("packs/bar/app/services/taco.rb"), + "class BasicObject::Taco\n def self.make; end\nend\n\ + module Kernel\n SALSA = 1\n\n class Pepper\n def self.x; end\n \ + end\nend\n", + )?; + let file = "packs/foo/app/services/foo_taco.rb"; + fs::write( + fixture.path(file), + "module Outer\n Taco.make\n ::Taco.make\n SALSA\nend\n", + )?; + // Rails looks up `:peppers` in `Outer::Pepper` as `::Pepper`, which finds + // `Kernel::Pepper`, but takes only a class of the very name it looks for + // from the model's name, so `Billing::Thing` has none + let associations = "packs/foo/app/services/foo_peppers.rb"; + fs::write( + fixture.path(associations), + "class Outer::Pepper\n has_many :peppers\nend\n\ + class Billing::Thing\n has_many :peppers\nend\n", + )?; + + assert_eq!( + list_references( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "references": [ + bar_from_foo(), + to_bar_pack(associations, Some((2, 2)), "::Kernel::Pepper"), + to_bar_pack(file, Some((2, 2)), "::BasicObject::Taco"), + to_bar_pack(file, Some((3, 2)), "::BasicObject::Taco"), + to_bar_pack(file, Some((4, 2)), "::Kernel::SALSA"), + ] + }) + ); + + 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 cf6ed55234ea56cb220bf4410bb7f0971efbd27c Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 13 Oct 2026 20:10:44 -0700 Subject: [PATCH 9/9] Name a class defined in `class_eval` or `Class.new` as packwerk does packwerk names the reference that a class or module definition makes to itself from every class, module, `class_eval` block on a constant, and constant assigned `Class.new` or `Module.new` around it (`parent_module_name` in node_helpers.rb). pks named it from the classes and modules alone, so in `module Aa; Bb.class_eval do; class ::Cee` it left out `Bb`. c081104 made the Zeitwerk resolver drop the empty name at the end of a parent, as packwerk does, and that turned the gap into a regression there: pks found no reference where packwerk finds one to `Aa`, and in `module Aa; module Bb; Cc.class_eval do; class ::Dee` it found one to `Aa` where packwerk's is to `Aa::Bb`. The packwerk parser now keeps a second stack of names for this, with the receiver of `Foo.class_eval do` and the constant of `Foo = Class.new do`, and names the reference from it. A block that takes `_1` or `it`, which packwerk's parser makes a `numblock` or `itblock`, adds nothing, as in packwerk. Where packwerk can't name the class, in a block passed with `&.` or to `super`, pks no longer counts a reference either. The file's own definitions, and the namespaces each reference is looked up in, still come from the classes and modules alone, as in packwerk. On a copy of `simple_app` with 66 such files, list-references and packwerk 3.3.1 now differ on one line: a reference packwerk counts to the scope `Kk` of `Kk::Ll = 1`, which pks has never counted. --- CHANGELOG.md | 11 +- src/packs/parsing/ruby/packwerk.rs | 127 ++++++++++++++ src/packs/parsing/ruby/packwerk/parser.rs | 199 ++++++++++++++++++++-- src/packs/parsing/ruby/parse_utils.rs | 8 +- tests/list_references_test.rs | 56 ++++++ 5 files changed, 382 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f70f5b7..146a4786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,8 +117,8 @@ very name it looks for. #### pks finds what packwerk finds around a name written from the top level -Without `--experimental-parser`, pks differed from packwerk in two cases, and -now matches it: +Without `--experimental-parser`, pks differed from packwerk in three cases, +and now matches it: - In `module Outer; class ::Burrito`, packwerk names the class `::Outer::::Burrito` and finds no constant for it. pks took it for a @@ -130,6 +130,13 @@ now matches it: class too, as a reference, since it names the definition `::::Taco`. pks took it for the file's own, as it did a constant defined in the class, as `Taco::Shell` after `class ::Taco; class Shell`. +- packwerk names a class or module defined in a `class_eval` block, or in a + `Class.new` or `Module.new` block assigned to a constant, inside the block's + receiver or the constant. It takes `class Cee` in + `module Aa; Bb.class_eval do` for a reference to `::Aa::Bb::Cee`, and + `class ::Cee` there for one to `::Aa::Bb::::Cee`, whose parent is `Aa`. pks + left out `Bb`. As in packwerk, a class defined in a block passed with `&.` + or to `super` is now no reference. So `pks check` may report a violation it didn't, or no longer report one, and a `package_todo.yml` entry for one it no longer reports is stale until you run diff --git a/src/packs/parsing/ruby/packwerk.rs b/src/packs/parsing/ruby/packwerk.rs index 32f6f58e..c3a918dc 100644 --- a/src/packs/parsing/ruby/packwerk.rs +++ b/src/packs/parsing/ruby/packwerk.rs @@ -925,6 +925,133 @@ FOO = BAR } } + #[test] + fn class_defined_in_class_eval() { + // packwerk names the class as one in `Foo`, but looks up the constants + // in it, and names those defined around it, as if it were in `M` + let contents = "\ +module M + Foo.class_eval do + class Bar + Baz + end + QUX = 1 + QUX + end +end +"; + let references: Vec<(String, Vec)> = process_from_contents( + contents.to_owned(), + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references + .into_iter() + .map(|reference| (reference.name, reference.namespace_path)) + .collect(); + assert_eq!( + references, + vec![ + (String::from("::M"), vec![]), + (String::from("Foo"), vec![String::from("M")]), + (String::from("::M::Foo::Bar"), vec![String::from("M")]), + ( + String::from("Baz"), + vec![String::from("M"), String::from("Bar")] + ), + ] + ); + } + + #[test] + fn classes_defined_in_blocks_and_assignments() { + // The name packwerk gives `Bar`, from the `class_eval` blocks and + // `Class.new` or `Module.new` assignments around it. It names none in + // a block passed with `&.` or to `super`. + for (contents, bar) in [ + ("Foo.class_eval do class Bar; end end", Some("::Foo::Bar")), + ("Foo.class_eval { module Bar; end }", Some("::Foo::Bar")), + ( + "Foo.class_eval(1) do |x| class Bar; end end", + Some("::Foo::Bar"), + ), + ("A::B.class_eval do class Bar; end end", Some("::A::B::Bar")), + ( + "::Foo.class_eval do class Bar; end end", + Some("::::Foo::Bar"), + ), + ( + "module M; self::Foo.class_eval do class Bar; end end; end", + Some("::M::::Foo::Bar"), + ), + ( + "module M; Foo.class_eval do class ::Bar; end end; end", + Some("::M::Foo::::Bar"), + ), + ( + "A.class_eval do B.class_eval do class Bar; end end end", + Some("::A::B::Bar"), + ), + ("Foo.class_eval(class Bar; end) do end", Some("::Foo::Bar")), + ("Foo = Class.new do class Bar; end end", Some("::Foo::Bar")), + ( + "A::B = Module.new do module Bar; end end", + Some("::A::B::Bar"), + ), + ( + "Foo = Class.new(Base) { class Bar; end }", + Some("::Foo::Bar"), + ), + ("Foo = Class.new(class Bar; self; end)", Some("::Foo::Bar")), + ( + "Foo = Class.new do A.class_eval do class Bar; end end end", + Some("::Foo::A::Bar"), + ), + ( + "Foo.class_eval do end.then do class Bar; end end", + Some("::Bar"), + ), + ("Foo.module_eval do class Bar; end end", Some("::Bar")), + ("Foo.class_exec do class Bar; end end", Some("::Bar")), + ("foo.class_eval do class Bar; end end", Some("::Bar")), + ("self.class_eval do class Bar; end end", Some("::Bar")), + ("class_eval do class Bar; end end", Some("::Bar")), + ("Foo.class_eval { _1; class Bar; end }", Some("::Bar")), + ("Foo.class_eval { it; class Bar; end }", Some("::Bar")), + ("Foo&.class_eval { _1; class Bar; end }", Some("::Bar")), + ("Foo = Class.new { _1; class Bar; end }", Some("::Bar")), + ("Foo = Class&.new(class Bar; self; end)", Some("::Bar")), + ("Foo = ::Class.new do class Bar; end end", Some("::Bar")), + ("Foo = Struct.new(:a) do class Bar; end end", Some("::Bar")), + ("Foo = (Class.new do class Bar; end end)", Some("::Bar")), + ("Foo ||= Class.new do class Bar; end end", Some("::Bar")), + ("Foo, Baz = Class.new do class Bar; end end", Some("::Bar")), + ("Foo&.class_eval do class Bar; end end", None), + ("Foo&.then do class Bar; end end", None), + ("foo::Baz.class_eval do class Bar; end end", None), + ("Foo = Class&.new do class Bar; end end", None), + ("super do class Bar; end end", None), + ("super(1) do class Bar; end end", None), + ] { + let references: Vec = process_from_contents( + contents.to_owned(), + &PathBuf::from("path/to/file.rb"), + &Configuration::default(), + ) + .unresolved_references + .into_iter() + .map(|reference| reference.name) + .filter(|name| name.ends_with("Bar")) + .collect(); + assert_eq!( + references, + bar.into_iter().collect::>(), + "{}", + contents + ); + } + } + #[test] fn association_calls() { // Where each finds `Bar`. pks has never read a `&.` call as an diff --git a/src/packs/parsing/ruby/packwerk/parser.rs b/src/packs/parsing/ruby/packwerk/parser.rs index 95377e9a..dda4cd2a 100644 --- a/src/packs/parsing/ruby/packwerk/parser.rs +++ b/src/packs/parsing/ruby/packwerk/parser.rs @@ -17,8 +17,9 @@ use crate::packs::{ }; use line_col::LineColLookup; use ruby_prism::{ - CallNode, ClassNode, ConstantPathNode, ConstantReadNode, Location, - ModuleNode, Visit, + BlockNode, CallNode, ClassNode, ConstantId, ConstantPathNode, + ConstantPathWriteNode, ConstantReadNode, ConstantWriteNode, + ForwardingSuperNode, Location, ModuleNode, Node, SuperNode, Visit, }; use serde::{Deserialize, Serialize}; use std::{collections::HashMap, path::Path}; @@ -33,6 +34,11 @@ struct ReferenceCollector<'a> { pub references: Vec, pub definitions: Vec, pub current_namespaces: Vec, + /// The names that packwerk's `parent_module_name` puts before a class or + /// module's own: those of the classes and modules around it, and `Foo` in + /// `Foo.class_eval do` or `Foo = Class.new do`. An error where packwerk + /// can't name it. + pub definition_namespaces: Vec>, pub line_col_lookup: LineColLookup<'a>, pub in_superclass: bool, pub superclasses: Vec, @@ -102,6 +108,112 @@ impl ReferenceCollector<'_> { location: loc_to_range(location, &self.line_col_lookup), }) } + + /// The name of the class or module `name` defined here, as packwerk names + /// the reference it counts there, or `None` if it can't + fn definition_reference_name(&self, name: &str) -> Option { + let mut names = self + .definition_namespaces + .iter() + .map(|namespace| namespace.as_deref().ok()) + .collect::>>()?; + names.push(name); + Some(format!("::{}", names.join("::"))) + } + + /// Runs `visit` with `namespace`, if any, in `definition_namespaces` + fn visit_in_definition_namespace( + &mut self, + namespace: Option>, + visit: impl FnOnce(&mut Self), + ) { + let Some(namespace) = namespace else { + return visit(self); + }; + self.definition_namespaces.push(namespace); + visit(self); + self.definition_namespaces.pop(); + } + + /// packwerk names a class or module defined in `Foo = Class.new do` as one + /// in `Foo` + fn visit_assigned_value( + &mut self, + name: Result, + value: &Node, + ) { + let namespace = creates_module(value).then_some(name); + self.visit_in_definition_namespace(namespace, |collector| { + collector.visit(value) + }); + } +} + +/// A block that takes no parameters, or named ones. packwerk's parser makes +/// one that takes `_1` or `it` a `numblock` or `itblock`, which packwerk +/// doesn't take for a block. +fn is_plain_block(block: &BlockNode) -> bool { + block.parameters().is_none_or(|parameters| { + parameters.as_block_parameters_node().is_some() + }) +} + +/// What packwerk puts before the name of a class or module defined in a block +/// passed to `call`: `Foo` for `Foo.class_eval do`, and nothing for most other +/// calls. It can't name one in a block passed with `&.`. +fn block_namespace(call: &CallNode) -> Option> { + let block = call.block()?.as_block_node()?; + if !is_plain_block(&block) { + return None; + } + if call.is_safe_navigation() { + return Some(Err(ParseError::Metaprogramming)); + } + if call.name().as_slice() != b"class_eval" { + return None; + } + let receiver = call.receiver()?; + if let Some(constant) = receiver.as_constant_read_node() { + return Some(Ok(constant_name(&constant.name()))); + } + let path = receiver.as_constant_path_node()?; + Some(packwerk_const_path_name(path.parent(), path.name())) +} + +/// `Class.new` or `Module.new`, with or without a block, as packwerk sees it +fn creates_module(node: &Node) -> bool { + let Some(call) = node.as_call_node() else { + return false; + }; + let class_or_module = call + .receiver() + .and_then(|receiver| receiver.as_constant_read_node()) + .is_some_and(|receiver| { + matches!(receiver.name().as_slice(), b"Class" | b"Module") + }); + let block = call.block().and_then(|block| block.as_block_node()); + class_or_module + && call.name().as_slice() == b"new" + && !call.is_safe_navigation() + && block.is_none_or(|block| is_plain_block(&block)) +} + +/// As `fetch_const_path_name`, but naming a path from `self` from the top +/// level, as packwerk does, so `self::Foo` is `::Foo` +fn packwerk_const_path_name( + parent: Option, + name: Option, +) -> Result { + let name = name.ok_or(ParseError::Metaprogramming)?; + let parent_namespace = match parent { + Some(parent) if parent.as_self_node().is_some() => String::new(), + Some(parent) => match parent.as_constant_path_node() { + Some(path) => packwerk_const_path_name(path.parent(), path.name())?, + None => fetch_const_name(&parent)?, + }, + None => String::new(), + }; + Ok(format!("{}::{}", parent_namespace, constant_name(&name))) } impl<'pr> Visit<'pr> for ReferenceCollector<'_> { @@ -131,20 +243,22 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &location, ); - let name = definition.fully_qualified_name.to_owned(); let namespace_path = self.current_namespaces.to_owned(); self.definitions.push(definition); // Packwerk also considers a definition to be a "reference" - self.references.push(UnresolvedReference { - name, - namespace_path, - location, - }); + if let Some(name) = self.definition_reference_name(&namespace) { + self.references.push(UnresolvedReference { + name, + namespace_path, + 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 // more efficient? + self.definition_namespaces.push(Ok(namespace.clone())); self.current_namespaces.push(namespace); if let Some(inner) = node.body() { @@ -152,6 +266,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } self.current_namespaces.pop(); + self.definition_namespaces.pop(); self.superclasses.pop(); } @@ -170,10 +285,58 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } } - ruby_prism::visit_call_node(self, node); + // In packwerk's tree, the block holds the call, so a class defined in + // the receiver or arguments is in the block too + self.visit_in_definition_namespace( + block_namespace(node), + |collector| ruby_prism::visit_call_node(collector, node), + ); } - visit_constant_assignments!(); + // packwerk can't name a class or module defined in a block passed to + // `super` + fn visit_super_node(&mut self, node: &SuperNode<'pr>) { + let block = node.block().and_then(|block| block.as_block_node()); + let namespace = block + .filter(is_plain_block) + .map(|_| Err(ParseError::Metaprogramming)); + self.visit_in_definition_namespace(namespace, |collector| { + ruby_prism::visit_super_node(collector, node) + }); + } + + fn visit_forwarding_super_node(&mut self, node: &ForwardingSuperNode<'pr>) { + let namespace = node + .block() + .filter(is_plain_block) + .map(|_| Err(ParseError::Metaprogramming)); + self.visit_in_definition_namespace(namespace, |collector| { + ruby_prism::visit_forwarding_super_node(collector, node) + }); + } + + visit_constant_assignments!(except_writes); + + fn visit_constant_write_node(&mut self, node: &ConstantWriteNode<'pr>) { + let name = constant_name(&node.name()); + self.define_constant(Ok(name.clone()), &node.location()); + self.visit_assigned_value(Ok(name), &node.value()); + } + + fn visit_constant_path_write_node( + &mut self, + node: &ConstantPathWriteNode<'pr>, + ) { + let target = node.target(); + self.define_constant( + fetch_const_path_name(target.parent(), target.name()), + &node.location(), + ); + self.visit_assigned_value( + packwerk_const_path_name(target.parent(), target.name()), + &node.value(), + ); + } fn visit_module_node(&mut self, node: &ModuleNode<'pr>) { // A module whose name isn't a constant, as `module self::Foo`, or one @@ -192,20 +355,22 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { &location, ); - let name = definition.fully_qualified_name.to_owned(); let namespace_path = self.current_namespaces.to_owned(); self.definitions.push(definition); // Packwerk also considers a definition to be a "reference" - self.references.push(UnresolvedReference { - name, - namespace_path, - location, - }); + if let Some(name) = self.definition_reference_name(&namespace) { + self.references.push(UnresolvedReference { + name, + namespace_path, + 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 // more efficient? + self.definition_namespaces.push(Ok(namespace.clone())); self.current_namespaces.push(namespace); if let Some(inner) = node.body() { @@ -213,6 +378,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { } self.current_namespaces.pop(); + self.definition_namespaces.pop(); } fn visit_constant_read_node(&mut self, node: &ConstantReadNode<'pr>) { @@ -273,6 +439,7 @@ fn process( let mut collector = ReferenceCollector { references: vec![], current_namespaces: vec![], + definition_namespaces: vec![], definitions: vec![], line_col_lookup: lookup, in_superclass: false, diff --git a/src/packs/parsing/ruby/parse_utils.rs b/src/packs/parsing/ruby/parse_utils.rs index 42f8e619..ba6c289a 100644 --- a/src/packs/parsing/ruby/parse_utils.rs +++ b/src/packs/parsing/ruby/parse_utils.rs @@ -300,7 +300,8 @@ pub fn is_plain_call(node: &CallNode) -> bool { /// The `Visit` methods for the nodes that assign a constant, for an /// `impl<'pr> Visit<'pr>` whose type has a `define_constant(name, location)` /// method. They visit only the value assigned, so neither the constant nor its -/// scope is a reference. +/// scope is a reference. With `except_writes`, they leave out `Foo = 1` and +/// `Foo::Bar = 1`, for a type that visits those itself. macro_rules! visit_constant_assignments { () => { fn visit_constant_write_node( @@ -331,6 +332,11 @@ macro_rules! visit_constant_assignments { self.visit(&node.value()); } + $crate::packs::parsing::ruby::parse_utils::visit_constant_assignments!( + except_writes + ); + }; + (except_writes) => { fn visit_constant_or_write_node( &mut self, node: &ruby_prism::ConstantOrWriteNode<'pr>, diff --git a/tests/list_references_test.rs b/tests/list_references_test.rs index 498e2136..6846a7f1 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -465,6 +465,62 @@ fn test_list_references_to_a_class_defined_from_the_top_level_in_a_namespace( Ok(()) } +#[test] +fn test_list_references_to_a_class_defined_in_class_eval_or_class_new( +) -> Result<(), Box> { + let fixture = common::Fixture::new("simple_app"); + for (path, contents) in [ + ("aa.rb", "module Aa\nend\n"), + ("aa/bb.rb", "module Aa::Bb\nend\n"), + ] { + let path = fixture.path(&format!("packs/bar/app/services/{}", path)); + fs::create_dir_all(path.parent().unwrap())?; + fs::write(path, contents)?; + } + // As in packwerk, a class defined in `Bb.class_eval do` or + // `Bb = Class.new do` is named as one in `Bb`, so the parent of + // `::Aa::Bb::::Cee` is `::Aa`, and that of `::Aa::Bb::Cc::::Dee` is + // `::Aa::Bb` + let class_eval = "packs/foo/app/services/foo_class_eval.rb"; + fs::write( + fixture.path(class_eval), + "module Aa\n Bb.class_eval do\n class ::Cee\n end\n \ + class Dee\n end\n end\nend\n", + )?; + let class_new = "packs/foo/app/services/foo_class_new.rb"; + fs::write( + fixture.path(class_new), + "module Aa\n Bb = Class.new do\n class Cee\n end\n end\nend\n", + )?; + let nested = "packs/foo/app/services/foo_nested.rb"; + fs::write( + fixture.path(nested), + "module Aa\n module Bb\n Cc.class_eval do\n class ::Dee\n \ + end\n end\n end\nend\n", + )?; + + assert_eq!( + list_references(&fixture, &["list-references"]), + json!({ + "references": [ + bar_from_foo(), + baz_from_foo(), + to_bar_pack(class_eval, Some((1, 7)), "::Aa"), + to_bar_pack(class_eval, Some((2, 2)), "::Aa::Bb"), + to_bar_pack(class_eval, Some((3, 10)), "::Aa::Bb::::Cee"), + to_bar_pack(class_eval, Some((5, 10)), "::Aa::Bb::Dee"), + to_bar_pack(class_new, Some((1, 7)), "::Aa"), + to_bar_pack(class_new, Some((3, 10)), "::Aa::Bb::Cee"), + to_bar_pack(nested, Some((1, 7)), "::Aa"), + to_bar_pack(nested, Some((2, 9)), "::Aa::Bb"), + to_bar_pack(nested, Some((4, 12)), "::Aa::Bb::Cc::::Dee"), + ] + }) + ); + + Ok(()) +} + #[test] fn test_list_references_after_a_class_defined_from_the_top_level( ) -> Result<(), Box> {