Merge pull request #992 from Shopify/at-check-todo

Also check sorbet/rbi/todo.rbi for duplicated shims when running the `check-shims` command

README.md

@@ -717,6 +717,8 @@ Options:
                                                    # Default: sorbet/rbi/shims
       [--annotations-rbi-dir=ANNOTATIONS_RBI_DIR]  # Path to annotations RBIs
                                                    # Default: sorbet/rbi/annotations
+      [--todo-rbi-file=TODO_RBI_FILE]              # Path to the generated todo RBI file
+                                                   # Default: sorbet/rbi/todo.rbi
       [--payload], [--no-payload]                  # Check shims against Sorbet's payload
                                                    # Default: true
   -c, [--config=<config file path>]                # Path to the Tapioca configuration file
@@ -792,6 +794,7 @@ check_shims:
   dsl_rbi_dir: sorbet/rbi/dsl
   shim_rbi_dir: sorbet/rbi/shims
   annotations_rbi_dir: sorbet/rbi/annotations
+  todo_rbi_file: sorbet/rbi/todo.rbi
   payload: true
 annotations:
   sources:

lib/tapioca/cli.rb

@@ -258,13 +258,15 @@ def gem(*gems)
     option :dsl_rbi_dir, type: :string, desc: "Path to DSL RBIs", default: DEFAULT_DSL_DIR
     option :shim_rbi_dir, type: :string, desc: "Path to shim RBIs", default: DEFAULT_SHIM_DIR
     option :annotations_rbi_dir, type: :string, desc: "Path to annotations RBIs", default: DEFAULT_ANNOTATIONS_DIR
+    option :todo_rbi_file, type: :string, desc: "Path to the generated todo RBI file", default: DEFAULT_TODO_FILE
     option :payload, type: :boolean, desc: "Check shims against Sorbet's payload", default: true
     def check_shims
       command = Commands::CheckShims.new(
         gem_rbi_dir: options[:gem_rbi_dir],
         dsl_rbi_dir: options[:dsl_rbi_dir],
         shim_rbi_dir: options[:shim_rbi_dir],
         annotations_rbi_dir: options[:annotations_rbi_dir],
+        todo_rbi_file: options[:todo_rbi_file],
         payload: options[:payload]
       )
       command.execute

lib/tapioca/commands/check_shims.rb

@@ -14,23 +14,25 @@ class CheckShims < Command
           dsl_rbi_dir: String,
           annotations_rbi_dir: String,
           shim_rbi_dir: String,
+          todo_rbi_file: String,
           payload: T::Boolean
         ).void
       end
-      def initialize(gem_rbi_dir:, dsl_rbi_dir:, annotations_rbi_dir:, shim_rbi_dir:, payload:)
+      def initialize(gem_rbi_dir:, dsl_rbi_dir:, annotations_rbi_dir:, shim_rbi_dir:, todo_rbi_file:, payload:)
         super()
         @gem_rbi_dir = gem_rbi_dir
         @dsl_rbi_dir = dsl_rbi_dir
         @annotations_rbi_dir = annotations_rbi_dir
         @shim_rbi_dir = shim_rbi_dir
+        @todo_rbi_file = todo_rbi_file
         @payload = payload
       end
 
       sig { override.void }
       def execute
         index = RBI::Index.new
 
-        if !Dir.exist?(@shim_rbi_dir) || Dir.empty?(@shim_rbi_dir)
+        if (!Dir.exist?(@shim_rbi_dir) || Dir.empty?(@shim_rbi_dir)) && !File.exist?(@todo_rbi_file)
           say("No shim RBIs to check", :green)
           exit(0)
         end
@@ -59,12 +61,13 @@ def execute
           end
         end
 
+        index_rbi(index, "todo", @todo_rbi_file)
         index_rbis(index, "shim", @shim_rbi_dir)
         index_rbis(index, "gem", @gem_rbi_dir)
         index_rbis(index, "dsl", @dsl_rbi_dir)
         index_rbis(index, "annotation", @annotations_rbi_dir)
 
-        duplicates = duplicated_nodes_from_index(index, @shim_rbi_dir)
+        duplicates = duplicated_nodes_from_index(index, shim_rbi_dir: @shim_rbi_dir, todo_rbi_file: @todo_rbi_file)
         unless duplicates.empty?
           duplicates.each do |key, nodes|
             say_error("\nDuplicated RBI for #{key}:", :red)
@@ -76,7 +79,7 @@ def execute
               say_error(" * #{loc_string}", :red)
             end
           end
-          say_error("\nPlease remove the duplicated definitions from the #{@shim_rbi_dir} directory.", :red)
+          say_error("\nPlease remove the duplicated definitions from #{@shim_rbi_dir} and #{@todo_rbi_file}", :red)
           exit(1)
         end
 

lib/tapioca/helpers/rbi_files_helper.rb

@@ -19,6 +19,15 @@ def index_payload(index, dir)
       say(" Done", :green)
     end
 
+    sig { params(index: RBI::Index, kind: String, file: String).void }
+    def index_rbi(index, kind, file)
+      return unless File.exist?(file)
+
+      say("Loading #{kind} RBIs from #{file}... ")
+      parse_and_index_file(index, file)
+      say(" Done", :green)
+    end
+
     sig { params(index: RBI::Index, kind: String, dir: String).void }
     def index_rbis(index, kind, dir)
       return unless Dir.exist?(dir) && !Dir.empty?(dir)
@@ -29,13 +38,19 @@ def index_rbis(index, kind, dir)
       say(" Done", :green)
     end
 
-    sig { params(index: RBI::Index, shim_rbi_dir: String).returns(T::Hash[String, T::Array[RBI::Node]]) }
-    def duplicated_nodes_from_index(index, shim_rbi_dir)
+    sig do
+      params(
+        index: RBI::Index,
+        shim_rbi_dir: String,
+        todo_rbi_file: String
+      ).returns(T::Hash[String, T::Array[RBI::Node]])
+    end
+    def duplicated_nodes_from_index(index, shim_rbi_dir:, todo_rbi_file:)
       duplicates = {}
       say("Looking for duplicates... ")
       index.keys.each do |key|
         nodes = index[key]
-        next unless shims_have_duplicates?(nodes, shim_rbi_dir)
+        next unless shims_or_todos_have_duplicates?(nodes, shim_rbi_dir: shim_rbi_dir, todo_rbi_file: todo_rbi_file)
 
         duplicates[key] = nodes
       end
@@ -137,31 +152,39 @@ def validate_rbi_files(command:, gem_dir:, dsl_dir:, auto_strictness:, gems: [],
 
     sig { params(index: RBI::Index, files: T::Array[String]).void }
     def parse_and_index_files(index, files)
-      trees = files.map do |file|
-        RBI::Parser.parse_file(file)
-      rescue RBI::ParseError => e
-        say_error("\nWarning: #{e} (#{e.location})", :yellow)
-      end.compact
-      index.visit_all(trees)
+      files.each do |file|
+        parse_and_index_file(index, file)
+      end
+    end
+
+    sig { params(index: RBI::Index, file: String).void }
+    def parse_and_index_file(index, file)
+      tree = RBI::Parser.parse_file(file)
+      index.visit(tree)
+    rescue RBI::ParseError => e
+      say_error("\nWarning: #{e} (#{e.location})", :yellow)
     end
 
-    sig { params(nodes: T::Array[RBI::Node], shim_rbi_dir: String).returns(T::Boolean) }
-    def shims_have_duplicates?(nodes, shim_rbi_dir)
+    sig { params(nodes: T::Array[RBI::Node], shim_rbi_dir: String, todo_rbi_file: String).returns(T::Boolean) }
+    def shims_or_todos_have_duplicates?(nodes, shim_rbi_dir:, todo_rbi_file:)
       return false if nodes.size == 1
 
-      shims = extract_shims(nodes, shim_rbi_dir)
-      return false if shims.empty?
+      shims_or_todos = extract_shims_and_todos(nodes, shim_rbi_dir: shim_rbi_dir, todo_rbi_file: todo_rbi_file)
+      return false if shims_or_todos.empty?
 
-      props = extract_methods_and_attrs(shims)
+      shims_or_todos_empty_scopes = extract_empty_scopes(shims_or_todos)
+      return true unless shims_or_todos_empty_scopes.empty?
+
+      props = extract_methods_and_attrs(shims_or_todos)
       return false if props.empty?
 
-      shims_with_sigs = extract_nodes_with_sigs(props)
-      shims_with_sigs.each do |shim|
-        shim_sigs = shim.sigs
+      shims_or_todos_with_sigs = extract_nodes_with_sigs(props)
+      shims_or_todos_with_sigs.each do |shim_or_todo|
+        shims_or_todos_sigs = shim_or_todo.sigs
 
         extract_methods_and_attrs(nodes).each do |node|
-          next if node == shim
-          return true if shim_sigs.all? { |sig| node.sigs.include?(sig) }
+          next if node == shim_or_todo
+          return true if shims_or_todos_sigs.all? { |sig| node.sigs.include?(sig) }
         end
 
         return false
@@ -170,13 +193,18 @@ def shims_have_duplicates?(nodes, shim_rbi_dir)
       true
     end
 
-    sig { params(nodes: T::Array[RBI::Node], shim_rbi_dir: String).returns(T::Array[RBI::Node]) }
-    def extract_shims(nodes, shim_rbi_dir)
+    sig { params(nodes: T::Array[RBI::Node], shim_rbi_dir: String, todo_rbi_file: String).returns(T::Array[RBI::Node]) }
+    def extract_shims_and_todos(nodes, shim_rbi_dir:, todo_rbi_file:)
       nodes.select do |node|
-        node.loc&.file&.start_with?(shim_rbi_dir)
+        node.loc&.file&.start_with?(shim_rbi_dir) || node.loc&.file == todo_rbi_file
       end
     end
 
+    sig { params(nodes: T::Array[RBI::Node]).returns(T::Array[RBI::Scope]) }
+    def extract_empty_scopes(nodes)
+      T.cast(nodes.select { |node| node.is_a?(RBI::Scope) && node.empty? }, T::Array[RBI::Scope])
+    end
+
     sig { params(nodes: T::Array[RBI::Node]).returns(T::Array[T.any(RBI::Method, RBI::Attr)]) }
     def extract_methods_and_attrs(nodes)
       T.cast(nodes.select do |node|

sorbet/rbi/shims/rdoc.rbi

@@ -1,4 +1,3 @@
 # typed: true
 
-module RDoc; end
 class RDoc::Task; end