Follow up #82: look up associations as Rails does, name constants through Object, and match packwerk on :: names and class_eval

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

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<UnresolvedReference>,
     pub definitions: Vec<ParsedDefinition>,
+    /// 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<UnresolvedAssociation>,
+    /// With `--experimental-parser`
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    pub inclusions: Vec<Inclusion>,
+    /// 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<String>,
     /// Never cached, since a file with a syntax error isn't
     #[serde(skip)]
     pub syntax_error: Option<SyntaxError>,
@@ -295,17 +314,11 @@ pub(crate) fn list_definitions(
     ambiguous: bool,
 ) -> anyhow::Result<()> {
     let constant_resolver = if configuration.experimental_parser {
-        let processed_files: Vec<ProcessedFile> = 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");

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 {

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<String> {
     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<String> = 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 {}",

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<ProcessedFile> = 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();

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()
     }
 }

src/packs/parsing/erb/packwerk/parser.rs

@@ -45,5 +45,6 @@ pub(crate) fn process_from_contents(
             line: None,
             ..error
         }),
+        ..ProcessedFile::default()
     }
 }

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<String>,
+}
+
 #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Default)]
 pub struct Range {
     pub start_row: usize,