Handle mixed visibility arguments

Ruby allows mixing inline and retroactive visibility arguments in a
single call (`private def foo; end, :bar`), where both methods should
become private. The previous implementation made a binary call-level
decision based on the first argument, routing all args to either the
inline or retroactive path and silently dropping arguments that didn't
match the chosen path.

We now classify and dispatch each argument independently instead of
making a single call-level decision. Inline defs and retroactive
literal targets can coexist in the same visibility call, and invalid
arguments now emit diagnostics at their own locations instead of once
for the whole call.

This also preserves the existing `attr_*` special case only for the
sole-argument form (`private attr_reader(:foo)`). In mixed calls like
`private attr_reader(:foo), :bar`, the `attr_reader` call is no longer
treated as an inline visibility target.

Author: alexcrocha

rust/rubydex/src/indexing/ruby_indexer.rs

@@ -1260,17 +1260,7 @@ impl<'a> RubyIndexer<'a> {
         }
     }
 
-    fn should_apply_inline_visibility(args: &ruby_prism::NodeList) -> bool {
-        if args.len() != 1 {
-            return false;
-        }
-
-        let arg = args.iter().next().unwrap();
-
-        if matches!(arg, ruby_prism::Node::DefNode { .. }) {
-            return true;
-        }
-
+    fn is_attr_call(arg: &ruby_prism::Node) -> bool {
         arg.as_call_node().is_some_and(|call| {
             let receiver = call.receiver();
             let bare_or_self = receiver.is_none() || receiver.as_ref().is_some_and(|r| r.as_self_node().is_some());
@@ -1282,54 +1272,79 @@ impl<'a> RubyIndexer<'a> {
         })
     }
 
-    fn handle_retroactive_method_visibility(
+    /// Classifies each visibility argument and applies visibility left-to-right:
+    /// - `DefNode`: inline visibility (always valid)
+    /// - Sole attr_* call: inline visibility (multi-arg attr_* is unsupported — returns array)
+    /// - `SymbolNode`/`StringNode`: retroactive `MethodVisibilityDefinition`
+    /// - Anything else: per-arg diagnostic
+    fn handle_visibility_arguments(
         &mut self,
-        node: &ruby_prism::CallNode,
         arguments: &ruby_prism::ArgumentsNode,
         visibility: Visibility,
         call_offset: &Offset,
         call_name: &str,
     ) {
         let args = arguments.arguments();
+        let arg_count = args.len();
 
-        let is_literal = |arg: ruby_prism::Node| {
-            matches!(
+        for arg in &args {
+            if matches!(arg, ruby_prism::Node::DefNode { .. }) || (arg_count == 1 && Self::is_attr_call(&arg)) {
+                self.visibility_stack
+                    .push(VisibilityModifier::new(visibility, true, call_offset.clone()));
+                self.visit(&arg);
+                self.visibility_stack.pop();
+            } else if matches!(
                 arg,
                 ruby_prism::Node::SymbolNode { .. } | ruby_prism::Node::StringNode { .. }
-            )
-        };
-
-        if !args.iter().all(&is_literal) {
-            let has_any_literal = args.iter().any(is_literal);
-            let message = if has_any_literal {
-                format!("`{call_name}` called with mixed literal and non-literal arguments")
+            ) {
+                self.create_method_visibility_definition(&arg, visibility);
             } else {
-                format!("`{call_name}` called with non-literal arguments")
-            };
-
-            self.local_graph
-                .add_diagnostic(Rule::InvalidMethodVisibility, call_offset.clone(), message);
-            self.visit_arguments_node(arguments);
-            return;
+                // Unsupported arg — diagnostic + visit for side effects.
+                let arg_offset = Offset::from_prism_location(&arg.location());
+                let message = if Self::is_attr_call(&arg) {
+                    format!("`{call_name}` with `attr_*` is only supported as a single argument")
+                } else {
+                    format!("`{call_name}` called with a non-literal argument")
+                };
+                self.local_graph
+                    .add_diagnostic(Rule::InvalidMethodVisibility, arg_offset, message);
+                self.visit(&arg);
+            }
         }
+    }
 
-        // All symbols/strings — create MethodVisibilityDefinitions
-        Self::each_string_or_symbol_arg(node, |name, location| {
-            let str_id = self.local_graph.intern_string(format!("{name}()"));
-            let arg_offset = Offset::from_prism_location(&location);
-            let definition = Definition::MethodVisibility(Box::new(MethodVisibilityDefinition::new(
-                str_id,
-                visibility,
-                self.uri_id,
-                arg_offset,
-                Box::default(),
-                DefinitionFlags::empty(),
-                self.current_nesting_definition_id(),
-            )));
+    fn create_method_visibility_definition(&mut self, arg: &ruby_prism::Node, visibility: Visibility) {
+        let (name, location) = match arg {
+            ruby_prism::Node::SymbolNode { .. } => {
+                let symbol = arg.as_symbol_node().unwrap();
+                if let Some(value_loc) = symbol.value_loc() {
+                    (Self::location_to_string(&value_loc), value_loc)
+                } else {
+                    return;
+                }
+            }
+            ruby_prism::Node::StringNode { .. } => {
+                let string = arg.as_string_node().unwrap();
+                let name = String::from_utf8_lossy(string.unescaped()).to_string();
+                (name, arg.location())
+            }
+            _ => return,
+        };
 
-            let definition_id = self.local_graph.add_definition(definition);
-            self.add_member_to_current_owner(definition_id);
-        });
+        let str_id = self.local_graph.intern_string(format!("{name}()"));
+        let arg_offset = Offset::from_prism_location(&location);
+        let definition = Definition::MethodVisibility(Box::new(MethodVisibilityDefinition::new(
+            str_id,
+            visibility,
+            self.uri_id,
+            arg_offset,
+            Box::default(),
+            DefinitionFlags::empty(),
+            self.current_nesting_definition_id(),
+        )));
+
+        let definition_id = self.local_graph.add_definition(definition);
+        self.add_member_to_current_owner(definition_id);
     }
 }
 
@@ -1949,33 +1964,15 @@ impl Visit<'_> for RubyIndexer<'_> {
                 let offset = Offset::from_prism_location(&node.location());
 
                 if let Some(arguments) = node.arguments() {
-                    let args = arguments.arguments();
-
-                    if Self::should_apply_inline_visibility(&args) {
-                        // Inline/scoped form: `private def foo(bar); end`
-                        //
-                        // 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 if visibility == Visibility::ModuleFunction && !self.current_nesting_is_module() {
+                    if visibility == Visibility::ModuleFunction && !self.current_nesting_is_module() {
                         self.local_graph.add_diagnostic(
                             Rule::InvalidMethodVisibility,
                             offset,
                             "`module_function` can only be used in modules".to_string(),
                         );
                         self.visit_arguments_node(&arguments);
                     } else {
-                        // Retroactive method visibility: `private :foo`, `module_function :bar`
-                        self.handle_retroactive_method_visibility(
-                            node,
-                            &arguments,
-                            visibility,
-                            &offset,
-                            message.as_str(),
-                        );
+                        self.handle_visibility_arguments(&arguments, visibility, &offset, message.as_str());
                     }
                 } else {
                     // Flag mode: `private` with no arguments

rust/rubydex/src/indexing/ruby_indexer_tests.rs

@@ -3406,18 +3406,15 @@ mod visibility_tests {
 
         assert_local_diagnostics_eq!(
             &context,
-            vec!["invalid-method-visibility: `private` called with mixed literal and non-literal arguments (4:3-4:27)",]
+            vec!["invalid-method-visibility: `private` called with a non-literal argument (4:17-4:27)"]
         );
 
-        // No MethodVisibilityDefinition created
-        for def in context.graph().definitions().values() {
-            assert!(
-                !matches!(def, Definition::MethodVisibility(_)),
-                "should not create MethodVisibility for mixed args"
-            );
-        }
+        // :foo is a literal arg, so visibility is still applied
+        assert_definition_at!(&context, "4:12-4:15", MethodVisibility, |def| {
+            assert_def_str_eq!(&context, def, "foo()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
 
-        // Dynamic args still visited — constant reference indexed
         assert_constant_references_eq!(&context, ["SOME_CONST"]);
     }
 
@@ -3460,7 +3457,7 @@ mod visibility_tests {
 
         assert_local_diagnostics_eq!(
             &context,
-            vec!["invalid-method-visibility: `private` called with non-literal arguments (4:3-4:21)"]
+            vec!["invalid-method-visibility: `private` called with a non-literal argument (4:11-4:21)"]
         );
 
         // No MethodVisibilityDefinition created
@@ -3484,7 +3481,7 @@ mod visibility_tests {
 
         assert_local_diagnostics_eq!(
             &context,
-            vec!["invalid-method-visibility: `private` called with non-literal arguments (2:3-2:23)"]
+            vec!["invalid-method-visibility: `private` called with a non-literal argument (2:11-2:23)"]
         );
 
         // No MethodVisibilityDefinition created
@@ -3508,7 +3505,7 @@ mod visibility_tests {
 
         assert_local_diagnostics_eq!(
             &context,
-            vec!["invalid-method-visibility: `private` called with non-literal arguments (2:3-2:35)"]
+            vec!["invalid-method-visibility: `private` called with a non-literal argument (2:11-2:35)"]
         );
 
         // No MethodVisibilityDefinition or AttrReader created from that call
@@ -3548,23 +3545,17 @@ mod visibility_tests {
             ",
         );
 
+        // attr_reader(:foo) returns an array in multi-arg context, invalid for `private`
         assert_local_diagnostics_eq!(
             &context,
-            vec!["invalid-method-visibility: `private` called with mixed literal and non-literal arguments (2:3-2:34)"]
+            vec![
+                "invalid-method-visibility: `private` with `attr_*` is only supported as a single argument (2:11-2:28)"
+            ]
         );
 
-        // No MethodVisibilityDefinition created
-        for def in context.graph().definitions().values() {
-            assert!(
-                !matches!(def, Definition::MethodVisibility(_)),
-                "should not create MethodVisibility for attr_reader with extra args"
-            );
-        }
-
-        // The attr_reader(:foo) call is still visited — AttrReader for foo() is indexed
+        // foo reader still defined via side effects, but public
         assert_definition_at!(&context, "2:24-2:27", AttrReader, |def| {
             assert_def_str_eq!(&context, def, "foo()");
-            // The attr_reader is public (default) — the visibility stack was NOT pushed
             assert_eq!(def.visibility(), &Visibility::Public);
         });
     }
@@ -3613,6 +3604,130 @@ mod visibility_tests {
             );
         }
     }
+
+    #[test]
+    fn index_inline_visibility_mixed_with_retroactive() {
+        let context = index_source(
+            "
+            class Foo
+              def bar; end
+              private def foo; end, :bar
+            end
+            ",
+        );
+
+        assert_no_local_diagnostics!(&context);
+
+        assert_definition_at!(&context, "3:11-3:23", Method, |def| {
+            assert_def_str_eq!(&context, def, "foo()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+
+        assert_definition_at!(&context, "3:26-3:29", MethodVisibility, |def| {
+            assert_def_str_eq!(&context, def, "bar()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+    }
+
+    #[test]
+    fn index_inline_visibility_multiple_defs() {
+        let context = index_source(
+            "
+            class Foo
+              private def foo; end, def bar; end
+            end
+            ",
+        );
+
+        assert_no_local_diagnostics!(&context);
+
+        assert_definition_at!(&context, "2:11-2:23", Method, |def| {
+            assert_def_str_eq!(&context, def, "foo()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+
+        assert_definition_at!(&context, "2:25-2:37", Method, |def| {
+            assert_def_str_eq!(&context, def, "bar()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+    }
+
+    #[test]
+    fn index_inline_visibility_mixed_with_unsupported() {
+        let context = index_source(
+            "
+            class Foo
+              private def foo; end, CONST_A
+              private CONST_B, def bar; end
+            end
+            ",
+        );
+
+        assert_local_diagnostics_eq!(
+            &context,
+            vec![
+                "invalid-method-visibility: `private` called with a non-literal argument (2:25-2:32)",
+                "invalid-method-visibility: `private` called with a non-literal argument (3:11-3:18)",
+            ]
+        );
+
+        // Def gets visibility regardless of arg position
+        assert_definition_at!(&context, "2:11-2:23", Method, |def| {
+            assert_def_str_eq!(&context, def, "foo()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+        assert_definition_at!(&context, "3:20-3:32", Method, |def| {
+            assert_def_str_eq!(&context, def, "bar()");
+            assert_eq!(def.visibility(), &Visibility::Private);
+        });
+
+        assert_constant_references_eq!(&context, ["CONST_A", "CONST_B"]);
+    }
+
+    #[test]
+    fn index_retroactive_visibility_multiple_unsupported_args() {
+        let context = index_source(
+            "
+            class Foo
+              private CONST_A, CONST_B
+            end
+            ",
+        );
+
+        assert_local_diagnostics_eq!(
+            &context,
+            vec![
+                "invalid-method-visibility: `private` called with a non-literal argument (2:11-2:18)",
+                "invalid-method-visibility: `private` called with a non-literal argument (2:20-2:27)",
+            ]
+        );
+
+        assert_constant_references_eq!(&context, ["CONST_A", "CONST_B"]);
+    }
+
+    #[test]
+    fn index_module_function_mixed_args_in_class_is_invalid() {
+        let context = index_source(
+            "
+            class Foo
+              module_function def foo; end, :bar
+            end
+            ",
+        );
+
+        // module_function in a class is always invalid regardless of args
+        assert_local_diagnostics_eq!(
+            &context,
+            vec!["invalid-method-visibility: `module_function` can only be used in modules (2:3-2:37)"]
+        );
+
+        for def in context.graph().definitions().values() {
+            assert!(
+                !matches!(def, Definition::MethodVisibility(_)),
+                "should not create MethodVisibility for module_function in class"
+            );
+        }
+    }
 }
 
 mod attr_accessor_tests {