Name a class written from the top level as Ruby does, with --experimental-parser

CHANGELOG.md

@@ -40,6 +40,32 @@ into a symlinked directory, so it no longer finds the files or packs in one. A
 link to a directory that contains it, such as `loop -> ..`, made pks list the
 same files and packs over and over, and no longer does.
 
+### Fixes
+
+#### `--experimental-parser` names a class written from the top level
+
+With `--experimental-parser`, pks gave a class or module written from the top
+level, as `class ::Foo`, the name `::::Foo`, or `::Outer::::Foo` inside
+`module Outer`, and gave the constants defined in it the same prefix. A
+constant assigned from the top level, as `::BAR = 1` inside `module Outer`,
+was `::Outer::::BAR`. A reference to one was found only from inside another
+`class ::Foo` written the same way, and then under that name, as
+`::::Foo::BAR`. Others were missed, as was a reference written inside
+`class ::Foo` to a constant that `class Foo::Bar` defines. pks now names them
+as Ruby does, `::Foo` and `::Foo::BAR`, so it may find new references and
+violations. A violation that a `package_todo.yml` records under a `::::` name
+is reported again under the new name, and the old entry as stale, until you
+run `pks update`.
+
+A class reopened this way, as `class ::String` inside `module CoreExt`, now
+counts as defined in its pack, as a `class String` at the top level of a file
+does. So `expose-monkey-patches` lists it, and a reference to `String` from
+another pack is a reference to that pack.
+
+Without `--experimental-parser`, pks still looks up a constant written inside
+`class ::Foo` as packwerk does, so `Bar` written there is never `::Foo::Bar`.
+It's missed, or taken for a top-level `::Bar`.
+
 ## 0.7.0
 
 ### Features

src/packs/parsing/ruby/experimental.rs

@@ -416,6 +416,40 @@ end
         }
     }
 
+    #[test]
+    fn definitions_from_the_top_level() {
+        for (contents, definitions) in [
+            ("class ::Foo; def a; end; end", &["::Foo"][..]),
+            (
+                "module Outer; class ::Foo; def a; end; end; end",
+                &["::Foo"],
+            ),
+            (
+                "module Outer; class ::Foo::Bar; def a; end; end; end",
+                &["::Foo::Bar"],
+            ),
+            (
+                "module Outer; module ::Foo; BAR = 1; end; end",
+                &["::Foo::BAR"],
+            ),
+            ("module Outer; ::BAR = 1; end", &["::BAR"]),
+            (
+                "module Outer; module ::Foo; def a; end; end; end",
+                &["::Foo"],
+            ),
+            (
+                "module A; class ::B; module C; BAR = 1; end; end; end",
+                &["::B::C::BAR"],
+            ),
+            (
+                "module A; module ::B; module ::C; BAR = 1; end; end; end",
+                &["::C::BAR"],
+            ),
+        ] {
+            assert_eq!(definition_names(contents), definitions, "{}", contents);
+        }
+    }
+
     #[test]
     fn class_definition_with_private_constant() {
         let contents: String = String::from(

src/packs/parsing/ruby/experimental/constant_resolver.rs

@@ -7,7 +7,7 @@ use std::{
 
 use crate::packs::{
     constant_resolver::{ConstantDefinition, ConstantResolver},
-    parsing::ruby::namespace_calculator::combine_namespace_with_constant_name,
+    parsing::ruby::namespace_calculator::nest_constant_name,
 };
 
 #[derive(Default, Debug)]
@@ -126,17 +126,19 @@ impl ExperimentalConstantResolver {
     // ::Foo::Boo
     // ::Boo
     //
+    // A namespace written from the top level starts the name again, so in
+    // `module Foo; class ::Bar`, the path is ['Foo', '::Bar'], and `Boo` could
+    // be ::Bar::Boo, ::Foo::Boo or ::Boo, the order of `Module.nesting`.
+    //
     // We need to check each of these possibilities in order, and return the first one that exists
-    // If none of them exist, return None
+    // If none of them exist, return no definitions
     fn resolve_traversing_namespace_path<'a>(
         &'a self,
         const_name: &'a str,
         current_namespace_path: &'a [&str],
     ) -> Vec<ConstantDefinition> {
-        let fully_qualified_name_guess = combine_namespace_with_constant_name(
-            current_namespace_path,
-            const_name,
-        );
+        let fully_qualified_name_guess =
+            nest_constant_name(current_namespace_path, const_name);
 
         let definitions =
             self.constant_for_fully_qualified_name(&fully_qualified_name_guess);
@@ -172,3 +174,34 @@ impl ExperimentalConstantResolver {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn resolve_in_a_namespace_from_the_top_level() {
+        let constants =
+            ["::Foo::Bar", "::Outer::Baz"].map(|name| ConstantDefinition {
+                fully_qualified_name: name.to_owned(),
+                absolute_path_of_definition: PathBuf::from("/app/foo.rb"),
+            });
+        let resolver = ExperimentalConstantResolver::create(
+            constants.to_vec(),
+            Path::new("/app"),
+            &HashMap::new(),
+        );
+
+        // In `module Outer; class ::Foo; end; end`, as Ruby looks them up
+        for (name, resolved) in [("Bar", "::Foo::Bar"), ("Baz", "::Outer::Baz")]
+        {
+            let definitions = resolver.resolve(name, &["Outer", "::Foo"]);
+            let names: Vec<String> = definitions
+                .unwrap_or_default()
+                .into_iter()
+                .map(|definition| definition.fully_qualified_name)
+                .collect();
+            assert_eq!(names, [resolved], "{}", name);
+        }
+    }
+}

src/packs/parsing/ruby/experimental/parser.rs

@@ -1,14 +1,16 @@
 use crate::packs::file_utils::file_read_contents;
 use crate::packs::{
     parsing::{
-        ruby::parse_utils::{
-            constant_name, fetch_const_name, fetch_const_path_name,
-            first_syntax_error, get_constant_assignment_definition,
-            get_definition_from, get_reference_from_active_record_association,
-            is_plain_call, loc_to_range, visit_constant_assignments,
-            ParseError,
+        ruby::{
+            namespace_calculator::nest_constant_name,
+            parse_utils::{
+                constant_name, fetch_const_name, fetch_const_path_name,
+                first_syntax_error,
+                get_reference_from_active_record_association, is_plain_call,
+                loc_to_range, visit_constant_assignments, ParseError,
+            },
         },
-        ParsedDefinition, UnresolvedReference,
+        ParsedDefinition, Range, UnresolvedReference,
     },
     Configuration, ProcessedFile,
 };
@@ -35,18 +37,22 @@ impl ReferenceCollector<'_> {
         name: Result<String, ParseError>,
         location: &Location,
     ) {
-        let definition = get_constant_assignment_definition(
-            name,
-            location,
-            self.current_namespaces.to_owned(),
-            &self.line_col_lookup,
-        );
-
-        if let Some(definition) = definition {
+        if let Ok(name) = name {
+            let location = loc_to_range(location, &self.line_col_lookup);
+            let definition = self.definition(&name, location);
             self.definitions.push(definition);
         }
     }
 
+    fn definition(&self, name: &str, location: Range) -> ParsedDefinition {
+        let namespaces: Vec<&str> =
+            self.current_namespaces.iter().map(String::as_str).collect();
+        ParsedDefinition {
+            fully_qualified_name: nest_constant_name(&namespaces, name),
+            location,
+        }
+    }
+
     fn reference_constant(
         &mut self,
         name: Result<String, ParseError>,
@@ -97,11 +103,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> {
             &self.line_col_lookup,
         );
 
-        let definition = get_definition_from(
-            &namespace,
-            &self.current_namespaces,
-            &location,
-        );
+        let definition = self.definition(&namespace, location);
 
         // Note – is there a way to use lifetime specifiers to get rid of this and
         // just keep current namespaces as a vector of string references or something else
@@ -181,11 +183,7 @@ impl<'pr> Visit<'pr> for ReferenceCollector<'_> {
             &self.line_col_lookup,
         );
 
-        let definition = get_definition_from(
-            &namespace,
-            &self.current_namespaces,
-            &location,
-        );
+        let definition = self.definition(&namespace, location);
 
         // Note – is there a way to use lifetime specifiers to get rid of this and
         // just keep current namespaces as a vector of string references or something else

src/packs/parsing/ruby/namespace_calculator.rs

@@ -45,6 +45,24 @@ pub fn combine_namespace_with_constant_name(
     format!("::{}", fully_qualified_name_vec.join("::"))
 }
 
+/// The fully qualified name of `const_name` in `namespace_path`, as Ruby names
+/// it. A name written from the top level, as `class ::Foo` or `::BAR = 1`,
+/// doesn't nest in the namespaces around it. packwerk nests it anyway, naming
+/// `class ::Foo` `::::Foo`, or `::Outer::::Foo` inside `module Outer`, and so
+/// do the packwerk parser and the Zeitwerk resolver, which match it.
+pub fn nest_constant_name(namespace_path: &[&str], const_name: &str) -> String {
+    let mut name = String::new();
+    for part in namespace_path.iter().copied().chain([const_name]) {
+        if part.starts_with("::") {
+            name.clear();
+        } else {
+            name.push_str("::");
+        }
+        name.push_str(part);
+    }
+    name
+}
+
 pub fn possible_fully_qualified_constants(
     namespace_path: &[&str],
     const_name: &str,

tests/list_references_test.rs

@@ -362,6 +362,65 @@ enforcement_globs_ignore:
     Ok(())
 }
 
+#[test]
+fn test_list_references_to_a_class_defined_from_the_top_level(
+) -> Result<(), Box<dyn Error>> {
+    let fixture = common::Fixture::new("simple_app");
+    // The experimental parser names this `::Taco`, not `::Outer::::Taco`
+    fs::write(
+        fixture.path("packs/bar/app/services/taco.rb"),
+        "module Outer\n  class ::Taco\n    def self.make; end\n  end\nend\n",
+    )?;
+    let file = "packs/foo/app/services/foo_taco.rb";
+    fs::write(fixture.path(file), "Taco.make\n")?;
+
+    assert_eq!(
+        list_references(
+            &fixture,
+            &["--experimental-parser", "list-references"]
+        ),
+        json!({
+            "references": [
+                bar_from_foo(),
+                to_bar_pack(file, Some((1, 0)), "::Taco"),
+            ]
+        })
+    );
+
+    Ok(())
+}
+
+#[test]
+fn test_list_references_inside_a_class_defined_from_the_top_level(
+) -> Result<(), Box<dyn Error>> {
+    let fixture = common::Fixture::new("simple_app");
+    fs::write(
+        fixture.path("packs/bar/app/services/taco.rb"),
+        "module Taco\n  class Shell\n    def self.fold; end\n  end\nend\n",
+    )?;
+    let file = "packs/foo/app/services/foo_taco.rb";
+    // As in Ruby, `Shell` here is looked up in `::Taco`, not `::Outer::::Taco`
+    fs::write(
+        fixture.path(file),
+        "module Outer\n  class ::Taco\n    Shell.fold\n  end\nend\n",
+    )?;
+
+    assert_eq!(
+        list_references(
+            &fixture,
+            &["--experimental-parser", "list-references"]
+        ),
+        json!({
+            "references": [
+                bar_from_foo(),
+                to_bar_pack(file, Some((3, 4)), "::Taco::Shell"),
+            ]
+        })
+    );
+
+    Ok(())
+}
+
 /// A reference from `packs/foo` to a constant that `packs/bar` keeps private,
 /// which breaks the same two rules as `bar_from_foo`. `None` is a position in
 /// ERB.