Resolve retroactive singleton method visibility changes

rust/rubydex/src/model/graph.rs

@@ -6,7 +6,7 @@ use crate::diagnostic::Diagnostic;
 use crate::indexing::local_graph::LocalGraph;
 use crate::model::built_in::{OBJECT_ID, add_built_in_data};
 use crate::model::declaration::{Ancestor, Declaration, Namespace};
-use crate::model::definitions::{Definition, Receiver};
+use crate::model::definitions::{Definition, MethodVisibilityDefinition, Receiver};
 use crate::model::document::Document;
 use crate::model::encoding::Encoding;
 use crate::model::identity_maps::{IdentityHashMap, IdentityHashSet};
@@ -333,10 +333,15 @@ impl Graph {
                     .or_else(|| self.find_enclosing_namespace_name_id(it.lexical_nesting_id().as_ref())),
                 it.target(),
             ),
-            Definition::MethodVisibility(it) => (
-                self.find_enclosing_namespace_name_id(it.lexical_nesting_id().as_ref()),
-                it.str_id(),
-            ),
+            Definition::MethodVisibility(it) => {
+                if it.flags().is_singleton_method_visibility() {
+                    return self.find_singleton_method_visibility_declaration(it);
+                }
+                (
+                    self.find_enclosing_namespace_name_id(it.lexical_nesting_id().as_ref()),
+                    it.str_id(),
+                )
+            }
             Definition::GlobalVariable(it) => (
                 self.find_enclosing_namespace_name_id(it.lexical_nesting_id().as_ref()),
                 it.str_id(),
@@ -413,6 +418,27 @@ impl Graph {
         None
     }
 
+    /// Looks up the declaration for a singleton method visibility through the singleton class.
+    fn find_singleton_method_visibility_declaration(
+        &self,
+        definition: &MethodVisibilityDefinition,
+    ) -> Option<&DeclarationId> {
+        let nesting_name_id = self.find_enclosing_namespace_name_id(definition.lexical_nesting_id().as_ref());
+        let nesting_declaration_id = match nesting_name_id {
+            Some(name_id) => self.name_id_to_declaration_id(*name_id),
+            None => Some(&*OBJECT_ID),
+        }?;
+        let singleton_id = self
+            .declarations
+            .get(nesting_declaration_id)?
+            .as_namespace()?
+            .singleton_class()?;
+        self.declarations
+            .get(singleton_id)?
+            .as_namespace()?
+            .member(definition.str_id())
+    }
+
     /// Looks up the declaration for a `SelfReceiver` method/alias through the singleton class.
     fn find_self_receiver_declaration(&self, def_id: DefinitionId, member_str_id: StringId) -> Option<&DeclarationId> {
         let owner_decl_id = self.definition_id_to_declaration_id(def_id)?;

rust/rubydex/src/resolution.rs

@@ -604,10 +604,8 @@ impl<'a> Resolver<'a> {
                         self.graph.add_document_diagnostic(uri_id, diagnostic);
                     }
                 }
-                Definition::MethodVisibility(visibility_def) => {
-                    if !visibility_def.flags().is_singleton_method_visibility() {
-                        method_visibility_ids.push(id);
-                    }
+                Definition::MethodVisibility(_) => {
+                    method_visibility_ids.push(id);
                 }
                 Definition::Class(_)
                 | Definition::SingletonClass(_)
@@ -622,7 +620,8 @@ impl<'a> Resolver<'a> {
         self.resolve_method_visibilities(method_visibility_ids);
     }
 
-    /// Resolves retroactive method visibility changes (`private :foo`, `protected :foo`, `public :foo`).
+    /// Resolves retroactive method visibility changes (`private :foo`, `protected :foo`, `public :foo`,
+    /// `private_class_method :foo`, `public_class_method :foo`).
     ///
     /// Runs as a second pass after all methods/attrs are declared, so `private :bar` works
     /// regardless of whether `def bar` appeared before or after it in source.
@@ -638,11 +637,21 @@ impl<'a> Resolver<'a> {
             let uri_id = *method_visibility.uri_id();
             let offset = method_visibility.offset().clone();
             let lexical_nesting_id = *method_visibility.lexical_nesting_id();
+            let is_singleton = method_visibility.flags().is_singleton_method_visibility();
 
-            let Some(owner_id) = self.resolve_lexical_owner(lexical_nesting_id, id) else {
+            let Some(lexical_owner_id) = self.resolve_lexical_owner(lexical_nesting_id, id) else {
                 continue;
             };
 
+            let owner_id = if is_singleton {
+                let Some(singleton_id) = self.get_or_create_singleton_class(lexical_owner_id, true) else {
+                    continue;
+                };
+                singleton_id
+            } else {
+                lexical_owner_id
+            };
+
             let Some(Declaration::Namespace(namespace)) = self.graph.declarations().get(&owner_id) else {
                 continue;
             };
@@ -691,13 +700,20 @@ impl<'a> Resolver<'a> {
                     Rule::UndefinedMethodVisibilityTarget,
                     uri_id,
                     offset,
-                    format!("undefined method `{method_name}` for visibility change in `{owner_name}`"),
+                    format!("undefined method `{owner_name}#{method_name}` for visibility change"),
                 );
-                self.graph
-                    .declarations_mut()
-                    .get_mut(&owner_id)
-                    .unwrap()
-                    .add_diagnostic(diagnostic);
+                if is_singleton {
+                    // Document-scoped: the singleton class may be synthetic (created by this
+                    // visibility resolution) and won't be cleaned up on file delete, so attaching
+                    // the diagnostic to the declaration would leave it orphaned.
+                    self.graph.add_document_diagnostic(uri_id, diagnostic);
+                } else {
+                    self.graph
+                        .declarations_mut()
+                        .get_mut(&owner_id)
+                        .unwrap()
+                        .add_diagnostic(diagnostic);
+                }
             }
         }
 
@@ -1882,7 +1898,7 @@ impl<'a> Resolver<'a> {
                             singleton_methods.push(Unit::Definition(id));
                         }
                         _ => {
-                            others.push(id);
+                            others.push((id, (*definition.uri_id(), definition.offset())));
                         }
                     }
                 }
@@ -1922,14 +1938,15 @@ impl<'a> Resolver<'a> {
             (depths.get(name_a).unwrap(), uri_a, offset_a).cmp(&(depths.get(name_b).unwrap(), uri_b, offset_b))
         });
 
+        others.sort_unstable_by_key(|(_, key)| *key);
+
         // Definitions first, then constant refs, then singleton methods, then ancestors
         self.unit_queue.extend(definitions.into_iter().map(|(id, _)| id));
         self.unit_queue.extend(const_refs.into_iter().map(|(id, _)| id));
         self.unit_queue.extend(singleton_methods);
         self.unit_queue.extend(ancestors.into_iter().map(Unit::Ancestors));
 
-        others.shrink_to_fit();
-        others
+        others.into_iter().map(|(id, _)| id).collect()
     }
 
     /// Returns the singleton parent ID for an attached object ID. A singleton class' parent depends on what the attached

rust/rubydex/src/resolution_tests.rs

@@ -4926,6 +4926,25 @@ mod visibility_resolution_tests {
         };
     }
 
+    #[test]
+    fn retroactive_visibility_override_applies_in_source_order() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+              def bar; end
+              private :bar
+              public :bar
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo#bar()", Visibility::Public);
+    }
+
     #[test]
     fn retroactive_visibility_on_direct_method() {
         let mut context = GraphTest::new();
@@ -5066,7 +5085,7 @@ mod visibility_resolution_tests {
         assert_diagnostics_eq!(
             context,
             &[
-                "undefined-method-visibility-target: undefined method `nonexistent()` for visibility change in `Foo` (2:12-2:23)"
+                "undefined-method-visibility-target: undefined method `Foo#nonexistent()` for visibility change (2:12-2:23)"
             ]
         );
     }
@@ -5306,4 +5325,139 @@ mod visibility_resolution_tests {
 
         assert_visibility_eq!(context, "Foo::BAR", Visibility::Private);
     }
+
+    #[test]
+    fn retroactive_singleton_method_visibility_on_direct_member() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+              def self.bar; end
+              def self.baz; end
+
+              private_class_method :bar
+              private_class_method :baz
+              public_class_method :baz
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo::<Foo>#bar()", Visibility::Private);
+        assert_visibility_eq!(context, "Foo::<Foo>#baz()", Visibility::Public);
+    }
+
+    #[test]
+    fn retroactive_singleton_method_visibility_on_inherited_method() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Parent
+              def self.foo; end
+            end
+
+            class Child < Parent
+              private_class_method :foo
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_declaration_exists!(context, "Child::<Child>#foo()");
+        assert_visibility_eq!(context, "Child::<Child>#foo()", Visibility::Private);
+        assert_visibility_eq!(context, "Parent::<Parent>#foo()", Visibility::Public);
+    }
+
+    #[test]
+    fn retroactive_singleton_method_visibility_on_undefined_method_emits_diagnostic() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+              private_class_method :nonexistent
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_diagnostics_eq!(
+            context,
+            &[
+                "undefined-method-visibility-target: undefined method `Foo::<Foo>#nonexistent()` for visibility change (2:25-2:36)"
+            ]
+        );
+    }
+
+    #[test]
+    fn retroactive_singleton_method_visibility_undefined_target_diagnostic_clears_when_file_deleted() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+            end
+            ",
+        );
+        context.index_uri(
+            "file:///bad.rb",
+            r"
+            class Foo
+              private_class_method :missing
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_diagnostics_eq!(
+            context,
+            &[
+                "undefined-method-visibility-target: undefined method `Foo::<Foo>#missing()` for visibility change (2:25-2:32)"
+            ]
+        );
+
+        context.delete_uri("file:///bad.rb");
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+    }
+
+    #[test]
+    fn retroactive_singleton_method_visibility_undefined_target_diagnostic_clears_when_target_added() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+              private_class_method :missing
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_diagnostics_eq!(
+            context,
+            &[
+                "undefined-method-visibility-target: undefined method `Foo::<Foo>#missing()` for visibility change (2:25-2:32)"
+            ]
+        );
+
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            class Foo
+              def self.missing; end
+              private_class_method :missing
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo::<Foo>#missing()", Visibility::Private);
+    }
 }

test/declaration_test.rb

@@ -789,6 +789,41 @@ class Foo
     end
   end
 
+  def test_class_method_visibility_via_private_class_method
+    with_context do |context|
+      context.write!("file1.rb", <<~RUBY)
+        class Foo
+          def self.bar; end
+          private_class_method :bar
+        end
+      RUBY
+
+      graph = Rubydex::Graph.new
+      graph.index_all(context.glob("**/*.rb"))
+      graph.resolve
+
+      assert_equal(:private, graph["Foo::<Foo>#bar()"].visibility)
+    end
+  end
+
+  def test_class_method_visibility_via_public_class_method
+    with_context do |context|
+      context.write!("file1.rb", <<~RUBY)
+        class Foo
+          def self.bar; end
+          private_class_method :bar
+          public_class_method :bar
+        end
+      RUBY
+
+      graph = Rubydex::Graph.new
+      graph.index_all(context.glob("**/*.rb"))
+      graph.resolve
+
+      assert_equal(:public, graph["Foo::<Foo>#bar()"].visibility)
+    end
+  end
+
   def test_constant_alias_visibility
     with_context do |context|
       context.write!("file1.rb", <<~RUBY)