From 94a1a2f700c8a233d0184a4db4c7511fac51bc65 Mon Sep 17 00:00:00 2001 From: Alex Rocha Date: Mon, 4 May 2026 19:55:34 -0700 Subject: [PATCH] Resolve singleton method visibility --- rust/rubydex/src/model/graph.rs | 36 ++++++- rust/rubydex/src/resolution.rs | 47 +++++--- rust/rubydex/src/resolution_tests.rs | 156 ++++++++++++++++++++++++++- test/declaration_test.rb | 35 ++++++ 4 files changed, 253 insertions(+), 21 deletions(-) diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index d866fa10..c9e09f9b 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/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)?; diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index db63c23a..47015b06 100644 --- a/rust/rubydex/src/resolution.rs +++ b/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 diff --git a/rust/rubydex/src/resolution_tests.rs b/rust/rubydex/src/resolution_tests.rs index 1631dbae..0ab164d1 100644 --- a/rust/rubydex/src/resolution_tests.rs +++ b/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::#bar()", Visibility::Private); + assert_visibility_eq!(context, "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::#foo()"); + assert_visibility_eq!(context, "Child::#foo()", Visibility::Private); + assert_visibility_eq!(context, "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::#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::#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::#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::#missing()", Visibility::Private); + } } diff --git a/test/declaration_test.rb b/test/declaration_test.rb index 4a31a416..97adea06 100644 --- a/test/declaration_test.rb +++ b/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::#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::#bar()"].visibility) + end + end + def test_constant_alias_visibility with_context do |context| context.write!("file1.rb", <<~RUBY)