Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

restore_original_visibility crashing when prepended modules and refinements both exist #1395

Closed
fledman opened this issue Jan 7, 2021 · 14 comments
Closed

restore_original_visibility crashing when prepended modules and refinements both exist #1395

fledman opened this issue Jan 7, 2021 · 14 comments

Comments

@fledman
Copy link

fledman commented Jan 7, 2021 •
edited
Loading

Subject of the issue

the restore_original_visibility step of resetting a method double crashes when both of the following conditions are true:

  1. any module is prepended to the mocked object's class
  2. a refinement for the mocked method is defined; it does NOT need to be used

Your environment

  • Ruby version: 2.7.2
  • rspec-mocks version: 3.10.0

Steps to reproduce

# frozen_string_literal: true

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "rspec", "3.10.0"
end

puts "Ruby version is: #{RUBY_VERSION}"

require "rspec/autorun"

module Nothing
end

Hash.prepend(Nothing)

module TheAnswer
  refine Hash do
    def [](key)
      42
    end
  end
end

# note: we never use TheAnswer 

describe "rspec visibility problem" do
  let(:thing) { { flag: "YES" } }

  context "[]" do
    subject { thing[:flag] }

    it "allow works" do
      allow(thing).to receive(:[]).and_call_original
      subject
    end

    it "expect works" do
      expect(thing).to receive(:[]).and_call_original
      subject
    end
  end
end

Expected behavior

The tests pass without any errors

Actual behavior

Ruby version is: 2.7.2
FF

Failures:

  1) rspec visibility problem [] allow works
     Failure/Error: object_singleton_class.__send__(@original_visibility, method_name)
     
     NameError:
       undefined method `[]' for class `#<Class:#<Hash:0x00007fc7efce4758>>'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:108:in `public'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:108:in `restore_original_visibility'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:87:in `restore_original_method'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:118:in `reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `block in reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `each_value'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `block in reset_all'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `each_value'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `reset_all'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks.rb:52:in `teardown'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/mocking_adapters/rspec.rb:27:in `teardown_mocks_for_rspec'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:518:in `run_after_example'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:281:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `block in with_around_and_singleton_context_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `block in with_around_example_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `with_around_example_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `with_around_and_singleton_context_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:259:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:644:in `block in run_examples'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `run_examples'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:606:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/configuration.rb:2067:in `with_suite_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:116:in `block in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/reporter.rb:74:in `report'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:115:in `run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:89:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:71:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:45:in `invoke'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:38:in `perform_at_exit'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:24:in `block in autorun'
     # 
     #   Showing full backtrace because every line was filtered out.
     #   See docs for RSpec::Configuration#backtrace_exclusion_patterns and
     #   RSpec::Configuration#backtrace_inclusion_patterns for more information.

  2) rspec visibility problem [] expect works
     Failure/Error: object_singleton_class.__send__(@original_visibility, method_name)
     
     NameError:
       undefined method `[]' for class `#<Class:#<Hash:0x00007fc7efd10448>>'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:108:in `public'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:108:in `restore_original_visibility'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:87:in `restore_original_method'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/method_double.rb:118:in `reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `block in reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `each_value'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/proxy.rb:353:in `reset'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `block in reset_all'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `each_value'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks/space.rb:79:in `reset_all'
     # .gem/ruby/2.7.2/gems/rspec-mocks-3.10.1/lib/rspec/mocks.rb:52:in `teardown'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/mocking_adapters/rspec.rb:27:in `teardown_mocks_for_rspec'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:518:in `run_after_example'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:281:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `block in with_around_and_singleton_context_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `block in with_around_example_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:624:in `run_around_example_hooks_for'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/hooks.rb:486:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:465:in `with_around_example_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:508:in `with_around_and_singleton_context_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example.rb:259:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:644:in `block in run_examples'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:640:in `run_examples'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:606:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `block in run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/example_group.rb:607:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `map'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/configuration.rb:2067:in `with_suite_hooks'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:116:in `block in run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/reporter.rb:74:in `report'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:115:in `run_specs'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:89:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:71:in `run'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:45:in `invoke'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:38:in `perform_at_exit'
     # .gem/ruby/2.7.2/gems/rspec-core-3.10.1/lib/rspec/core/runner.rb:24:in `block in autorun'
     # 
     #   Showing full backtrace because every line was filtered out.
     #   See docs for RSpec::Configuration#backtrace_exclusion_patterns and
     #   RSpec::Configuration#backtrace_inclusion_patterns for more information.

Finished in 0.00627 seconds (files took 0.05754 seconds to load)
2 examples, 2 failures

Failed examples:

rspec visibility_problem_spec.rb:39 # rspec visibility problem [] allow works
rspec visibility_problem_spec.rb:44 # rspec visibility problem [] expect works

Related Issues

rspec/rspec-rails#2394 seems to just be this issue under the hood

#1218 is a broader change, but may actually resolve this?

Resolution Thoughts

The problem seems to be a mismatch between stashing and cleanup.

Before stashing, we check definition and ownership:

rspec-mocks/lib/rspec/mocks/instance_method_stasher.rb

Lines 100 to 110 in 7c492bd

def method_defined_directly_on_klass?
method_defined_on_klass? && method_owned_by_klass?
end
# @private
def method_defined_on_klass?(klass=@klass)
MethodReference.method_defined_at_any_visibility?(klass, @method)
end
def method_owned_by_klass?
owner = @klass.instance_method(@method).owner

While before restoring visibility, we just check definition:

rspec-mocks/lib/rspec/mocks/method_double.rb

Lines 104 to 109 in 7c492bd

def restore_original_visibility
return unless @original_visibility &&
MethodReference.method_defined_at_any_visibility?(object_singleton_class, @method_name)
object_singleton_class.__send__(@original_visibility, method_name)
end

That is why I expect this subset of the diff from #1218 to resolve things:

diff --git a/lib/rspec/mocks/method_double.rb b/lib/rspec/mocks/method_double.rb
index 4ffdcdf2..b720e14b 100644
--- a/lib/rspec/mocks/method_double.rb
+++ b/lib/rspec/mocks/method_double.rb
@@ -83,8 +83,11 @@ def restore_original_method
         return unless @method_is_proxied
 
         remove_method_from_definition_target
-        @method_stasher.restore if @method_stasher.method_is_stashed?
-        restore_original_visibility
+
+        if @method_stasher.method_is_stashed?
+          @method_stasher.restore
+          restore_original_visibility
+        end
 
         @method_is_proxied = false
       end

The reason that the current code mostly works is that the ruby set_visibility*** function is inconsistent. If you call it on a singleton class where the method in question is defined in an ancestor, it works most of the time. However, it fails when a refinement and a prepended module are both present. I am not sure which of the following is true:

  1. set_visibility should be called on the module where the method is defined; rspec-mocks should not expect Ruby to walk the ancestor tree
  2. set_visibility works as rspec-mocks currently uses it, and this is a bug in Ruby

if (1) is the case, then we could also fix things by passing false to the three _method_defined? calls, to prevent searching the ancestor tree in our method existence check:

rspec-mocks/lib/rspec/mocks/method_reference.rb

Lines 67 to 68 in 7c492bd

def self.instance_method_visibility_for(klass, method_name)
if klass.public_method_defined?(method_name)

EDIT: (1) is also a Ruby bug: if set_visibility has constraints, then they should be enforced consistently

*** set_visibility is the C function public/private/protected delegate to
@fledman fledman mentioned this issue Jan 7, 2021
Open
@fledman
Copy link
Author

fledman commented Jan 8, 2021

I submitted an issue to the Ruby tracker: https://bugs.ruby-lang.org/issues/17519

@pirj
Copy link
Member

pirj commented Jan 8, 2021

Thanks, @fledman !

@pirj
Copy link
Member

pirj commented Jan 9, 2021

Might be related #1101

@fledman
Copy link
Author

fledman commented Feb 2, 2021

unfortunately no one has responded to my Ruby bug report
hopefully we can merge #1218 in the near future

@pirj
Copy link
Member

pirj commented Feb 2, 2021

@JonRowe do you think it makes sense to summon someone from the Ruby core team to take a quick look?

@fledman
Copy link
Author

fledman commented Feb 4, 2021

if you can get someone to take a look, that would be helpful

@fledman
Copy link
Author

fledman commented Feb 17, 2021

@pirj @JonRowe any luck?

@pirj
Copy link
Member

pirj commented Feb 17, 2021

@jeremyevans @eregon may I kindly ask you to take a look, please?
We have a reproduction case and filed a ticket https://bugs.ruby-lang.org/issues/17519

@jeremyevans
Copy link

@pirj That bug is on my radar, I hope to have time to look into it before the end of the month.

@tennety
Copy link

tennety commented Apr 7, 2021

If it helps, the Ruby 2.7.3 release contains this fix, and the tests that were failing for me before with the error described in this issue are passing on 2.7.3!

@pirj
Copy link
Member

pirj commented Apr 7, 2021

@tennety Thanks for the heads-up.
@fledman Can you please confirm that it also fixes the issue for you?

@fledman
Copy link
Author

fledman commented Apr 21, 2021

the Ruby 2.7.3 release did fix this particular bug, so we can probably close the issue

FYI it introduced another similar bug, see ruby/ruby#4200 (comment)
the fix for that new bug still needs to be merged: ruby/ruby#4357

@pirj
Copy link
Member

pirj commented Apr 22, 2021

Awesome!
Since there's nothing to fix on our side, closing.
Thanks again for the in-depth research.

@pirj pirj closed this as completed Apr 22, 2021
@fledman
Copy link
Author

fledman commented Apr 22, 2021 •
edited
Loading

@pirj
as a reminder, for rubies without the bug fix, rspec 4 should also resolve the issue, assuming #1218 gets merged in

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jeremyevans @pirj @tennety @fledman