self review

Author: chris-olszewski

.github/workflows/ci.yml

@@ -108,7 +108,6 @@ jobs:
         run: bundle exec rake TESTOPTS="--verbose"
 
       - name: Check RBI types with Sorbet
-        if: ${{ matrix.checkTarget }}
         working-directory: ./temporalio
         run: |
           cd extra/sorbet_check

README.md

@@ -1419,28 +1419,21 @@ The SDK ships two sets of type signatures:
 * **RBI** (`rbi/temporalio.rbi`) -- Enriched Sorbet types derived from the RBS signatures. Must be updated manually
   when the RBS changes (see below).
 
-The RBI file is validated in CI by running `srb tc` against `extra/sorbet_check/check_types.rb`. If a public API
-signature changes and the RBI is not updated, this check will fail.
+The RBI is validated in CI two ways:
+
+* **Static** -- `srb tc` checks that `extra/sorbet_check/check_types.rb` typechecks against the RBI. This catches
+  inconsistencies within the RBI itself (missing classes, wrong param types, etc.).
+* **Runtime** -- The test suite runs with `TEMPORAL_SORBET_RUNTIME_CHECK=1`, which applies every RBI signature to the
+  real implementation at runtime via `SigApplicator`. This catches drift between the RBI and actual code (e.g. a
+  renamed parameter, a changed return type, or a missing method).
 
 **When adding or changing a public method:**
 
 1. Update the RBS file in `sig/` as usual. Verify with `bundle exec rake steep`.
-2. Update `rbi/temporalio.rbi` to match. Apply the following type mappings:
-
-   | RBS | RBI (Sorbet) |
-   |-----|-------------|
-   | `String?` | `T.nilable(String)` |
-   | `Integer \| Float` / `duration` | `T.any(Integer, Float)` |
-   | `singleton(Foo)` | `T.class_of(Foo)` |
-   | `bool` | `T::Boolean` |
-   | `untyped` | `T.untyped` |
-   | `Array[X]` | `T::Array[X]` |
-   | `Hash[K, V]` | `T::Hash[K, V]` |
-   | `X \| Y` | `T.any(X, Y)` |
-   | Enum `::enum` | `Integer` |
-
+2. Update `rbi/temporalio.rbi` to match.
 3. If the new method is important for users, add a usage example to `extra/sorbet_check/check_types.rb`.
 4. Run the Sorbet check: `cd extra/sorbet_check && bundle exec srb tc`.
+5. Run tests with runtime type checking enabled `TEMPORAL_SORBET_RUNTIME_CHECK=1 bundle exec rake test`
 
 **What NOT to include in the RBI:**
 * `Temporalio::Internal::*` types (excluded entirely)

temporalio/test/support/sig_applicator.rb

@@ -44,6 +44,7 @@ module SigApplicator
 
   class << self
     def apply_all!
+      @mutex.synchronize { @type_errors.clear }
       configure_error_handler!
       register_summary_hook!
 
@@ -145,14 +146,12 @@ def apply_scope(node, errors)
       return [0, 0] if SKIP_PREFIXES.any? { |prefix| class_name.start_with?(prefix) }
       return [0, 0] if SKIP_CLASSES.include?(class_name)
 
-      missing_name = false
-      klass = begin
-        Object.const_get(class_name)
+      begin
+        klass = Object.const_get(class_name)
       rescue NameError
         errors << "#{class_name}: class not found"
-        missing_name = true
+        return [0, 0]
       end
-      return [0, 0] if missing_name
 
       applied = 0
       skipped = 0
@@ -192,47 +191,19 @@ def apply_method_sig(target, class_name, method_node, errors, class_method: fals
 
       return :skipped if SKIP_METHODS.include?(full_name)
 
-      missing_method = false
-      original = begin
-        target.instance_method(method_name)
+      begin
+        original = target.instance_method(method_name)
       rescue NameError
         errors << "#{full_name}: method not found"
-        missing_method = true
+        return false
       end
-      return false if missing_method
 
-      # Skip when there's a block param mismatch between the sig and actual
-      # method. Common causes:
-      # - Anonymous block forwarding (Ruby 3.1+ `def foo(&)`)
-      # - Methods using yield with no block param
-      # - Sigs that omit block params the method declares
-      actual_params = original.parameters
-      actual_block = actual_params.find { |kind, _| kind == :block }
-      sig_block_params = method_node.sigs.flat_map { |sig| sig.params.select { |p| p.type&.include?('T.proc') } }
-      actual_has_block = !actual_block.nil?
-      actual_block_anonymous = actual_block && (actual_block[1].nil? || actual_block[1] == :&)
-      sig_has_block = sig_block_params.any?
-      return :skipped if actual_block_anonymous && sig_has_block
-      return :skipped if actual_has_block && !sig_has_block
-      return :skipped if !actual_has_block && sig_has_block
-
-      # Skip setter methods where Ruby creates unnamed params (attr_writer)
-      has_unnamed_params = actual_params.any? { |_kind, name| name.nil? }
-      return :skipped if has_unnamed_params && method_name.end_with?('=')
-
-      # Skip when the actual method only has rest/unnamed params but the sig
-      # declares specific named params. This happens with synthetic methods
-      # (e.g., Data.define generates .new, .[], #initialize, #with with a
-      # single splat) where the RBI provides typed keyword params for better
-      # static checking but the runtime signature is incompatible.
-      non_block_params = actual_params.reject { |kind, _| kind == :block } # rubocop:disable Style/HashExcept
-      all_rest_or_unnamed = non_block_params.all? { |kind, _| kind == :rest || kind == :keyrest }
-      sig_named_params = method_node.sigs.flat_map { |s| s.params.reject { |p| p.type&.include?('T.proc') } }
-      return :skipped if all_rest_or_unnamed && non_block_params.any? && sig_named_params.any?
+      return :skipped if skip_method?(original, method_node, method_name)
 
       target.extend(T::Sig)
 
       method_node.sigs.each do |sig|
+        # RBI::Sig#string serializes back to valid T::Sig DSL source
         sig_source = sig.string
         begin
           target.class_eval(sig_source)
@@ -252,5 +223,35 @@ def apply_method_sig(target, class_name, method_node, errors, class_method: fals
 
       true
     end
+
+    # Determines whether a method should be skipped for sig application based
+    # on parameter shape mismatches between the RBI sig and the actual method.
+    def skip_method?(original, method_node, method_name)
+      actual_params = original.parameters
+
+      # Block param mismatch: anonymous block forwarding (Ruby 3.1+ `def foo(&)`),
+      # methods using yield with no block param, or sigs that omit declared blocks.
+      actual_block = actual_params.find { |kind, _| kind == :block }
+      sig_block_params = method_node.sigs.flat_map { |sig| sig.params.select { |p| p.type&.include?('T.proc') } }
+      actual_has_block = !actual_block.nil?
+      actual_block_anonymous = actual_block && (actual_block[1].nil? || actual_block[1] == :&)
+      sig_has_block = sig_block_params.any?
+      return true if actual_block_anonymous && sig_has_block
+      return true if actual_has_block != sig_has_block
+
+      # Setter methods where Ruby creates unnamed params (attr_writer)
+      has_unnamed_params = actual_params.any? { |_kind, name| name.nil? }
+      return true if has_unnamed_params && method_name.end_with?('=')
+
+      # Synthetic methods (e.g., Data.define generates .new, .[], #initialize,
+      # #with with a single splat) where the RBI provides typed keyword params
+      # for better static checking but the runtime signature is incompatible.
+      non_block_params = actual_params.reject { |kind, _| kind == :block } # rubocop:disable Style/HashExcept
+      all_rest_or_unnamed = non_block_params.all? { |kind, _| kind == :rest || kind == :keyrest }
+      sig_named_params = method_node.sigs.flat_map { |s| s.params.reject { |p| p.type&.include?('T.proc') } }
+      return true if all_rest_or_unnamed && non_block_params.any? && sig_named_params.any?
+
+      false
+    end
   end
 end

temporalio/test/support/sig_applicator_test.rb

@@ -0,0 +1,126 @@
+# frozen_string_literal: true
+
+require 'minitest/autorun'
+require 'rbi'
+require 'support/sig_applicator'
+
+module Support
+  class SigApplicatorTest < Minitest::Test
+    # --- Block param mismatch skips ---
+
+    def test_skips_anonymous_block_with_sig_block
+      klass = Class.new do
+        def foo(&); end
+      end
+      method_node = parse_method(
+        'class X; sig { params(blk: T.proc.void).void }; def foo(&blk); end; end'
+      )
+      original = klass.instance_method(:foo)
+      assert skip_method?(original, method_node, :foo)
+    end
+
+    def test_skips_method_with_block_but_sig_without
+      klass = Class.new do
+        def foo(&); end
+      end
+      method_node = parse_method('class X; sig { void }; def foo; end; end')
+      original = klass.instance_method(:foo)
+      assert skip_method?(original, method_node, :foo)
+    end
+
+    def test_skips_sig_with_block_but_method_without
+      klass = Class.new do
+        def foo; end
+      end
+      method_node = parse_method(
+        'class X; sig { params(blk: T.proc.void).void }; def foo(&blk); end; end'
+      )
+      original = klass.instance_method(:foo)
+      assert skip_method?(original, method_node, :foo)
+    end
+
+    def test_does_not_skip_matching_named_block
+      klass = Class.new do
+        def foo(&blk); end # rubocop:disable Naming/BlockForwarding
+      end
+      method_node = parse_method(
+        'class X; sig { params(blk: T.proc.void).void }; def foo(&blk); end; end'
+      )
+      original = klass.instance_method(:foo)
+      refute skip_method?(original, method_node, :foo)
+    end
+
+    # --- Setter / unnamed param skips ---
+
+    def test_skips_attr_writer_with_unnamed_params
+      klass = Class.new { attr_writer :bar }
+      method_node = parse_method(
+        'class X; sig { params(value: Integer).void }; def bar=(value); end; end'
+      )
+      original = klass.instance_method(:bar=)
+      assert skip_method?(original, method_node, :bar=)
+    end
+
+    def test_does_not_skip_regular_setter
+      klass = Class.new do
+        def bar=(value); end
+      end
+      method_node = parse_method(
+        'class X; sig { params(value: Integer).void }; def bar=(value); end; end'
+      )
+      original = klass.instance_method(:bar=)
+      refute skip_method?(original, method_node, :bar=)
+    end
+
+    # --- Synthetic rest-param skips ---
+
+    def test_skips_rest_only_method_with_named_sig_params
+      klass = Class.new do
+        def initialize(*args); end
+      end
+      method_node = parse_method(<<~RBI)
+        class X
+          sig { params(name: String, age: Integer).void }
+          def initialize(name:, age:); end
+        end
+      RBI
+      original = klass.instance_method(:initialize)
+      assert skip_method?(original, method_node, :initialize)
+    end
+
+    def test_does_not_skip_when_params_match
+      klass = Class.new do
+        def foo(val1, val2); end
+      end
+      method_node = parse_method(<<~RBI)
+        class X
+          sig { params(val1: Integer, val2: String).returns(String) }
+          def foo(val1, val2); end
+        end
+      RBI
+      original = klass.instance_method(:foo)
+      refute skip_method?(original, method_node, :foo)
+    end
+
+    def test_does_not_skip_no_param_method
+      klass = Class.new do
+        def foo; end
+      end
+      method_node = parse_method('class X; sig { returns(String) }; def foo; end; end')
+      original = klass.instance_method(:foo)
+      refute skip_method?(original, method_node, :foo)
+    end
+
+    private
+
+    def parse_method(source)
+      tree = RBI::Parser.parse_string(source)
+      klass = tree.nodes.first
+      klass.nodes.find { |n| n.is_a?(RBI::Method) }
+    end
+
+    def skip_method?(original, method_node, method_name)
+      SigApplicator.send(:skip_method?, original, method_node, method_name)
+    end
+  end
+end