Implement MatcherDelegator using the BlankSlate pattern

[casperisfine]

This allows to minimize the amount of method inherited by the delegator object, and protects RSpec from new methods being introduced by future Ruby versions or monkey patched in by user code or libraries.

On modern Rubies there is BasicObject for this purpose, that's what delegate.rb uses for instance, and is considered best practice for delegators and other proxy objects. However since RSpec still support Ruby 1.8 so it's not an option.

Additionally BasicObject doesn't inherit from Object so all constant resolutions in a BasicObject descendant must be fully qualified which is tedious, but more importantly changing this now would probably break third party code that inherit from MatcherDelegator.

[pirj]

Fails even on 2.2, but we don’t care much, since Rails 7.1 is Ruby 2.7+, right?

Please don’t hesitate to ping if you need me to approve CI.

[casperisfine]

Thanks @pirj ! (Actually I'm now thinking I may be able to open a PR on my fork to get CI running without approval 🤦 ).

we don’t care much, since Rails 7.1 is Ruby 2.7+, right?

Well, Rails 7.2 is what trigger the issue at hand, but the same kind of issue can be caused by applications monkey patches, etc. And ideally I'd avoid too big of a if RUBY_VERSION >=.

[casperisfine]

Oh, interesting, you couldn't copy methods in modules pre 2.2:

Failure/Error: define_method(method, ::Object.instance_method(method))

TypeError:
  bind argument must be a subclass of Object

I suppose the solution is to go with the "BlankSlate" pattern for all versions, that also avoid running into constant resolution issues.

[casperisfine]

Ok, I pushed a new version that passes CI on my fork except for the with diff-lcs job, but I suspect it's a red herring. I think it may pass here.

[casperisfine]

So I had to keep these two only to not break:

    # Decorator used for matchers that have special implementations of
    # operators like `==` and `===`.
    # @private
    class AliasedMatcherWithOperatorSupport < AliasedMatcher
      # We undef these so that they get delegated via `method_missing`.
      undef ==
      undef ===
    end

I think we could remove them from BaseDelegator and just make AliasedMatcherWithOperatorSupport and empty class.

[JonRowe]

=== is the base method matchers use to match on, so if they are explicitly undefined its likely to fall back to delegated ones

[casperisfine]

What I meant by this comment, is that I left them in the BlankSlate to minimize the diff.

But IMO they should be removed from the BlankSlate and AliasedMatcherWithOperatorSupport should stop undefining them.

[casperisfine]

0b13e89

[casperisfine]

Note that these are the methods required to pass the test suite, they don't all necessarily make sense to keep. e.g. method for instance is redundant with __method__ and a cleanup of the test suite would allow to get rid of it.

[casperisfine]

Also, I'm unfamiliar with RSpec, but if there are other delegators of the same type, it may make sense to proactively convert them to a similar pattern.

[byroot]

the with diff-lcs job, but I suspect it's a red herring.

So it fails on that branch too, but I still think it's unrelated.

/active_support/core_ext/time/conversions.rb:62:in `<class:Time>': undefined method `deprecator' for ActiveSupport:Module (NoMethodError)
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/core_ext/time/conversions.rb:7:in `<top (required)>'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/core_ext/object/json.rb:14:in `require'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/core_ext/object/json.rb:14:in `<top (required)>'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/json/encoding.rb:3:in `require'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/json/encoding.rb:3:in `<top (required)>'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/json.rb:4:in `require'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/activesupport-7.1.1/lib/active_support/json.rb:4:in `<top (required)>'
	from /home/runner/work/rspec-expectations/rspec-expectations/bundle/ruby/2.7.0/gems/protobuf-cucumber-3.10.8/lib/protobuf.rb:10:in `require'

This happens because before cherry-picking things inside Active Support you must first require "active_support", and the protobuf gem doesn't respect that: https://github.com/ruby-protobuf/protobuf/blob/b866fc5667226d0582f328ab24c648e578c5a380/lib/protobuf.rb#L7-L11

I'll open another PR there, but In the meantime I think this one is ready.

[byroot]

Upstream PR: ruby-protobuf/protobuf#431

[casperisfine]

@pirj anything else I can do?

[casperisfine]

👋 any news here?

[JonRowe]

Thanks for this, I think this is a good improvement for us as a protection against monkey patching Object, I'm not quite sure when I'll release this yet as I'm debating the level of change this justifies and reflecting on where we could extend this logic too.

(Apologies for the delayed merge then delayed comment, I was originally letting Phil handle it whilst being busy with rspec-rails [I don't use Ruby or Rails in my day-job currently])