Merge pull request #170 from dduugg/merge-rbi-sigs

Merge sigs in RBI files with existing documentation

Author: dduugg

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## main
 
+### New Features
+
+* [#141](https://github.com/dduugg/yard-sorbet/issues/141) Merge RBI sigs into existing documentation
+
 ### Bug Fixes
 
 * Handle multiple invocations of `mixes_in_class_methods` within a class

README.md

@@ -12,6 +12,7 @@ A YARD [plugin](https://rubydoc.info/gems/yard/file/docs/GettingStarted.md#Plugi
 - Generates constant definitions from `T::Enum` enums
 - Modules marked `abstract!` or `interface!` are tagged `@abstract`
 - Modules using `mixes_in_class_methods` will attach class methods
+- Merges `sig`s in rbi files with source code documentation (rbi files must come after source code in yard configuration)
 
 ## Usage
 

lib/yard-sorbet/handlers/sig_handler.rb

@@ -10,43 +10,93 @@ class SigHandler < YARD::Handlers::Ruby::Base
       handles method_call(:sig)
       namespace_only
 
-      # These node types attached to sigs represent attr_* declarations
-      ATTR_NODE_TYPES = T.let(%i[command fcall].freeze, T::Array[Symbol])
-      private_constant :ATTR_NODE_TYPES
+      # YARD types that can have docstrings attached to them
+      Documentable = T.type_alias do
+        T.any(
+          YARD::CodeObjects::MethodObject, YARD::Parser::Ruby::MethodCallNode, YARD::Parser::Ruby::MethodDefinitionNode
+        )
+      end
+      private_constant :Documentable
 
       # Swap the method definition docstring and the sig docstring.
       # Parse relevant parts of the `sig` and include them as well.
       sig { void }
       def process
         method_node = NodeUtils.get_method_node(NodeUtils.sibling_node(statement))
-        docstring, directives = Directives.extract_directives(statement.docstring)
-        parse_sig(method_node, docstring)
-        method_node.docstring = docstring.to_raw
-        Directives.add_directives(method_node.docstring, directives)
+        case method_node
+        when YARD::Parser::Ruby::MethodDefinitionNode then process_def(method_node)
+        when YARD::Parser::Ruby::MethodCallNode then process_attr(method_node)
+        end
         statement.docstring = nil
       end
 
       private
 
-      sig { params(method_node: YARD::Parser::Ruby::AstNode, docstring: YARD::Docstring).void }
-      def parse_sig(method_node, docstring)
+      sig { params(def_node: YARD::Parser::Ruby::MethodDefinitionNode).void }
+      def process_def(def_node)
+        separator = scope == :instance && def_node.type == :def ? '#' : '.'
+        registered = YARD::Registry.at("#{namespace}#{separator}#{def_node.method_name(true)}")
+        if registered
+          parse_node(registered, registered.docstring)
+          # Since we're probably in an RBI file, delete the def node, which could otherwise erroneously override the
+          # visibility setting
+          NodeUtils.delete_node(def_node)
+        else
+          parse_node(def_node, statement.docstring)
+        end
+      end
+
+      sig { params(attr_node: YARD::Parser::Ruby::MethodCallNode).void }
+      def process_attr(attr_node)
+        return if merged_into_attr?(attr_node)
+
+        parse_node(attr_node, statement.docstring, include_params: false)
+      end
+
+      # An attr* sig can be merged into a previous attr* docstring if it is the only parameter passed to the attr*
+      # declaration. This is to avoid needing to rewrite the source code to separate merged and unmerged attr*
+      # declarations.
+      sig { params(attr_node: YARD::Parser::Ruby::MethodCallNode).returns(T::Boolean) }
+      def merged_into_attr?(attr_node)
+        names = NodeUtils.validated_attribute_names(attr_node)
+        return false if names.size != 1
+
+        attrs = namespace.attributes[scope][names[0]]
+        return false if attrs.nil? || attrs.empty?
+
+        document_attr_methods(attrs.values.compact)
+        attr_node.docstring = nil
+        true
+      end
+
+      sig { params(method_objects: T::Array[YARD::CodeObjects::MethodObject]).void }
+      def document_attr_methods(method_objects)
+        method_objects.each { parse_node(_1, _1.docstring, include_params: false) }
+      end
+
+      sig { params(attach_to: Documentable, docstring: T.nilable(String), include_params: T::Boolean).void }
+      def parse_node(attach_to, docstring, include_params: true)
+        existing_docstring = docstring.is_a?(YARD::Docstring)
+        docstring, directives = Directives.extract_directives(docstring) unless existing_docstring
+        parse_sig(docstring, include_params: include_params)
+        attach_to.docstring = docstring.to_raw
+        Directives.add_directives(attach_to.docstring, directives) unless existing_docstring
+      end
+
+      sig { params(docstring: YARD::Docstring, include_params: T::Boolean).void }
+      def parse_sig(docstring, include_params: true)
         NodeUtils.bfs_traverse(statement) do |node|
           case node.source
           when 'returns' then parse_return(node, docstring)
-          when 'params' then parse_params(method_node, node, docstring)
+          when 'params' then parse_params(node, docstring) if include_params
           when 'void' then TagUtils.upsert_tag(docstring, 'return', TagUtils::VOID_RETURN_TYPE)
           when 'abstract' then TagUtils.upsert_tag(docstring, 'abstract')
           end
         end
       end
 
-      sig do
-        params(method_node: YARD::Parser::Ruby::AstNode, node: YARD::Parser::Ruby::AstNode, docstring: YARD::Docstring)
-          .void
-      end
-      def parse_params(method_node, node, docstring)
-        return if ATTR_NODE_TYPES.include?(method_node.type)
-
+      sig { params(node: YARD::Parser::Ruby::AstNode, docstring: YARD::Docstring).void }
+      def parse_params(node, docstring)
         sibling = NodeUtils.sibling_node(node)
         sibling.dig(0, 0).each do |param|
           param_name = param.dig(0, 0)

lib/yard-sorbet/node_utils.rb

@@ -28,6 +28,11 @@ def self.bfs_traverse(node, &_blk)
       end
     end
 
+    sig { params(node: YARD::Parser::Ruby::AstNode).void }
+    def self.delete_node(node)
+      node.parent.children.delete(node)
+    end
+
     # Gets the node that a sorbet `sig` can be attached do, bypassing visisbility modifiers and the like
     sig { params(node: YARD::Parser::Ruby::AstNode).returns(SigableNode) }
     def self.get_method_node(node)
@@ -51,5 +56,17 @@ def self.sigable_node?(node)
       else false
       end
     end
+
+    # @see https://github.com/lsegal/yard/blob/main/lib/yard/handlers/ruby/attribute_handler.rb
+    #   YARD::Handlers::Ruby::AttributeHandler.validated_attribute_names
+    sig { params(attr_node: YARD::Parser::Ruby::MethodCallNode).returns(T::Array[String]) }
+    def self.validated_attribute_names(attr_node)
+      attr_node.parameters(false).map do |obj|
+        case obj
+        when YARD::Parser::Ruby::LiteralNode then obj[0][0].source
+        else raise YARD::Parser::UndocumentableError, obj.source
+        end
+      end
+    end
   end
 end

spec/data/sig_handler.rbi.txt

@@ -0,0 +1,24 @@
+module Merge
+  class A
+    sig { returns(Numeric) }
+    attr_accessor :a_foo
+
+    sig { returns(T.nilable(String)) }
+    attr_reader :a_bar
+
+    sig { params(writer: Integer).returns(Integer) }
+    attr_writer :a_baz
+
+    sig { returns(String) }
+    def foo; end
+
+    sig { params(a: Integer).returns(Float) }
+    def self.bar(a); end
+
+    sig { returns(String) }
+    def baz; end
+
+    sig { returns(Integer) }
+    def bat; end
+  end
+end

spec/data/sig_handler.txt

@@ -294,7 +294,7 @@ end
 
 class AttrSigs
   sig {returns(String)}
-  attr_accessor :my_accessor
+  attr_accessor 'my_accessor'
 
   sig {returns(Integer)}
   attr_reader :my_reader
@@ -337,3 +337,25 @@ class Nodes
   sig { returns(INT) }
   def returns_const; 1; end
 end
+
+module Merge
+  class A
+    # annotated attr_accessor
+    attr_accessor :a_foo
+
+    attr_reader :a_bar
+
+    attr_writer :a_baz
+
+    # The foo instance method for A
+    def foo; end
+
+    # The bar singleton method for A
+    def self.bar(a); end
+
+    private def baz; end
+
+    # @return the result
+    def bat; end
+  end
+end

spec/yard_sorbet/handlers/sig_handler_spec.rb

@@ -2,11 +2,67 @@
 # frozen_string_literal: true
 
 RSpec.describe YARDSorbet::Handlers::SigHandler do
-  path = File.join(File.expand_path('../../data', __dir__), 'sig_handler.txt')
-
-  before do
+  # The :all (and corresponding rubocop disable) isn't strictly necessary, but it speeds up tests considerably
+  before(:all) do # rubocop:disable RSpec/BeforeAfterAll
     YARD::Registry.clear
+    path = File.join(File.expand_path('../../data', __dir__), 'sig_handler.txt')
     YARD::Parser::SourceParser.parse(path)
+    rbi_path = File.join(File.expand_path('../../data', __dir__), 'sig_handler.rbi.txt')
+    YARD::Parser::SourceParser.parse(rbi_path)
+  end
+
+  describe 'Merging an RBI file' do
+    it 'includes docstring from original attr_accessor' do
+      expect(YARD::Registry.at('Merge::A#a_foo').docstring).to eq('annotated attr_accessor')
+    end
+
+    it 'merges attr_accessor sig' do
+      expect(YARD::Registry.at('Merge::A#a_foo').tag(:return).types).to eq(['Numeric'])
+    end
+
+    it 'includes docstring from original attr_reader' do
+      expect(YARD::Registry.at('Merge::A#a_bar').docstring).to eq('Returns the value of attribute a_bar.')
+    end
+
+    it 'merges attr_reader sig' do
+      expect(YARD::Registry.at('Merge::A#a_bar').tag(:return).types).to eq(%w[String nil])
+    end
+
+    it 'includes docstring from original attr_writer' do
+      expect(YARD::Registry.at('Merge::A#a_baz=').docstring).to eq('Sets the attribute a_baz')
+    end
+
+    it 'merges attr_writer sig' do
+      expect(YARD::Registry.at('Merge::A#a_baz=').tag(:return).types).to eq(['Integer'])
+    end
+
+    it 'includes docstring from original instance def' do
+      expect(YARD::Registry.at('Merge::A#foo').docstring).to eq('The foo instance method for A')
+    end
+
+    it 'merges instance def sig' do
+      expect(YARD::Registry.at('Merge::A#foo').tag(:return).types).to eq(['String'])
+    end
+
+    it 'includes docstring from original singleton def' do
+      expect(YARD::Registry.at('Merge::A.bar').docstring).to eq('The bar singleton method for A')
+    end
+
+    it 'merges singleton def sig' do
+      expect(YARD::Registry.at('Merge::A.bar').tag(:return).types).to eq(['Float'])
+    end
+
+    it 'preserves the visibility of the original method' do
+      expect(YARD::Registry.at('Merge::A#baz').visibility).to be(:private)
+    end
+
+    it 'merges sig return type with return tag' do
+      expect(YARD::Registry.at('Merge::A#bat').tag(:return).types).to eq(['Integer'])
+    end
+
+    it 'merges return tag comment with sig return type' do
+      expect(YARD::Registry.at('Merge::A#bat').tag(:return).text).to eq('the result')
+    end
   end
 
   describe 'attaching to method' do
@@ -240,7 +296,7 @@
       end
 
       it 'preserves visibility modifier' do
-        expect(YARD::Registry.at('CollectionSigs#fixed_hash').visibility).to eq(:protected)
+        expect(YARD::Registry.at('CollectionSigs#fixed_hash').visibility).to be(:protected)
       end
     end
 
@@ -440,4 +496,21 @@
       end
     end
   end
+
+  describe 'Unparsable sigs' do
+    before do
+      allow(log).to receive(:warn)
+      YARD::Parser::SourceParser.parse_string(<<~RUBY)
+        class Test
+          CONST = :foo
+          sig { returns(Integer) }
+          attr_reader CONST
+        end
+      RUBY
+    end
+
+    it 'warn when parsing an attr* with a constant param' do
+      expect(log).to have_received(:warn).with(/Undocumentable CONST/).twice
+    end
+  end
 end