Merge pull request #931 from Shopify/ar-constant-visibility-diagnostics

Distinguish public_constant from private_constant in diagnostics

Author: alexcrocha

rust/rubydex/src/diagnostic.rs

@@ -103,7 +103,7 @@ rules! {
     DynamicSingletonDefinition;
     DynamicAncestor;
     TopLevelMixinSelf;
-    InvalidPrivateConstant;
+    InvalidConstantVisibility;
     InvalidMethodVisibility;
 
     // Resolution

rust/rubydex/src/indexing/ruby_indexer.rs

@@ -1180,10 +1180,11 @@ impl<'a> RubyIndexer<'a> {
 
     fn handle_constant_visibility(&mut self, node: &ruby_prism::CallNode, visibility: Visibility) {
         let receiver = node.receiver();
+        let call_name = String::from_utf8_lossy(node.name().as_slice());
 
-        let receiver_name_id = match receiver {
+        let receiver_name_id = match &receiver {
             Some(ruby_prism::Node::ConstantPathNode { .. } | ruby_prism::Node::ConstantReadNode { .. }) => {
-                self.index_constant_reference(&receiver.unwrap(), true)
+                self.index_constant_reference(receiver.as_ref().unwrap(), true)
             }
             Some(ruby_prism::Node::SelfNode { .. }) | None => match self.nesting_stack.last() {
                 Some(Nesting::Method(_)) => {
@@ -1192,20 +1193,20 @@ impl<'a> RubyIndexer<'a> {
                 }
                 None => {
                     self.local_graph.add_diagnostic(
-                        Rule::InvalidPrivateConstant,
+                        Rule::InvalidConstantVisibility,
                         Offset::from_prism_location(&node.location()),
-                        "Private constant called at top level".to_string(),
+                        format!("`{call_name}` called at top level"),
                     );
                     self.visit_call_node_parts(node);
                     return;
                 }
                 _ => None,
             },
-            _ => {
+            Some(other) => {
                 self.local_graph.add_diagnostic(
-                    Rule::InvalidPrivateConstant,
-                    Offset::from_prism_location(&node.location()),
-                    "Dynamic receiver for private constant".to_string(),
+                    Rule::InvalidConstantVisibility,
+                    Offset::from_prism_location(&other.location()),
+                    format!("Dynamic receiver for `{call_name}`"),
                 );
                 self.visit_call_node_parts(node);
                 return;
@@ -1219,9 +1220,9 @@ impl<'a> RubyIndexer<'a> {
         for argument in &arguments.arguments() {
             let Some((name, location)) = Self::extract_literal_name(&argument) else {
                 self.local_graph.add_diagnostic(
-                    Rule::InvalidPrivateConstant,
+                    Rule::InvalidConstantVisibility,
                     Offset::from_prism_location(&argument.location()),
-                    "Private constant called with non-symbol argument".to_string(),
+                    format!("`{call_name}` called with a non-literal argument"),
                 );
                 self.visit(&argument);
                 continue;

rust/rubydex/src/indexing/ruby_indexer_tests.rs

@@ -1750,20 +1750,20 @@ mod visibility_tests {
     fn index_private_constant_calls_diagnostics() {
         let context = index_source({
             "
-            private_constant :NOT_INDEXED
-            self.private_constant :NOT_INDEXED
-            foo.private_constant :NOT_INDEXED # not indexed, dynamic receiver
+            private_constant :BAR # not indexed, called at top level
+            self.private_constant :BAR # not indexed, self receiver at top level
+            foo.private_constant :BAR # not indexed, dynamic receiver
 
             module Foo
-              private_constant NOT_INDEXED, not_indexed # not indexed, not a symbol
+              private_constant SomeConst, some_method # not indexed, non-literal arguments
               private_constant # not indexed, no arguments
 
               def self.qux
-                private_constant :Bar # not indexed, dynamic
+                private_constant :BAR # not indexed, inside a method
               end
 
               def foo
-                private_constant :Bar # not indexed, dynamic
+                private_constant :BAR # not indexed, inside a method
               end
             end
             "
@@ -1772,15 +1772,52 @@ mod visibility_tests {
         assert_local_diagnostics_eq!(
             &context,
             vec![
-                "invalid-private-constant: Private constant called at top level (1:1-1:30)",
-                "invalid-private-constant: Private constant called at top level (2:1-2:35)",
-                "invalid-private-constant: Dynamic receiver for private constant (3:1-3:34)",
-                "invalid-private-constant: Private constant called with non-symbol argument (6:20-6:31)",
-                "invalid-private-constant: Private constant called with non-symbol argument (6:33-6:44)",
+                "invalid-constant-visibility: `private_constant` called at top level (1:1-1:22)",
+                "invalid-constant-visibility: `private_constant` called at top level (2:1-2:27)",
+                "invalid-constant-visibility: Dynamic receiver for `private_constant` (3:1-3:4)",
+                "invalid-constant-visibility: `private_constant` called with a non-literal argument (6:20-6:29)",
+                "invalid-constant-visibility: `private_constant` called with a non-literal argument (6:31-6:42)",
             ]
         );
 
-        assert_eq!(context.graph().definitions().len(), 3); // Foo, Foo::Qux, Foo#foo
+        assert_eq!(context.graph().definitions().len(), 3); // Foo, Foo.qux, Foo#foo
+    }
+
+    #[test]
+    fn index_public_constant_calls_diagnostics() {
+        let context = index_source({
+            "
+            public_constant :BAR # not indexed, called at top level
+            self.public_constant :BAR # not indexed, self receiver at top level
+            foo.public_constant :BAR # not indexed, dynamic receiver
+
+            module Foo
+              public_constant SomeConst, some_method # not indexed, non-literal arguments
+              public_constant # not indexed, no arguments
+
+              def self.qux
+                public_constant :BAR # not indexed, inside a method
+              end
+
+              def foo
+                public_constant :BAR # not indexed, inside a method
+              end
+            end
+            "
+        });
+
+        assert_local_diagnostics_eq!(
+            &context,
+            vec![
+                "invalid-constant-visibility: `public_constant` called at top level (1:1-1:21)",
+                "invalid-constant-visibility: `public_constant` called at top level (2:1-2:26)",
+                "invalid-constant-visibility: Dynamic receiver for `public_constant` (3:1-3:4)",
+                "invalid-constant-visibility: `public_constant` called with a non-literal argument (6:19-6:28)",
+                "invalid-constant-visibility: `public_constant` called with a non-literal argument (6:30-6:41)",
+            ]
+        );
+
+        assert_eq!(context.graph().definitions().len(), 3); // Foo, Foo.qux, Foo#foo
     }
 
     #[test]

rust/rubydex/src/operation/ruby_builder.rs

@@ -887,28 +887,29 @@ impl<'a> RubyOperationBuilder<'a> {
 
     fn handle_constant_visibility(&mut self, node: &ruby_prism::CallNode, visibility: Visibility) {
         let receiver = node.receiver();
+        let call_name = String::from_utf8_lossy(node.name().as_slice());
 
-        let receiver_name_id = match receiver {
+        let receiver_name_id = match &receiver {
             Some(ruby_prism::Node::ConstantPathNode { .. } | ruby_prism::Node::ConstantReadNode { .. }) => {
-                self.index_constant_reference(&receiver.unwrap(), true)
+                self.index_constant_reference(receiver.as_ref().unwrap(), true)
             }
             Some(ruby_prism::Node::SelfNode { .. }) | None => match self.nesting_stack.last() {
                 Some(Nesting::Method { .. }) => return,
                 None => {
                     self.add_diagnostic(
-                        Rule::InvalidPrivateConstant,
+                        Rule::InvalidConstantVisibility,
                         Offset::from_prism_location(&node.location()),
-                        "Private constant called at top level".to_string(),
+                        format!("`{call_name}` called at top level"),
                     );
                     return;
                 }
                 _ => None,
             },
-            _ => {
+            Some(other) => {
                 self.add_diagnostic(
-                    Rule::InvalidPrivateConstant,
-                    Offset::from_prism_location(&node.location()),
-                    "Dynamic receiver for private constant".to_string(),
+                    Rule::InvalidConstantVisibility,
+                    Offset::from_prism_location(&other.location()),
+                    format!("Dynamic receiver for `{call_name}`"),
                 );
                 return;
             }
@@ -921,9 +922,9 @@ impl<'a> RubyOperationBuilder<'a> {
         for argument in &arguments.arguments() {
             let Some((name, location)) = Self::extract_literal_name(&argument) else {
                 self.add_diagnostic(
-                    Rule::InvalidPrivateConstant,
+                    Rule::InvalidConstantVisibility,
                     Offset::from_prism_location(&argument.location()),
-                    "Private constant called with non-symbol argument".to_string(),
+                    format!("`{call_name}` called with a non-literal argument"),
                 );
                 continue;
             };