Skip to content

Add local retroactive method visibility directives #692

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
alexcrocha wants to merge 1 commit into
base: 03-20-add_visibility_tests_for_methods_and_constants
Choose a base branch
from
Draft

Add local retroactive method visibility directives #692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions rust/rubydex/src/diagnostic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ rules! {
DynamicConstantReference;
DynamicSingletonDefinition;
DynamicAncestor;
DynamicVisibilityDirective;
TopLevelMixinSelf;

// Resolution
InvalidVisibilityDirectiveTarget;
}
11 changes: 11 additions & 0 deletions rust/rubydex/src/indexing/local_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand All @@ -20,6 +21,7 @@ type LocalGraphParts = (
IdentityHashMap<ReferenceId, ConstantReference>,
IdentityHashMap<ReferenceId, MethodRef>,
IdentityHashMap<NameId, Vec<NameDependent>>,
Vec<VisibilityDirective>,
);

#[derive(Debug)]
Expand All @@ -32,6 +34,7 @@ pub struct LocalGraph {
constant_references: IdentityHashMap<ReferenceId, ConstantReference>,
method_references: IdentityHashMap<ReferenceId, MethodRef>,
name_dependents: IdentityHashMap<NameId, Vec<NameDependent>>,
visibility_directives: Vec<VisibilityDirective>,
}

impl LocalGraph {
Expand All @@ -46,6 +49,7 @@ impl LocalGraph {
constant_references: IdentityHashMap::default(),
method_references: IdentityHashMap::default(),
name_dependents: IdentityHashMap::default(),
visibility_directives: Vec::new(),
}
}

Expand Down Expand Up @@ -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]
Expand All @@ -219,6 +229,7 @@ impl LocalGraph {
self.constant_references,
self.method_references,
self.name_dependents,
self.visibility_directives,
)
}
}
90 changes: 72 additions & 18 deletions rust/rubydex/src/indexing/ruby_indexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand Down
36 changes: 26 additions & 10 deletions rust/rubydex/src/model/declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -109,6 +110,8 @@ macro_rules! namespace_declaration {
singleton_class_id: Option<DeclarationId>,
/// Diagnostics associated with this declaration
diagnostics: Vec<Diagnostic>,
/// The visibility of this declaration (default: Public)
visibility: Visibility,
}

impl $name {
Expand All @@ -124,6 +127,7 @@ macro_rules! namespace_declaration {
descendants: IdentityHashSet::default(),
singleton_class_id: None,
diagnostics: Vec::new(),
visibility: Visibility::Public,
}
}

Expand Down Expand Up @@ -219,6 +223,8 @@ macro_rules! simple_declaration {
owner_id: DeclarationId,
/// Diagnostics associated with this declaration
diagnostics: Vec<Diagnostic>,
/// The visibility of this declaration (default: Public)
visibility: Visibility,
}

impl $name {
Expand All @@ -230,6 +236,7 @@ macro_rules! simple_declaration {
references: IdentityHashSet::default(),
owner_id,
diagnostics: Vec::new(),
visibility: Visibility::Public,
}
}

Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions rust/rubydex/src/model/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,13 @@ impl Document {
self.diagnostics.push(diagnostic);
}

pub fn retain_diagnostics<F>(&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)
Expand Down
Loading
Loading