Resolve retroactive method visibility changes by alexcrocha · Pull Request #738 · Shopify/rubydex

alexcrocha · 2026-04-14T03:57:45Z

This PR continues the work in #89 and follows #695, which added indexing for MethodVisibilityDefinition and left the Definition::MethodVisibility arm in resolution as a TODO.

This change resolves retroactive private, protected, and public visibility changes by deferring visibility definitions to a second pass inside handle_remaining_definitions, so methods and attrs are always declared before visibility is applied.

For inherited targets such as private :foo where foo comes from a parent class or included module, the resolver creates a child-owned MethodDeclaration, matching CRuby's behaviour where Child.instance_method(:foo).owner is Child. When ancestor resolution is partial, unresolved visibility changes are requeued for a later resolve pass.

We also add diagnostics for undefined visibility targets and introduce Graph::visibility() as a helper for querying a method declaration's resolved visibility.

Note: This PR intentionally covers retroactive private, protected, and public resolution only. module_function will come in a follow-up PR because its dual instance/singleton behaviour needs separate handling.

alexcrocha · 2026-04-14T03:57:57Z

Resolve retroactive method visibility changes #738 👈 (View in Graphite)
Move resolution tests to separate file #737
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

vinistock · 2026-04-16T14:20:31Z

+
+            let str_id = *method_visibility.str_id();
+            let uri_id = *method_visibility.uri_id();
+            let offset = method_visibility.offset().clone();


Do we need this clone?

I Derived Copy on Offset, as a new commit since it touches other areas of the codebase, and we no longer need to clone

Sorry, I was meaning to understand why we need to clone the offset in this case. I don't think we should derive Copy in Offset because that makes Rust implicitly create new copies of the structure when needed, which may end up creating unnecessary offset copies.

I'm okay with the explicit clone, I just wanted to understand why we needed it.

Ah, fair... I jumped the gun too quickly. Reverted the derive Copy.

We still need to clone here due to method_visibility being a reference borrowed from self.graph.definitions() which holds a shared borrow on self.graph, and keeping it as a reference would extend the borrow past the &mut self calls downstream, effectively blocking them

I see. However, we only need the offset in the failed case: when there's a diagnostic to create and we end up cloning the offset every time, even when there's no diagnostic.

Would it be possible to move it down and avoid cloning if there's no diagnostic?

Moving the clone down would keep method_visibility borrowed past the &mut self calls. We could refetch self.graph.definitions().get(&id) in the else branch, but I am not sure that's better than cloning here

vinistock · 2026-04-24T14:19:08Z

+
+            let str_id = *method_visibility.str_id();
+            let uri_id = *method_visibility.uri_id();
+            let offset = method_visibility.offset().clone();


I see. However, we only need the offset in the failed case: when there's a diagnostic to create and we end up cloning the offset every time, even when there's no diagnostic.

Would it be possible to move it down and avoid cloning if there's no diagnostic?

Continues #89 and follows #506, which indexed `ConstantVisibilityDefinition` and left the `Definition::ConstantVisibility` arm in resolution as a TODO. This PR resolves retroactive `private_constant` and `public_constant` calls by reusing the second-pass mechanism from #738: visibility definitions are processed in `handle_remaining_definitions` after the ancestors linearization loop terminates, so we can resolve the owner and look up the named constant reliably before we apply the visibility. Visibility is applied only to direct members of the owner. Inherited constants are rejected because both `private_constant` and `public_constant` raise `NameError` on inherited names in Ruby. When the target is missing, an `UndefinedConstantVisibilityTarget` diagnostic is emitted. `Graph::visibility()` was changed to treat constant visibility as sticky once set: reassigning a private constant in a reopened class doesn't flip it back to public. Note: we don't handle the case when an owner isn't yet in the graph or is indexed in a later resolve(). A follow-up PR will handle incremental resolution properly

Part of #89 and follows #780, which added the `SINGLETON_METHOD_VISIBILITY` bit flag and indexed singleton-flagged `MethodVisibilityDefinition`s. This PR routes singleton-flagged defs through the singleton class and folds the singleton path into the existing `resolve_method_visibilities` pass. Visibility definitions are processed after the main convergence loop, so the target method declaration is guaranteed to exist by the time we attach visibility. For inherited targets like `private_class_method :foo` where `foo` comes from a parent class's singleton, we create a child-owned `MethodDeclaration` on the child's singleton class. This matches what #738 did for instance methods, and what Ruby reports when you ask `Child.singleton_class.instance_method(:foo).owner`: ```ruby class Parent def self.foo; end end class Child < Parent private_class_method :foo end Child.singleton_class.instance_method(:foo).owner # => #<Class:Child> (Ruby copies the method to Child when visibility changes) Parent.singleton_class.instance_method(:foo).owner # => #<Class:Parent> (untouched on the parent) ``` ### Why document-scoped diagnostics for the singleton path When a singleton target doesn't resolve, the diagnostic attaches to the document, not to the singleton declaration. Walking ancestors via `get_or_create_singleton_class(Foo, ...)` can synthesize `Foo::<Foo>` as a side effect, even when it has no real members. For `class Foo; private_class_method :missing; end` where `Foo` has no other singleton methods, that synthetic singleton class only exists to host the diagnostic. Attaching to it would leak the declaration on file delete. Document-scoped clears with the file. Instance-method diagnostics keep their existing declaration scope: their owner can't be synthetic. ### Source-order processing of visibility defs Ruby applies visibility statements in the order they appear in source, so the later one wins: ```ruby class Foo def bar; end private :bar public :bar # Ruby: bar is Public end ``` This was already broken on `main` for instance methods; surfaced while wiring up the singleton path, fixed here for both. `prepare_units` sorted `definitions` and `const_refs` by `(uri_id, offset)` for determinism but left `others` (which holds the visibility defs queued by `handle_remaining_definitions`) in `IdentityHashMap` iteration order, which is bucket-hash order. So `Foo#bar` could end up `Private` because the resolver happened to apply `public` first and `private` second. Same sort applied to `others` so the override-order invariant holds for both instance and singleton paths. ### In this PR - branch on `is_singleton_method_visibility()` inside `resolve_method_visibilities`: resolve through `get_or_create_singleton_class` for the singleton path, render `"class method"` vs `"method"` in the diagnostic, attach document-scoped (singleton) vs declaration-scoped (instance) - add `Graph::find_singleton_method_visibility_declaration` and route through it from the existing `Definition::MethodVisibility` arm in `definition_to_declaration_id` when the flag is set - sort `others` by `(uri_id, offset)` in `prepare_units` so visibility overrides apply in source order

alexcrocha mentioned this pull request Apr 14, 2026

Move resolution tests to separate file #737

Merged

alexcrocha force-pushed the 04-13-resolve_retroactive_method_visibility_changes branch from e8ff8a0 to 815966b Compare April 14, 2026 04:12

alexcrocha force-pushed the 04-13-move_resolution_tests_to_separate_file branch from fe9f274 to 2dead23 Compare April 14, 2026 04:24

alexcrocha force-pushed the 04-13-resolve_retroactive_method_visibility_changes branch 2 times, most recently from 041f4a5 to 22d309e Compare April 14, 2026 04:51

alexcrocha marked this pull request as ready for review April 15, 2026 15:53

alexcrocha requested a review from a team as a code owner April 15, 2026 15:53

alexcrocha self-assigned this Apr 15, 2026

vinistock reviewed Apr 16, 2026

View reviewed changes

alexcrocha force-pushed the 04-13-resolve_retroactive_method_visibility_changes branch 2 times, most recently from ce1865b to 564d600 Compare April 17, 2026 04:35

alexcrocha force-pushed the 04-13-move_resolution_tests_to_separate_file branch from 2dead23 to 92c611d Compare April 17, 2026 04:35

vinistock reviewed Apr 17, 2026

View reviewed changes

Comment thread rust/rubydex/src/resolution.rs Outdated

Comment thread rust/rubydex/src/resolution.rs Outdated

Base automatically changed from 04-13-move_resolution_tests_to_separate_file to main April 17, 2026 21:26

alexcrocha force-pushed the 04-13-resolve_retroactive_method_visibility_changes branch from 564d600 to b555e38 Compare April 23, 2026 23:04

alexcrocha requested a review from vinistock April 23, 2026 23:49

vinistock approved these changes Apr 24, 2026

View reviewed changes

Resolve retroactive method visibility changes

09fa346

alexcrocha force-pushed the 04-13-resolve_retroactive_method_visibility_changes branch from b555e38 to 09fa346 Compare April 24, 2026 17:14

alexcrocha merged commit ce9e779 into main Apr 24, 2026
52 of 56 checks passed

alexcrocha deleted the 04-13-resolve_retroactive_method_visibility_changes branch April 24, 2026 18:15

alexcrocha mentioned this pull request Apr 29, 2026

Resolve retroactive constant visibility changes #766

Merged

alexcrocha mentioned this pull request May 5, 2026

Resolve retroactive singleton method visibility changes #781

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve retroactive method visibility changes#738

Resolve retroactive method visibility changes#738
alexcrocha merged 1 commit into
mainfrom
04-13-resolve_retroactive_method_visibility_changes

alexcrocha commented Apr 14, 2026 •

edited

Loading

Uh oh!

alexcrocha commented Apr 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

vinistock Apr 16, 2026

Uh oh!

alexcrocha Apr 17, 2026

Uh oh!

vinistock Apr 17, 2026

Uh oh!

alexcrocha Apr 23, 2026

Uh oh!

vinistock Apr 24, 2026

Uh oh!

alexcrocha Apr 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vinistock Apr 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alexcrocha commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrocha commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vinistock Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

alexcrocha Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

vinistock Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

alexcrocha Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

vinistock Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

alexcrocha Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vinistock Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexcrocha commented Apr 14, 2026 •

edited

Loading

alexcrocha commented Apr 14, 2026 •

edited

Loading