Support class redefinitions in runtime for rbs test mode

[HoneyryderChuck]

When used in "test mode" (via rbs/setup) for runtime type checking, rbs is not able to verify when a class is redefined, such as when remove_const and const_set are used. These are used in certain monkey-patching scenarios, and are common enough to warrant specialized support.

As an example, webmock uses it to apply monkey-patching to practically all supported http clients (this is for net-http, but everything else works the same way).

When used in such cases, runtime type checking will fail if webmock is disabled-and-enabled-again across tests, where the Net::HTTP class stored in the definition builder will fail to match against a just-redefined instance of Net::HTTP. Given that rbs/setup was designed to be used in tests, and webmock is the most widely used http client mock library, there should be some level of support for this.

I've done my research, and I see that rbs/setup uses the Tracepoint API to track class definitions and hooking them up with the type definitions. Sadly, there seems not to have a tracepoint hook for remove_const and const_set. Is this something that needs to be done at the tracepoint level first?

[soutaro]

@HoneyryderChuck Thank you for reporting the problem.

Hmm, rbs/test/setup is loaded before other libraries, net/http is defined, and webmock assigns new class to Net::HTTP...
It seems like we need to add a hook to setup runtime type checkers after loading webmock.

I also wonder if @ksss has any idea about this?

[ksss]

It is possible to hook remove_const and const_set with TracePoint, but I'm not yet clear on the potential issues.
Do you have a way to reproduce the problem?

[HoneyryderChuck]

I couldn't find narrow down the smallest possible reproduction, unfortunately. The error can be reproduced with httpx integration, test suite, as httpx performs the same "remove_const/const_set" trick in its webmock integrations. Just: clone the repo, bundle install, set up the rbs env vars and run bundle exec rake integration_tests.

[ksss]

@HoneyryderChuck
Thank you for the explanation. I understand the issue now.

When remove_const/const_set is executed and the class object is swapped, the old class remains in RBS::Test::TypeCheck#const_cache. Consequently, when calling #is_a? for the Types::ClassInstance check, the test fails because the object_id differs.

If we can hook into remove_const to clear RBS::Test::TypeCheck#const_cache, it seems we can solve the problem. This RBS::Test::TypeCheck#const_cache exists as a memoized cache within the RBS::Test::Tester::MethodCallTester instance, so we need to find this instance. However, currently, there doesn't seem to be a way to retrieve it.

For a makeshift solution, you could do something like the following, which should work for now:

diff --git a/lib/rbs/test/observer.rb b/lib/rbs/test/observer.rb
index ff023871..05cc0394 100644
--- a/lib/rbs/test/observer.rb
+++ b/lib/rbs/test/observer.rb
@@ -13,6 +13,10 @@ module RBS
         def register(key, object = nil, &block)
           @@observers[key] = object || block
         end
+
+        def observers
+          @@observers
+        end
       end
     end
   end
diff --git a/lib/rbs/test/setup.rb b/lib/rbs/test/setup.rb
index 1a8041c9..16c30e47 100644
--- a/lib/rbs/test/setup.rb
+++ b/lib/rbs/test/setup.rb
@@ -75,6 +75,21 @@ TracePoint.trace :end do |tp|
   end
 end

+hook = Module.new{
+  def remove_const(name)
+    super
+  end
+}
+Module.prepend(hook)
+
+TracePoint.new(:call) do |tp|
+  name = tp.binding.local_variable_get(:name)
+  type_name = TypeName("::#{tp.self}::#{name}")
+  RBS::Test::Observer.observers.values.each do |method_call_tester|
+    method_call_tester.check.const_cache.delete(type_name)
+  end
+end.enable(target: hook.instance_method(:remove_const))
+
 at_exit do
   if $!.nil? || $!.is_a?(SystemExit) && $!.success?
     if tester.targets.empty?

Alternatively, a very simple fix would be to just stop caching.

diff --git a/lib/rbs/test/type_check.rb b/lib/rbs/test/type_check.rb
index ac0edbc3..6ed8d134 100644
--- a/lib/rbs/test/type_check.rb
+++ b/lib/rbs/test/type_check.rb
@@ -208,11 +208,9 @@ module RBS
       end

       def get_class(type_name)
-        const_cache[type_name] ||= begin
-                                     Object.const_get(type_name.to_s)
-                                   rescue NameError
-                                     nil
-                                   end
+        Object.const_get(type_name.to_s)
+      rescue NameError
+        nil
       end

       def is_double?(value)

@soutaro How about this?

[soutaro]

Disable the caching! Seems like there were no strong reason to introduce it... #356

[ksss]

I'll send PR soon.

[HoneyryderChuck]

Thx everyone