Resolve module_function visibility

rust/rubydex/src/indexing/ruby_indexer.rs

@@ -1452,6 +1452,18 @@ impl<'a> RubyIndexer<'a> {
                 ruby_prism::Node::SymbolNode { .. } | ruby_prism::Node::StringNode { .. }
             ) {
                 self.create_method_visibility_definition(&arg, visibility, DefinitionFlags::empty());
+                if visibility == Visibility::ModuleFunction {
+                    // `module_function` also creates a public singleton method, so we emit a
+                    // second def for the singleton side. The two defs stay separate (rather than
+                    // shared) so reverse-lookup invalidation can detach `Foo#bar` and
+                    // `Foo::<Foo>#bar` independently. The `SINGLETON_METHOD_VISIBILITY` flag
+                    // routes this one to the singleton class.
+                    self.create_method_visibility_definition(
+                        &arg,
+                        visibility,
+                        DefinitionFlags::SINGLETON_METHOD_VISIBILITY,
+                    );
+                }
             } else {
                 // Unsupported arg — diagnostic + visit for side effects.
                 let arg_offset = Offset::from_prism_location(&arg.location());

rust/rubydex/src/indexing/ruby_indexer_tests.rs

@@ -2017,10 +2017,29 @@ mod visibility_tests {
 
         assert_no_local_diagnostics!(&context);
 
-        assert_definition_at!(&context, "4:20-4:23", MethodVisibility, |def| {
-            assert_def_str_eq!(&context, def, "foo()");
-            assert_eq!(def.visibility(), &Visibility::ModuleFunction);
-        });
+        let defs = context.all_definitions_at("4:20-4:23");
+        assert_eq!(defs.len(), 2, "expected two MethodVisibility defs");
+
+        let mut instance_side = None;
+        let mut singleton_side = None;
+        for def in defs {
+            let Definition::MethodVisibility(method_vis) = def else {
+                panic!("expected MethodVisibility, got {:?}", def.kind());
+            };
+            if method_vis.flags().is_singleton_method_visibility() {
+                singleton_side = Some(method_vis.as_ref());
+            } else {
+                instance_side = Some(method_vis.as_ref());
+            }
+        }
+
+        let instance_side = instance_side.expect("missing instance-side def");
+        assert_def_str_eq!(&context, instance_side, "foo()");
+        assert_eq!(instance_side.visibility(), &Visibility::ModuleFunction);
+
+        let singleton_side = singleton_side.expect("missing singleton-side def");
+        assert_def_str_eq!(&context, singleton_side, "foo()");
+        assert_eq!(singleton_side.visibility(), &Visibility::ModuleFunction);
     }
 
     #[test]

rust/rubydex/src/model/definitions.rs

@@ -834,7 +834,16 @@ impl MethodVisibilityDefinition {
 
     #[must_use]
     pub fn id(&self) -> DefinitionId {
-        DefinitionId::from(&format!("{}{}{}", *self.uri_id, self.offset.start(), *self.str_id))
+        // Flags are part of the hash because `module_function :bar` emits two
+        // `MethodVisibility` defs at the same source location, differing only
+        // in their `SINGLETON_METHOD_VISIBILITY` flag.
+        DefinitionId::from(&format!(
+            "{}{}{}{}",
+            *self.uri_id,
+            self.offset.start(),
+            *self.str_id,
+            self.flags.bits()
+        ))
     }
 
     #[must_use]

rust/rubydex/src/model/graph.rs

@@ -644,6 +644,9 @@ impl Graph {
     ///
     /// For methods, the latest definition wins. For constants, the latest
     /// `private_constant`/`public_constant` wins, otherwise `Public`.
+    ///
+    /// Methods declared via `module_function :bar` return `Private` on the instance
+    /// side (`Foo#bar`) and `Public` on the singleton side (`Foo::<Foo>#bar`).
     #[must_use]
     pub fn visibility(&self, declaration_id: &DeclarationId) -> Option<Visibility> {
         let declaration = self.declarations.get(declaration_id)?;
@@ -669,7 +672,13 @@ impl Graph {
                     };
 
                     let visibility = match definition {
-                        Definition::MethodVisibility(vis) => Some(*vis.visibility()),
+                        Definition::MethodVisibility(vis) => match *vis.visibility() {
+                            Visibility::ModuleFunction if vis.flags().is_singleton_method_visibility() => {
+                                Some(Visibility::Public)
+                            }
+                            Visibility::ModuleFunction => Some(Visibility::Private),
+                            other => Some(other),
+                        },
                         Definition::Method(method) => Some(*method.visibility()),
                         Definition::AttrAccessor(attr) => Some(*attr.visibility()),
                         Definition::AttrReader(attr) => Some(*attr.visibility()),

rust/rubydex/src/resolution.rs

@@ -16,6 +16,7 @@ use crate::model::{
     identity_maps::{IdentityHashBuilder, IdentityHashMap, IdentityHashSet},
     ids::{ConstantReferenceId, DeclarationId, DefinitionId, NameId, StringId},
     name::{Name, NameRef, ParentScope},
+    visibility::Visibility,
 };
 
 enum Outcome {
@@ -621,7 +622,7 @@ impl<'a> Resolver<'a> {
     }
 
     /// Resolves retroactive method visibility changes (`private :foo`, `protected :foo`, `public :foo`,
-    /// `private_class_method :foo`, `public_class_method :foo`).
+    /// `private_class_method :foo`, `public_class_method :foo`, `module_function :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,12 +639,24 @@ impl<'a> Resolver<'a> {
             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 is_module_function = *method_visibility.visibility() == Visibility::ModuleFunction;
 
             let Some(lexical_owner_id) = self.resolve_lexical_owner(lexical_nesting_id, id) else {
                 continue;
             };
 
-            let owner_id = if is_singleton {
+            // Two owners drive each resolution: `target_owner_id` (the namespace whose
+            // members we search for the target method) and `declaration_owner_id` (the
+            // namespace the resulting declaration belongs to). Three shapes:
+            //
+            // - `private`/`protected`/`public` (and `module_function :bar`'s instance-side):
+            //   both owners are the lexical owner.
+            // - `private_class_method`: both owners are the singleton class. Create it upfront.
+            // - `module_function :bar` singleton-side: target owner is the lexical (instance)
+            //   owner; declaration owner is the singleton class. Defer creating the singleton
+            //   class until the target is found, so a missing target doesn't leave an empty
+            //   `Foo::<Foo>`.
+            let target_owner_id = if is_singleton && !is_module_function {
                 let Some(singleton_id) = self.get_or_create_singleton_class(lexical_owner_id, true) else {
                     continue;
                 };
@@ -652,11 +665,11 @@ impl<'a> Resolver<'a> {
                 lexical_owner_id
             };
 
-            let Some(Declaration::Namespace(namespace)) = self.graph.declarations().get(&owner_id) else {
+            let Some(Declaration::Namespace(namespace)) = self.graph.declarations().get(&target_owner_id) else {
                 continue;
             };
 
-            let mut visibility_applied = false;
+            let mut target_found = false;
             let mut has_partial = false;
 
             for ancestor in namespace.ancestors() {
@@ -671,49 +684,71 @@ impl<'a> Resolver<'a> {
                             .is_some();
 
                         if has_member {
-                            // Direct member: `create_declaration`'s fully qualified name dedup attaches
-                            // this visibility definition to the existing method declaration.
-                            // Inherited: a new child-owned declaration is created.
-                            self.create_declaration(str_id, id, owner_id, |name| {
-                                Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
-                            });
-                            visibility_applied = true;
+                            target_found = true;
                             break;
                         }
                     }
                     Ancestor::Partial(_) => has_partial = true,
                 }
             }
 
-            if visibility_applied {
+            if target_found {
+                // `declaration_owner_id` only differs from `target_owner_id` for the
+                // module_function singleton-side def, where we deferred creating the singleton.
+                let declaration_owner_id = if is_singleton && is_module_function {
+                    let Some(singleton_id) = self.get_or_create_singleton_class(lexical_owner_id, true) else {
+                        continue;
+                    };
+                    singleton_id
+                } else {
+                    target_owner_id
+                };
+
+                // `create_declaration` deduplicates by fully qualified name: if a declaration
+                // with this name already exists, the visibility def attaches to it; otherwise
+                // a new declaration is created.
+                self.create_declaration(str_id, id, declaration_owner_id, |name| {
+                    Declaration::Method(Box::new(MethodDeclaration::new(name, declaration_owner_id)))
+                });
                 continue;
             }
 
             if has_partial {
                 // Method might exist on an unresolved ancestor — requeue for retry.
                 pending_work.push(Unit::Definition(id));
+                continue;
+            }
+
+            if is_module_function && is_singleton {
+                // The instance-side def for the same call owns the diagnostic; stay silent here.
+                continue;
+            }
+
+            let method_name = self.graph.strings().get(&str_id).unwrap().as_str().to_string();
+            let owner_name = self
+                .graph
+                .declarations()
+                .get(&target_owner_id)
+                .unwrap()
+                .name()
+                .to_string();
+            let diagnostic = Diagnostic::new(
+                Rule::UndefinedMethodVisibilityTarget,
+                uri_id,
+                offset,
+                format!("undefined method `{owner_name}#{method_name}` for visibility change"),
+            );
+            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 {
-                // Ancestors are fully resolved — method definitively doesn't exist.
-                let method_name = self.graph.strings().get(&str_id).unwrap().as_str().to_string();
-                let owner_name = self.graph.declarations().get(&owner_id).unwrap().name().to_string();
-                let diagnostic = Diagnostic::new(
-                    Rule::UndefinedMethodVisibilityTarget,
-                    uri_id,
-                    offset,
-                    format!("undefined method `{owner_name}#{method_name}` for visibility change"),
-                );
-                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);
-                }
+                self.graph
+                    .declarations_mut()
+                    .get_mut(&target_owner_id)
+                    .unwrap()
+                    .add_diagnostic(diagnostic);
             }
         }
 

rust/rubydex/src/resolution_tests.rs

@@ -5501,4 +5501,142 @@ mod visibility_resolution_tests {
         assert_visibility_eq!(context, "Foo::<Foo>#b()", Visibility::Private);
         assert_visibility_eq!(context, "Foo::<Foo>#c()", Visibility::Public);
     }
+
+    #[test]
+    fn inline_module_function() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            module Foo
+              module_function def bar; end
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo#bar()", Visibility::Private);
+        assert_visibility_eq!(context, "Foo::<Foo>#bar()", Visibility::Public);
+    }
+
+    #[test]
+    fn retroactive_module_function() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            module Foo
+              def bar; end
+              module_function :bar
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo#bar()", Visibility::Private);
+        assert_visibility_eq!(context, "Foo::<Foo>#bar()", Visibility::Public);
+
+        // Instance: Method def + MethodVisibility def
+        assert_declaration_definitions_count_eq!(context, "Foo#bar()", 2);
+        // Singleton companion: only the MethodVisibility def is attached
+        assert_declaration_definitions_count_eq!(context, "Foo::<Foo>#bar()", 1);
+    }
+
+    #[test]
+    fn retroactive_module_function_undefined_target_emits_single_diagnostic() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            module Foo
+              module_function :missing
+            end
+            ",
+        );
+        context.resolve();
+
+        // The singleton-side def stays silent; only the instance-side def emits.
+        assert_diagnostics_eq!(
+            context,
+            &["undefined-method-visibility-target: undefined method `Foo#missing()` for visibility change (2:20-2:27)"]
+        );
+        assert_declaration_does_not_exist!(context, "Foo::<Foo>#missing()");
+        assert_declaration_does_not_exist!(context, "Foo::<Foo>");
+    }
+
+    #[test]
+    fn retroactive_module_function_singleton_companion_cleared_when_removed_multi_file() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///a.rb",
+            r"
+            module Foo
+              def bar; end
+              module_function :bar
+            end
+            ",
+        );
+        context.index_uri(
+            "file:///b.rb",
+            r"
+            module Foo
+              def baz; end
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_visibility_eq!(context, "Foo::<Foo>#bar()", Visibility::Public);
+
+        // Re-index a.rb without `module_function :bar`. Foo still has b.rb's Module def, so the
+        // namespace stays alive and no cascade through Foo's singleton class runs. The singleton
+        // companion must still be cleaned up via reverse-lookup invalidation of its own def.
+        context.index_uri(
+            "file:///a.rb",
+            r"
+            module Foo
+              def bar; end
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo#bar()", Visibility::Public);
+        assert_declaration_does_not_exist!(context, "Foo::<Foo>#bar()");
+    }
+
+    #[test]
+    fn retroactive_module_function_singleton_companion_cleared_when_removed() {
+        let mut context = GraphTest::new();
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            module Foo
+              def bar; end
+              module_function :bar
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_visibility_eq!(context, "Foo::<Foo>#bar()", Visibility::Public);
+
+        context.index_uri(
+            "file:///foo.rb",
+            r"
+            module Foo
+              def bar; end
+            end
+            ",
+        );
+        context.resolve();
+
+        assert_no_diagnostics!(&context);
+        assert_visibility_eq!(context, "Foo#bar()", Visibility::Public);
+        assert_declaration_does_not_exist!(context, "Foo::<Foo>#bar()");
+        assert_declaration_does_not_exist!(context, "Foo::<Foo>");
+    }
 }