Merge pull request #981 from dduugg/deadcode-fix-orphaned-private-constant

Remove orphaned `private_constant` when removing a constant

Author: KaanOzkan

Gemfile.lock

@@ -131,6 +131,7 @@ PLATFORMS
   arm64-darwin-21
   arm64-darwin-23
   arm64-darwin-24
+  arm64-darwin-25
   universal-darwin-22
   x86_64-linux
 

lib/spoom/deadcode/remover.rb

@@ -117,6 +117,7 @@ def delete_constant_assignment(context)
                Prism::ConstantPathAndWriteNode, Prism::ConstantPathOrWriteNode
             # Nesting node is an assign, it means only one constant is assigned on the line
             # so we can remove the whole assign
+            remove_constant_visibility_call(context)
             delete_node_and_comments_and_sigs(context)
             return
           end
@@ -127,6 +128,7 @@ def delete_constant_assignment(context)
           if parent_node.is_a?(Prism::ConstantWriteNode)
             # Nesting node is an assign, it means only one constant is assigned on the line
             # so we can remove the whole assign
+            remove_constant_visibility_call(parent_context)
             delete_node_and_comments_and_sigs(parent_context)
             return
           elsif parent_node.is_a?(Prism::MultiWriteNode) && parent_node.lefts.size == 1
@@ -190,6 +192,64 @@ def delete_constant_assignment(context)
           end
         end
 
+        # A dead constant is often followed by a `private_constant`/`public_constant` call naming it.
+        # That call references the now-removed constant (a load-time `NameError` if left behind), so
+        # remove the reference too: delete the whole call when the constant is its only argument, or
+        # drop just that symbol when the call lists several constants.
+        #: (NodeContext context) -> void
+        def remove_constant_visibility_call(context)
+          node = context.node
+          return unless node.is_a?(Prism::ConstantWriteNode)
+
+          name = node.name
+          call = context.next_nodes.find { |sibling| constant_visibility_call?(sibling, name) }
+          return unless call.is_a?(Prism::CallNode)
+
+          call_context = NodeContext.new(@old_source, @node_context.comments, call, context.nesting)
+          arguments = call.arguments&.arguments #: Array[Prism::Node]?
+          if arguments && arguments.size > 1
+            delete_symbol_argument(call_context, name)
+          else
+            delete_node_and_comments_and_sigs(call_context)
+          end
+        end
+
+        # Whether `node` is a bare `private_constant`/`public_constant` call listing `name`.
+        #: (Prism::Node node, Symbol name) -> bool
+        def constant_visibility_call?(node, name)
+          return false unless node.is_a?(Prism::CallNode)
+          return false unless node.receiver.nil?
+          return false unless node.name == :private_constant || node.name == :public_constant
+
+          arguments = node.arguments&.arguments
+          return false unless arguments
+
+          arguments.any? { |argument| argument.is_a?(Prism::SymbolNode) && argument.value == name.to_s }
+        end
+
+        # Drop the `:name` symbol from a `private_constant`/`public_constant` call that lists several
+        # constants, keeping the call and the other names intact.
+        #: (NodeContext context, Symbol name) -> void
+        def delete_symbol_argument(context, name)
+          arguments = T.cast(context.node, Prism::CallNode).arguments&.arguments
+          return unless arguments
+
+          index = arguments.index { |argument| argument.is_a?(Prism::SymbolNode) && argument.value == name.to_s }
+          return unless index
+
+          argument = arguments.fetch(index)
+          prev_argument = arguments[index - 1] if index.positive?
+          next_argument = arguments[index + 1]
+
+          if prev_argument && next_argument
+            replace_chars(prev_argument.location.end_offset, next_argument.location.start_offset, ", ")
+          elsif prev_argument
+            delete_chars(prev_argument.location.end_offset, argument.location.end_offset)
+          elsif next_argument
+            delete_chars(argument.location.start_offset, next_argument.location.start_offset)
+          end
+        end
+
         #: (NodeContext context) -> void
         def delete_attr_accessor(context)
           args_context = context.parent_context

rbi/spoom.rbi

@@ -1497,6 +1497,9 @@ class Spoom::Deadcode::Remover::NodeRemover
 
   private
 
+  sig { params(node: ::Prism::Node, name: ::Symbol).returns(T::Boolean) }
+  def constant_visibility_call?(node, name); end
+
   sig { params(context: ::Spoom::Deadcode::Remover::NodeContext).void }
   def delete_attr_accessor(context); end
 
@@ -1512,6 +1515,9 @@ class Spoom::Deadcode::Remover::NodeRemover
   sig { params(context: ::Spoom::Deadcode::Remover::NodeContext).void }
   def delete_node_and_comments_and_sigs(context); end
 
+  sig { params(context: ::Spoom::Deadcode::Remover::NodeContext, name: ::Symbol).void }
+  def delete_symbol_argument(context, name); end
+
   sig do
     params(
       node: ::Prism::Node,
@@ -1524,6 +1530,9 @@ class Spoom::Deadcode::Remover::NodeRemover
   sig { params(def_node: ::Prism::DefNode).returns(T.nilable(::Spoom::Deadcode::Remover::NodeContext)) }
   def modifier_call_context(def_node); end
 
+  sig { params(context: ::Spoom::Deadcode::Remover::NodeContext).void }
+  def remove_constant_visibility_call(context); end
+
   sig { params(start_char: ::Integer, end_char: ::Integer, replacement: ::String).void }
   def replace_chars(start_char, end_char, replacement); end
 

test/spoom/deadcode/remover_test.rb

@@ -170,6 +170,62 @@ class Foo
         RB
       end
 
+      def test_removes_constant_and_its_private_constant
+        res = remove(<<~RB, "BAR")
+          class Foo
+            FOO = 1
+
+            BAR = 2
+            private_constant :BAR
+
+            BAZ = 3
+          end
+        RB
+
+        assert_equal(<<~RB, res)
+          class Foo
+            FOO = 1
+
+            BAZ = 3
+          end
+        RB
+      end
+
+      def test_removes_constant_and_its_public_constant
+        res = remove(<<~RB, "BAR")
+          class Foo
+            BAR = 2
+            public_constant :BAR
+            BAZ = 3
+          end
+        RB
+
+        assert_equal(<<~RB, res)
+          class Foo
+            BAZ = 3
+          end
+        RB
+      end
+
+      def test_removes_constant_referenced_by_a_multi_private_constant
+        res = remove(<<~RB, "BAR")
+          class Foo
+            FOO = 1
+            BAR = 2
+            BAZ = 3
+            private_constant :FOO, :BAR, :BAZ
+          end
+        RB
+
+        assert_equal(<<~RB, res)
+          class Foo
+            FOO = 1
+            BAZ = 3
+            private_constant :FOO, :BAZ
+          end
+        RB
+      end
+
       def test_removes_multiline_const
         res = remove(<<~RB, "BAR")
           class Foo