From 72982b6bc11fe8bee9fb2bb20e7477d906284489 Mon Sep 17 00:00:00 2001 From: Alex Rocha Date: Tue, 24 Mar 2026 20:38:16 -0700 Subject: [PATCH] Add local retroactive method visibility directives Track local retroactive visibility directives during indexing and replay them after resolution so instance method visibility reflects 'private/protected/public :foo'. Keep the initial scope conservative by diagnosing mixed literal/dynamic calls, skipping cross-URI application, and clearing resolver-owned diagnostics on re-resolve --- rust/rubydex/src/diagnostic.rs | 2 + rust/rubydex/src/indexing/local_graph.rs | 11 + rust/rubydex/src/indexing/ruby_indexer.rs | 90 +++- rust/rubydex/src/model/declaration.rs | 36 +- rust/rubydex/src/model/document.rs | 7 + rust/rubydex/src/model/graph.rs | 57 ++- rust/rubydex/src/model/visibility.rs | 61 +++ rust/rubydex/src/resolution.rs | 563 +++++++++++++++++++++- rust/rubydex/src/test_utils/graph_test.rs | 20 + 9 files changed, 804 insertions(+), 43 deletions(-) diff --git a/rust/rubydex/src/diagnostic.rs b/rust/rubydex/src/diagnostic.rs index bdd3fe8c8..d9733b418 100644 --- a/rust/rubydex/src/diagnostic.rs +++ b/rust/rubydex/src/diagnostic.rs @@ -102,7 +102,9 @@ rules! { DynamicConstantReference; DynamicSingletonDefinition; DynamicAncestor; + DynamicVisibilityDirective; TopLevelMixinSelf; // Resolution + InvalidVisibilityDirectiveTarget; } diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index 6f5bbcf99..aad13c62f 100644 --- a/rust/rubydex/src/indexing/local_graph.rs +++ b/rust/rubydex/src/indexing/local_graph.rs @@ -9,6 +9,7 @@ use crate::model::ids::{DefinitionId, NameId, ReferenceId, StringId, UriId}; use crate::model::name::{Name, NameRef}; use crate::model::references::{ConstantReference, MethodRef}; use crate::model::string_ref::StringRef; +use crate::model::visibility::VisibilityDirective; use crate::offset::Offset; type LocalGraphParts = ( @@ -20,6 +21,7 @@ type LocalGraphParts = ( IdentityHashMap, IdentityHashMap, IdentityHashMap>, + Vec, ); #[derive(Debug)] @@ -32,6 +34,7 @@ pub struct LocalGraph { constant_references: IdentityHashMap, method_references: IdentityHashMap, name_dependents: IdentityHashMap>, + visibility_directives: Vec, } impl LocalGraph { @@ -46,6 +49,7 @@ impl LocalGraph { constant_references: IdentityHashMap::default(), method_references: IdentityHashMap::default(), name_dependents: IdentityHashMap::default(), + visibility_directives: Vec::new(), } } @@ -206,6 +210,12 @@ impl LocalGraph { &self.name_dependents } + // Visibility directives + + pub fn add_visibility_directive(&mut self, directive: VisibilityDirective) { + self.visibility_directives.push(directive); + } + // Into parts #[must_use] @@ -219,6 +229,7 @@ impl LocalGraph { self.constant_references, self.method_references, self.name_dependents, + self.visibility_directives, ) } } diff --git a/rust/rubydex/src/indexing/ruby_indexer.rs b/rust/rubydex/src/indexing/ruby_indexer.rs index d46d6947f..0d898b0f2 100644 --- a/rust/rubydex/src/indexing/ruby_indexer.rs +++ b/rust/rubydex/src/indexing/ruby_indexer.rs @@ -14,7 +14,7 @@ use crate::model::document::Document; use crate::model::ids::{DefinitionId, NameId, StringId, UriId}; use crate::model::name::{Name, ParentScope}; use crate::model::references::{ConstantReference, MethodRef}; -use crate::model::visibility::Visibility; +use crate::model::visibility::{Visibility, VisibilityDirective, VisibilityDirectiveKind}; use crate::offset::Offset; use ruby_prism::{ParseResult, Visit}; @@ -1742,7 +1742,77 @@ impl Visit<'_> for RubyIndexer<'_> { self.visit_call_node_parts(node); } } - "private" | "protected" | "public" | "module_function" => { + "private" | "protected" | "public" => { + if let Some(_receiver) = node.receiver() { + self.visit_call_node_parts(node); + return; + } + + let visibility = Visibility::from_string(message.as_str()); + let offset = Offset::from_prism_location(&node.location()); + + if let Some(arguments) = node.arguments() { + let args = arguments.arguments(); + let is_literal = |arg: &ruby_prism::Node| { + matches!( + arg, + ruby_prism::Node::SymbolNode { .. } | ruby_prism::Node::StringNode { .. } + ) + }; + let has_literals = args.iter().any(|arg| is_literal(&arg)); + let only_literals = has_literals && args.iter().all(|arg| is_literal(&arg)); + + if only_literals { + // All-literal retroactive form: `private :foo, :bar` or `private "foo"` + let owner_definition_id = self.parent_nesting_id(); + let directive_kind = VisibilityDirectiveKind::SetInstanceMethodVisibility(visibility); + + for argument in &args { + let method_name = match argument { + ruby_prism::Node::SymbolNode { .. } => { + let symbol = argument.as_symbol_node().unwrap(); + symbol.value_loc().map(|loc| Self::location_to_string(&loc)) + } + ruby_prism::Node::StringNode { .. } => { + let string = argument.as_string_node().unwrap(); + Some(String::from_utf8_lossy(string.unescaped()).to_string()) + } + _ => None, + }; + + if let Some(name) = method_name { + let target_name = self.local_graph.intern_string(format!("{name}()")); + self.local_graph.add_visibility_directive(VisibilityDirective::new( + target_name, + directive_kind, + owner_definition_id, + offset.clone(), + )); + } + } + } else if has_literals { + // Mixed literal + non-literal args: `private :bar, MissingConst` + // Cannot model statically — emit diagnostic, visit all args, no directives. + self.local_graph.add_diagnostic( + Rule::DynamicVisibilityDirective, + offset, + format!("Visibility call `{message}` mixes literal and dynamic arguments"), + ); + self.visit_arguments_node(&arguments); + } else { + // Inline/scoped form: `private def foo; end` or `protected attr_reader :bar` + self.visibility_stack + .push(VisibilityModifier::new(visibility, true, offset)); + self.visit_arguments_node(&arguments); + self.visibility_stack.pop(); + } + } else { + // Flag mode: bare `private` affects all subsequent definitions + let last_visibility = self.visibility_stack.last_mut().unwrap(); + *last_visibility = VisibilityModifier::new(visibility, false, offset); + } + } + "module_function" => { if let Some(_receiver) = node.receiver() { self.visit_call_node_parts(node); return; @@ -1752,27 +1822,11 @@ impl Visit<'_> for RubyIndexer<'_> { let offset = Offset::from_prism_location(&node.location()); if let Some(arguments) = node.arguments() { - // With this case: - // - // ```ruby - // private def foo(bar); end - // ``` - // - // We push the new visibility to the stack and then pop it after visiting the arguments so it only affects the method definition. self.visibility_stack .push(VisibilityModifier::new(visibility, true, offset)); self.visit_arguments_node(&arguments); self.visibility_stack.pop(); } else { - // With this case: - // - // ```ruby - // private - // - // def foo(bar); end - // ``` - // - // We replace the current visibility with the new one so it only affects all the subsequent method definitions. let last_visibility = self.visibility_stack.last_mut().unwrap(); *last_visibility = VisibilityModifier::new(visibility, false, offset); } diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index 363194640..1a46558d1 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/rust/rubydex/src/model/declaration.rs @@ -3,6 +3,7 @@ use crate::diagnostic::Diagnostic; use crate::model::{ identity_maps::{IdentityHashMap, IdentityHashSet}, ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId}, + visibility::Visibility, }; /// A single ancestor in the linearized ancestor chain @@ -109,6 +110,8 @@ macro_rules! namespace_declaration { singleton_class_id: Option, /// Diagnostics associated with this declaration diagnostics: Vec, + /// The visibility of this declaration (default: Public) + visibility: Visibility, } impl $name { @@ -124,6 +127,7 @@ macro_rules! namespace_declaration { descendants: IdentityHashSet::default(), singleton_class_id: None, diagnostics: Vec::new(), + visibility: Visibility::Public, } } @@ -219,6 +223,8 @@ macro_rules! simple_declaration { owner_id: DeclarationId, /// Diagnostics associated with this declaration diagnostics: Vec, + /// The visibility of this declaration (default: Public) + visibility: Visibility, } impl $name { @@ -230,6 +236,7 @@ macro_rules! simple_declaration { references: IdentityHashSet::default(), owner_id, diagnostics: Vec::new(), + visibility: Visibility::Public, } } @@ -373,6 +380,15 @@ impl Declaration { pub fn clear_diagnostics(&mut self) { all_declarations!(self, it => it.diagnostics.clear()); } + + #[must_use] + pub fn visibility(&self) -> &Visibility { + all_declarations!(self, it => &it.visibility) + } + + pub fn set_visibility(&mut self, visibility: Visibility) { + all_declarations!(self, it => it.visibility = visibility); + } } #[derive(Debug)] @@ -503,25 +519,25 @@ impl Namespace { } namespace_declaration!(Class, ClassDeclaration); -assert_mem_size!(ClassDeclaration, 216); +assert_mem_size!(ClassDeclaration, 224); namespace_declaration!(Module, ModuleDeclaration); -assert_mem_size!(ModuleDeclaration, 216); +assert_mem_size!(ModuleDeclaration, 224); namespace_declaration!(SingletonClass, SingletonClassDeclaration); -assert_mem_size!(SingletonClassDeclaration, 216); +assert_mem_size!(SingletonClassDeclaration, 224); namespace_declaration!(Todo, TodoDeclaration); -assert_mem_size!(TodoDeclaration, 216); +assert_mem_size!(TodoDeclaration, 224); simple_declaration!(ConstantDeclaration); -assert_mem_size!(ConstantDeclaration, 112); +assert_mem_size!(ConstantDeclaration, 120); simple_declaration!(MethodDeclaration); -assert_mem_size!(MethodDeclaration, 112); +assert_mem_size!(MethodDeclaration, 120); simple_declaration!(GlobalVariableDeclaration); -assert_mem_size!(GlobalVariableDeclaration, 112); +assert_mem_size!(GlobalVariableDeclaration, 120); simple_declaration!(InstanceVariableDeclaration); -assert_mem_size!(InstanceVariableDeclaration, 112); +assert_mem_size!(InstanceVariableDeclaration, 120); simple_declaration!(ClassVariableDeclaration); -assert_mem_size!(ClassVariableDeclaration, 112); +assert_mem_size!(ClassVariableDeclaration, 120); simple_declaration!(ConstantAliasDeclaration); -assert_mem_size!(ConstantAliasDeclaration, 112); +assert_mem_size!(ConstantAliasDeclaration, 120); #[cfg(test)] mod tests { diff --git a/rust/rubydex/src/model/document.rs b/rust/rubydex/src/model/document.rs index 37f32a7cc..f16a921e5 100644 --- a/rust/rubydex/src/model/document.rs +++ b/rust/rubydex/src/model/document.rs @@ -87,6 +87,13 @@ impl Document { self.diagnostics.push(diagnostic); } + pub fn retain_diagnostics(&mut self, predicate: F) + where + F: FnMut(&Diagnostic) -> bool, + { + self.diagnostics.retain(predicate); + } + #[must_use] pub fn diagnostics_for_definition(&self, id: DefinitionId) -> &[Diagnostic] { self.definition_diagnostics.get(&id).map_or(&[], Vec::as_slice) diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index 26d5deaaf..87c8628ec 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -1,7 +1,7 @@ use std::collections::hash_map::Entry; use std::sync::LazyLock; -use crate::diagnostic::Diagnostic; +use crate::diagnostic::{Diagnostic, Rule}; use crate::indexing::local_graph::LocalGraph; use crate::model::declaration::{Ancestor, Declaration, Namespace}; use crate::model::definitions::Definition; @@ -12,6 +12,7 @@ use crate::model::ids::{DeclarationId, DefinitionId, NameId, ReferenceId, String use crate::model::name::{Name, NameRef, ParentScope, ResolvedName}; use crate::model::references::{ConstantReference, MethodRef}; use crate::model::string_ref::StringRef; +use crate::model::visibility::VisibilityDirective; use crate::stats; /// An entity whose validity depends on a particular `NameId`. @@ -57,6 +58,9 @@ pub struct Graph { /// Reverse index: for each `NameId`, which definitions, references, and child/nested names depend on it. /// Used during invalidation to efficiently find affected entities without scanning the full graph. name_dependents: IdentityHashMap>, + + /// Visibility directives keyed by URI for O(1) incremental removal + visibility_directives: IdentityHashMap>, } impl Graph { @@ -72,6 +76,7 @@ impl Graph { method_references: IdentityHashMap::default(), position_encoding: Encoding::default(), name_dependents: IdentityHashMap::default(), + visibility_directives: IdentityHashMap::default(), } } @@ -255,6 +260,21 @@ impl Graph { &self.documents } + pub fn add_document_diagnostic(&mut self, uri_id: UriId, diagnostic: Diagnostic) { + if let Some(document) = self.documents.get_mut(&uri_id) { + document.add_diagnostic(diagnostic); + } + } + + /// Removes all diagnostics matching `rule` from every document. Used to clear + /// resolver-produced diagnostics before re-running resolution without wiping + /// parse/index diagnostics. + pub fn clear_diagnostics_by_rule(&mut self, rule: Rule) { + for document in self.documents.values_mut() { + document.retain_diagnostics(|diagnostic| *diagnostic.rule() != rule); + } + } + /// # Panics /// /// Panics if the definition is not found @@ -372,6 +392,11 @@ impl Graph { &self.method_references } + #[must_use] + pub fn visibility_directives(&self) -> &IdentityHashMap> { + &self.visibility_directives + } + #[must_use] pub fn all_diagnostics(&self) -> Vec<&Diagnostic> { let document_diagnostics = self.documents.values().flat_map(Document::all_diagnostics); @@ -734,14 +759,24 @@ impl Graph { let uri_id = UriId::from(uri); let document = self.documents.remove(&uri_id)?; self.remove_definitions_for_document(&document); + self.remove_visibility_directives(uri_id); Some(uri_id) } /// Merges everything in `other` into this Graph. This method is meant to merge all graph representations from /// different threads, but not meant to handle updates to the existing global representation pub fn extend(&mut self, local_graph: LocalGraph) { - let (uri_id, document, definitions, strings, names, constant_references, method_references, name_dependents) = - local_graph.into_parts(); + let ( + uri_id, + document, + definitions, + strings, + names, + constant_references, + method_references, + name_dependents, + visibility_directives, + ) = local_graph.into_parts(); if self.documents.insert(uri_id, document).is_some() { debug_assert!(false, "UriId collision in global graph"); @@ -797,6 +832,13 @@ impl Graph { } } } + + if !visibility_directives.is_empty() { + self.visibility_directives + .entry(uri_id) + .or_default() + .extend(visibility_directives); + } } /// Updates the global representation with the information contained in `other`, handling deletions, insertions and @@ -808,10 +850,19 @@ impl Graph { if let Some(document) = self.documents.remove(&uri_id) { self.remove_definitions_for_document(&document); } + self.remove_visibility_directives(uri_id); self.extend(other); } + fn remove_visibility_directives(&mut self, uri_id: UriId) { + if let Some(directives) = self.visibility_directives.remove(&uri_id) { + for directive in &directives { + self.untrack_string(directive.target_name()); + } + } + } + // Removes all nodes and relationships associated to the given document. This is used to clean up stale data when a // document changes or when a document is deleted and we need to clean up the memory. // The document must already have been removed from `self.documents` before calling this. diff --git a/rust/rubydex/src/model/visibility.rs b/rust/rubydex/src/model/visibility.rs index 46ccdeed6..024863ff1 100644 --- a/rust/rubydex/src/model/visibility.rs +++ b/rust/rubydex/src/model/visibility.rs @@ -1,6 +1,9 @@ use core::fmt; use std::fmt::Display; +use super::ids::{DefinitionId, StringId}; +use crate::offset::Offset; + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Visibility { Public, @@ -9,6 +12,64 @@ pub enum Visibility { ModuleFunction, } +/// What a visibility directive does to its target. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum VisibilityDirectiveKind { + /// `private :foo` / `protected :foo` / `public :foo` + SetInstanceMethodVisibility(Visibility), +} + +/// A visibility directive recorded during discovery and executed during resolution. +/// For local members, mutates the declaration's visibility. +#[derive(Debug, Clone)] +pub struct VisibilityDirective { + /// Unqualified name of the target member + target_name: StringId, + /// What this directive does + kind: VisibilityDirectiveKind, + /// The definition ID of the owning namespace. None at top-level (owner is Object). + owner_definition_id: Option, + /// Source location (for ordering and diagnostics) + offset: Offset, +} + +impl VisibilityDirective { + #[must_use] + pub fn new( + target_name: StringId, + kind: VisibilityDirectiveKind, + owner_definition_id: Option, + offset: Offset, + ) -> Self { + Self { + target_name, + kind, + owner_definition_id, + offset, + } + } + + #[must_use] + pub fn target_name(&self) -> StringId { + self.target_name + } + + #[must_use] + pub fn kind(&self) -> VisibilityDirectiveKind { + self.kind + } + + #[must_use] + pub fn owner_definition_id(&self) -> Option { + self.owner_definition_id + } + + #[must_use] + pub fn offset(&self) -> &Offset { + &self.offset + } +} + impl Visibility { /// Parse a visibility from a string. /// diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index e8bd27fd8..3b3821ef4 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -3,6 +3,9 @@ use std::{ hash::BuildHasher, }; +use crate::diagnostic::{Diagnostic, Rule}; +use crate::offset::Offset; + use crate::model::{ declaration::{ Ancestor, Ancestors, ClassDeclaration, ClassVariableDeclaration, ConstantAliasDeclaration, ConstantDeclaration, @@ -12,8 +15,9 @@ use crate::model::{ definitions::{Definition, Mixin, Receiver}, graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID}, identity_maps::{IdentityHashBuilder, IdentityHashMap, IdentityHashSet}, - ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId}, + ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId, UriId}, name::{Name, NameRef, ParentScope}, + visibility::{Visibility, VisibilityDirective, VisibilityDirectiveKind}, }; pub enum Unit { @@ -155,6 +159,10 @@ impl<'a> Resolver<'a> { } self.handle_remaining_definitions(other_ids); + + self.graph + .clear_diagnostics_by_rule(Rule::InvalidVisibilityDirectiveTarget); + self.apply_visibility_operations(); } /// Resolves a single constant against the graph. This method is not meant to be used by the resolution phase, but by @@ -275,10 +283,36 @@ impl<'a> Resolver<'a> { /// Handle other definitions that don't require resolution, but need to have their declarations and membership created #[allow(clippy::too_many_lines)] fn handle_remaining_definitions(&mut self, other_ids: Vec) { + // Baseline visibility: last definition by source offset wins within each URI. + // Cross-URI: when multiple URIs define the same method with different flag-mode visibility, + // the result is nondeterministic (hash iteration order). Retroactive directives override + // this baseline, so the nondeterminism only affects methods with no explicit directive. + let mut baseline_visibility: IdentityHashMap<(DeclarationId, UriId), (Offset, Visibility)> = + IdentityHashMap::default(); + let track_baseline = |map: &mut IdentityHashMap<(DeclarationId, UriId), (Offset, Visibility)>, + decl_id: DeclarationId, + uri_id: UriId, + offset: Offset, + vis: Visibility| { + match map.entry((decl_id, uri_id)) { + Entry::Vacant(entry) => { + entry.insert((offset, vis)); + } + Entry::Occupied(mut entry) => { + if offset > entry.get().0 { + entry.insert((offset, vis)); + } + } + } + }; + for id in other_ids { match self.graph.definitions().get(&id).unwrap() { Definition::Method(method_definition) => { let str_id = *method_definition.str_id(); + let visibility = *method_definition.visibility(); + let def_offset = method_definition.offset().clone(); + let def_uri_id = *method_definition.uri_id(); let owner_id = match method_definition.receiver() { Some(Receiver::SelfReceiver(def_id)) => { let Some(&owner_decl_id) = self.graph.definition_id_to_declaration_id(*def_id) else { @@ -317,36 +351,77 @@ impl<'a> Resolver<'a> { } }; - self.create_declaration(str_id, id, owner_id, |name| { + let declaration_id = self.create_declaration(str_id, id, owner_id, |name| { Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) }); + + track_baseline( + &mut baseline_visibility, + declaration_id, + def_uri_id, + def_offset, + visibility, + ); } Definition::AttrAccessor(attr) => { let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else { continue; }; + let visibility = *attr.visibility(); + let def_offset = attr.offset().clone(); + let def_uri_id = *attr.uri_id(); - self.create_declaration(*attr.str_id(), id, owner_id, |name| { + let declaration_id = self.create_declaration(*attr.str_id(), id, owner_id, |name| { Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) }); + + track_baseline( + &mut baseline_visibility, + declaration_id, + def_uri_id, + def_offset, + visibility, + ); } Definition::AttrReader(attr) => { let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else { continue; }; + let visibility = *attr.visibility(); + let def_offset = attr.offset().clone(); + let def_uri_id = *attr.uri_id(); - self.create_declaration(*attr.str_id(), id, owner_id, |name| { + let declaration_id = self.create_declaration(*attr.str_id(), id, owner_id, |name| { Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) }); + + track_baseline( + &mut baseline_visibility, + declaration_id, + def_uri_id, + def_offset, + visibility, + ); } Definition::AttrWriter(attr) => { let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else { continue; }; + let visibility = *attr.visibility(); + let def_offset = attr.offset().clone(); + let def_uri_id = *attr.uri_id(); - self.create_declaration(*attr.str_id(), id, owner_id, |name| { + let declaration_id = self.create_declaration(*attr.str_id(), id, owner_id, |name| { Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) }); + + track_baseline( + &mut baseline_visibility, + declaration_id, + def_uri_id, + def_offset, + visibility, + ); } Definition::GlobalVariable(var) => { let owner_id = *OBJECT_ID; @@ -555,6 +630,12 @@ impl<'a> Resolver<'a> { } } } + + for ((declaration_id, _uri_id), (_offset, visibility)) in baseline_visibility { + if let Some(decl) = self.graph.declarations_mut().get_mut(&declaration_id) { + decl.set_visibility(visibility); + } + } } fn create_declaration( @@ -563,7 +644,8 @@ impl<'a> Resolver<'a> { definition_id: DefinitionId, owner_id: DeclarationId, declaration_builder: F, - ) where + ) -> DeclarationId + where F: FnOnce(String) -> Declaration, { let fully_qualified_name = { @@ -576,6 +658,7 @@ impl<'a> Resolver<'a> { .graph .add_declaration(definition_id, fully_qualified_name, declaration_builder); self.graph.add_member(&owner_id, declaration_id, str_id); + declaration_id } /// Resolves owner for class variables, bypassing singleton classes. Returns `None` if the owner can't be @@ -1766,6 +1849,128 @@ impl<'a> Resolver<'a> { _ => None, } } + + /// Maps a directive's owning definition to its declaration. Returns `None` when unresolvable. + fn resolve_directive_owner(&self, directive: &VisibilityDirective) -> Option { + directive.owner_definition_id().map_or(Some(*OBJECT_ID), |def_id| { + self.graph.definition_id_to_declaration_id(def_id).copied() + }) + } + + /// Replays visibility directives in deterministic URI+offset order after all declarations + /// exist. Currently handles `SetInstanceMethodVisibility` only. + fn apply_visibility_operations(&mut self) { + // Clone because we mutate the graph during replay. + let mut all_directives: Vec<(UriId, VisibilityDirective)> = self + .graph + .visibility_directives() + .iter() + .flat_map(|(&uri_id, directives)| directives.iter().map(move |directive| (uri_id, directive.clone()))) + .collect(); + + all_directives.sort_by(|(uri_a, directive_a), (uri_b, directive_b)| { + uri_a + .cmp(uri_b) + .then_with(|| directive_a.offset().cmp(directive_b.offset())) + }); + + for (uri_id, directive) in &all_directives { + let Some(owner_decl_id) = self.resolve_directive_owner(directive) else { + continue; + }; + + let VisibilityDirectiveKind::SetInstanceMethodVisibility(visibility) = directive.kind(); + self.apply_instance_method_visibility( + owner_decl_id, + directive.target_name(), + visibility, + *uri_id, + directive.offset(), + ); + } + } + + /// Mutates a local instance method's visibility. Same-URI only: requires a definition + /// preceding the directive's offset. Cross-URI directives are skipped — cross-file + /// ordering is nondeterministic and will emit diagnostics once conflict detection lands. + fn apply_instance_method_visibility( + &mut self, + owner_decl_id: DeclarationId, + target_name: StringId, + visibility: Visibility, + directive_uri: UriId, + directive_offset: &Offset, + ) { + let target_display_name = self + .graph + .strings() + .get(&target_name) + .map_or_else(|| "".to_string(), |s| s.as_str().to_string()); + + let member_decl_id = self + .graph + .declarations() + .get(&owner_decl_id) + .and_then(|decl| decl.as_namespace()) + .and_then(|ns| ns.member(&target_name)) + .copied(); + + let Some(member_id) = member_decl_id else { + // Not found locally — emit diagnostic. TODO: walk ancestors for inherited targets. + self.emit_directive_diagnostic( + directive_uri, + directive_offset, + format!("Method `{target_display_name}` does not exist on this class"), + ); + return; + }; + + // Collect definition URIs and same-URI offsets in one pass. If the declaration has + // definitions from multiple URIs, skip — cross-URI behavior requires conflict detection. + let mut seen_uris: IdentityHashSet = IdentityHashSet::default(); + let mut same_uri_offsets: Vec<&Offset> = Vec::new(); + if let Some(decl) = self.graph.declarations().get(&member_id) { + for def_id in decl.definitions() { + if let Some(def) = self.graph.definitions().get(def_id) { + seen_uris.insert(*def.uri_id()); + if *def.uri_id() == directive_uri { + same_uri_offsets.push(def.offset()); + } + } + } + } + + if seen_uris.len() > 1 { + return; + } + + let has_preceding = same_uri_offsets.iter().any(|off| *off < directive_offset); + let has_later_redefinition = same_uri_offsets.iter().any(|off| *off > directive_offset); + + if !has_preceding { + if !same_uri_offsets.is_empty() { + // The method has definitions in this URI, but all appear after the directive. + // In Ruby, this would raise NameError at runtime. + self.emit_directive_diagnostic( + directive_uri, + directive_offset, + format!("Method `{target_display_name}` is not defined before this visibility directive"), + ); + } + // If same_uri_offsets is empty, the method was defined in another URI. + // Cross-URI directive behavior is deferred — silently skip. + return; + } + + if !has_later_redefinition && let Some(decl) = self.graph.declarations_mut().get_mut(&member_id) { + decl.set_visibility(visibility); + } + } + + fn emit_directive_diagnostic(&mut self, uri_id: UriId, offset: &Offset, message: String) { + let diagnostic = Diagnostic::new(Rule::InvalidVisibilityDirectiveTarget, uri_id, offset.clone(), message); + self.graph.add_document_diagnostic(uri_id, diagnostic); + } } #[cfg(test)] @@ -5899,10 +6104,13 @@ mod todo_tests { #[cfg(test)] mod visibility { + use crate::model::visibility::Visibility; use crate::test_utils::GraphTest; + use crate::{assert_diagnostics_eq, assert_visibility_eq}; + + // --- Retroactive instance method visibility (local) --- #[test] - #[ignore = "not yet implemented"] fn making_method_private() { let mut context = GraphTest::new(); context.index_uri("file:///foo.rb", { @@ -5917,11 +6125,10 @@ mod visibility { }); context.resolve(); - // changing the visibility of an existing method modifies the visibility of the method + assert_visibility_eq!(context, "Foo#bar()", Visibility::Private); } #[test] - #[ignore = "not yet implemented"] fn making_method_public() { let mut context = GraphTest::new(); context.index_uri("file:///foo.rb", { @@ -5936,11 +6143,10 @@ mod visibility { }); context.resolve(); - // changing the visibility of an existing method modifies the visibility of the method + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); } #[test] - #[ignore = "not yet implemented"] fn making_method_protected() { let mut context = GraphTest::new(); context.index_uri("file:///foo.rb", { @@ -5955,9 +6161,192 @@ mod visibility { }); context.resolve(); - // changing the visibility of an existing method modifies the visibility of the method + assert_visibility_eq!(context, "Foo#bar()", Visibility::Protected); + } + + #[test] + fn multiple_symbols_in_one_call() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + def baz; end + + private :bar, :baz + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Private); + assert_visibility_eq!(context, "Foo#baz()", Visibility::Private); + } + + #[test] + fn last_directive_wins() { + 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_visibility_eq!(context, "Foo#bar()", Visibility::Public); } + #[test] + fn directive_overrides_flag_mode() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + private + + def bar; end + + public :bar + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + } + + #[test] + fn flag_mode_visibility_baseline() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def public_method; end + + private + + def private_method; end + + protected + + def protected_method; end + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#public_method()", Visibility::Public); + assert_visibility_eq!(context, "Foo#private_method()", Visibility::Private); + assert_visibility_eq!(context, "Foo#protected_method()", Visibility::Protected); + } + + #[test] + fn inline_private_def_visibility() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + private def bar; end + def baz; end + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Private); + assert_visibility_eq!(context, "Foo#baz()", Visibility::Public); + } + + #[test] + fn private_nonexistent_method_emits_diagnostic() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + + private :nonexistent + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + assert_diagnostics_eq!( + &context, + ["invalid-visibility-directive-target: Method `nonexistent()` does not exist on this class (4:3-4:23)"] + ); + } + + #[test] + fn mixed_literal_and_dynamic_args_emits_diagnostic() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + + private :bar, MissingConst + end + " + }); + context.resolve(); + + // Mixed calls are diagnostic-only — no directives enqueued, bar stays public. + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + assert_diagnostics_eq!( + &context, + ["dynamic-visibility-directive: Visibility call `private` mixes literal and dynamic arguments (4:3-4:29)"] + ); + } + + #[test] + fn mixed_literal_and_constant_args_emits_diagnostic() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + NAMES = [:bar] + def bar; end + + private :bar, NAMES + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + assert_diagnostics_eq!( + &context, + ["dynamic-visibility-directive: Visibility call `private` mixes literal and dynamic arguments (5:3-5:22)"] + ); + } + + #[test] + fn default_declaration_visibility_is_public() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + CONST = 1 + end + " + }); + context.resolve(); + + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + assert_visibility_eq!(context, "Foo::CONST", Visibility::Public); + assert_visibility_eq!(context, "Foo", Visibility::Public); + } + + // --- Retroactive instance method visibility (inherited) --- + #[test] #[ignore = "not yet implemented"] fn making_inherited_method_protected() { @@ -6002,6 +6391,8 @@ mod visibility { // with the modified visibility, but does not change the original one } + // --- Class method visibility --- + #[test] #[ignore = "not yet implemented"] fn making_class_method_private() { @@ -6085,6 +6476,8 @@ mod visibility { // child's singleton class with the modified visibility, but does not change the original one } + // --- Constant visibility --- + #[test] #[ignore = "not yet implemented"] fn making_constant_private() { @@ -6255,6 +6648,8 @@ mod visibility { // This results in a diagnostic saying FOO does not exist } + // --- Retroactive module_function --- + #[test] #[ignore = "not yet implemented"] fn make_module_function_through_inheritance() { @@ -6278,4 +6673,148 @@ mod visibility { // module_function :bar makes the instance method private and creates a public singleton // method on Baz. The original Foo#bar is unchanged. } + + // --- Reindex and delete: visibility directive lifecycle --- + + #[test] + fn reindexing_clears_old_visibility_directives() { + let mut context = GraphTest::new(); + + // First index: foo is private via retroactive directive + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + private :bar + end + " + }); + context.resolve(); + assert_visibility_eq!(context, "Foo#bar()", Visibility::Private); + + // Re-index same URI without the directive — bar should be public again + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + end + " + }); + context.resolve(); + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + } + + #[test] + fn deleting_document_clears_visibility_directives() { + let mut context = GraphTest::new(); + + // Index a single file with method + directive + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + private :bar + end + " + }); + context.resolve(); + assert_visibility_eq!(context, "Foo#bar()", Visibility::Private); + + // Delete the document entirely and re-add without the directive + context.delete_uri("file:///foo.rb"); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + end + " + }); + context.resolve(); + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + } + + #[test] + fn directive_before_definition_emits_diagnostic() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + private :bar + def bar; end + end + " + }); + context.resolve(); + + // In Ruby, `private :bar` before `def bar` raises NameError at runtime. + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + assert_diagnostics_eq!( + &context, + [ + "invalid-visibility-directive-target: Method `bar()` is not defined before this visibility directive (2:3-2:15)" + ] + ); + } + + #[test] + fn redefinition_after_directive_resets_visibility() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + def bar; end + private :bar + def bar; end + end + " + }); + context.resolve(); + + // The later `def bar` supersedes the directive — bar is public again. + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + } + + #[test] + fn mixed_uri_definitions_skip_directive() { + let mut context = GraphTest::new(); + context.index_uri("file:///a.rb", { + r" + class Foo + def bar; end + end + " + }); + context.index_uri("file:///b.rb", { + r" + class Foo + def bar; end + private :bar + end + " + }); + context.resolve(); + + // The declaration has definitions from two URIs. Retroactive directives are not + // applied to shared declarations — cross-URI behavior requires conflict detection. + assert_visibility_eq!(context, "Foo#bar()", Visibility::Public); + } + + #[test] + fn double_resolve_does_not_duplicate_diagnostics() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + class Foo + private :nonexistent + end + " + }); + context.resolve(); + context.resolve(); + + assert_diagnostics_eq!( + &context, + ["invalid-visibility-directive-target: Method `nonexistent()` does not exist on this class (2:3-2:23)"] + ); + } } diff --git a/rust/rubydex/src/test_utils/graph_test.rs b/rust/rubydex/src/test_utils/graph_test.rs index 000ae9da2..29556188c 100644 --- a/rust/rubydex/src/test_utils/graph_test.rs +++ b/rust/rubydex/src/test_utils/graph_test.rs @@ -562,6 +562,26 @@ macro_rules! assert_no_diagnostics { }}; } +#[cfg(test)] +#[macro_export] +macro_rules! assert_visibility_eq { + ($context:expr, $declaration_name:expr, $expected_visibility:expr) => { + let declaration = $context + .graph() + .declarations() + .get(&$crate::model::ids::DeclarationId::from($declaration_name)) + .unwrap_or_else(|| panic!("Declaration `{}` not found", $declaration_name)); + assert_eq!( + declaration.visibility(), + &$expected_visibility, + "Expected declaration `{}` to have visibility {:?}, got {:?}", + $declaration_name, + $expected_visibility, + declaration.visibility() + ); + }; +} + #[cfg(test)] mod tests { use super::*;