diff --git a/CHANGELOG.md b/CHANGELOG.md index 85c5724e..146a4786 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,82 @@ 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. + +When the class has the model's own name, as `has_many :tacos` in `class Taco` +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 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 + +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 + 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 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`. +- 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 +`pks update`. + ## 0.7.0 ### Features 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 2bc95bfd..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); } @@ -450,6 +745,46 @@ 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"], + ), + ("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", + &["::CoreExt::Object::BAR"], + ), + ] { + assert_eq!(definition_names(contents), definitions, "{}", contents); + } + } + #[test] fn class_definition_with_private_constant() { let contents: String = String::from( @@ -486,6 +821,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); } @@ -527,6 +863,7 @@ end unresolved_references, definitions, syntax_error: None, + ..ProcessedFile::default() }; assert_eq!(expected, actual); @@ -569,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 46ca51a3..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( @@ -179,29 +196,78 @@ 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"), + (&["::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"], + "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 e52abac5..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,6 +51,7 @@ impl ReferenceCollector<'_> { ) { if let Ok(name) = name { let location = loc_to_range(location, &self.line_col_lookup); + let name = self.defined_name(name); let definition = self.definition(&name, location); self.definitions.push(definition); } @@ -53,6 +66,94 @@ impl ReferenceCollector<'_> { } } + /// 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(); + 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( &mut self, name: Result, @@ -93,7 +194,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { return; } - let namespace = namespace_result.unwrap(); + let namespace = self.defined_name(namespace_result.unwrap()); if let Some(inner) = node.superclass() { self.visit(&inner); @@ -108,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; @@ -127,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>) { @@ -144,7 +246,13 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { ); if let Some(association_reference) = association_reference { - self.references.push(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); @@ -176,8 +284,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 = self.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, @@ -188,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; @@ -207,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>) { @@ -235,19 +345,30 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> { pub(crate) fn process_from_path( path: &Path, configuration: &Configuration, +) -> anyhow::Result { + process_from_path_with_objects(path, configuration, &HashSet::new()) +} + +/// 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 @@ -256,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( @@ -264,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()); @@ -272,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()); @@ -294,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/namespace_calculator.rs b/src/packs/parsing/ruby/namespace_calculator.rs index e2650c9a..6d9b6154 100644 --- a/src/packs/parsing/ruby/namespace_calculator.rs +++ b/src/packs/parsing/ruby/namespace_calculator.rs @@ -50,6 +50,11 @@ 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 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]) { @@ -60,6 +65,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 } 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 4779339c..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, @@ -318,10 +485,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 @@ -347,5 +512,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..ba6c289a 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() { @@ -289,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( @@ -320,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/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs index 6afaeac8..27281be5 100644 --- a/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs +++ b/src/packs/parsing/ruby/zeitwerk/constant_resolver.rs @@ -111,7 +111,14 @@ 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'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(); + } if split_const.len() <= 1 { return None; } @@ -209,3 +216,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"), + }]) + ); + } +} 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 7a1b8954..6846a7f1 100644 --- a/tests/list_references_test.rs +++ b/tests/list_references_test.rs @@ -421,6 +421,527 @@ 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_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> { + 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` 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\n Taco.make\nend\n\ + Taco.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((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"), + ] + }) + ); + + 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` 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), + "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", + )?; + // 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( + &fixture, + &["--experimental-parser", "list-references"] + ), + json!({ + "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"), + ] + }) + ); + + 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(()) +} + +#[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(()) +} + +#[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.