Skip to content

RSpec makes define_method public? #873

Merged
merged 3 commits into from Apr 18, 2013

4 participants

@moll
moll commented Apr 15, 2013

Beats me, but I just stumbled upon a passing test that failed IRL because it used the private Module.define_method. Seems that after requiring RSpec, it's not private any more.

On Ruby v1.9.3 with RSpec v2.13:

>> Object.define_method
NoMethodError: private method `define_method' called for Object:Class
    from (irb):1
    from /usr/local/bin/irb:12:in `<main>'
>> require "rspec"
=> true
>> Object.define_method
ArgumentError: wrong number of arguments (0 for 1)
    from (irb):3:in `define_method'
    from (irb):3
    from /usr/local/bin/irb:12:in `<main>'
@thomas-holmes
RSpec member

Looks like it is intentionally being made to be this way in memoization_helpers.rb:475

@myronmarston
RSpec member

Looks like it is intentionally being made to be this way in memoization_helpers.rb:475

That's only making it public in an anonymous module, though. I don't think that affect Object, although I could be wrong...

Regardless, we should fix this. Thanks for reporting it!

@thomas-holmes
RSpec member

I will try to look into it further, I'd like to help 👍

@thomas-holmes
RSpec member

I tried defining another method in the same place that was private (method_undefined) and it became public on Object. I think that definitively makes this the culprit. Any suggestions on how to proceed containing this?

@moll
moll commented Apr 17, 2013

Other than ditching Ruby? :) I'd say removing line 475 as the following holds true:

>> Object.define_method
NoMethodError: private method `define_method' called for Object:Class
        from (irb):1
        from /usr/local/bin/irb:12:in `<main>'
>> module Foo
>>   public_class_method :define_method
>> end
=> Foo
>> Object.define_method
ArgumentError: wrong number of arguments (0 for 1)
        from (irb):3:in `define_method'
        from (irb):3
        from /usr/local/bin/irb:12:in `<main>'
@myronmarston
RSpec member

To me, it feels like a bug in ruby. Regardless, we should fix the code so it doesn't do this. The reason I had put public_class_method :define_method was to make it easy to define new methods on those modules, as that's basically the role they serve. We can remove that line and use send instead, though. Alternately, if you can find another way to make define_method public w/o it leaking....then have at it :).

@thomas-holmes thomas-holmes added a commit to thomas-holmes/rspec-core that referenced this pull request Apr 18, 2013
@thomas-holmes thomas-holmes No longer make define_method public, use send instead.
MemoizedHelpers::ClassMethods module was making define_method
public for ease of use within the module. No longer do this and
use send instead. This fixes rspec/rspec-core#873.
5280e95
@JonRowe JonRowe was assigned Apr 18, 2013
@JonRowe
RSpec member
JonRowe commented Apr 18, 2013

I'm guessing public_class_method works on the original class it's defined on rather than the one we're creating, changing how we define the method as public (by redefining the method) works on the current class.

I've also added a test to ensure we don't leak it.

Review? /cc @myronmarston @samphippen @soulcutter

@moll
moll commented Apr 18, 2013

Hehe, after reporting this I investigated how visibility works in Ruby and found out the public method applies only to instance methods of the current class and thereby does not apply to methods set on the singleton class. It's a nuts system because the public you, @JonRowe, added on line 475 is useless.

Which, on a side note, is probably a SNAFU everywhere where people expect methods defined via def self.foo following private to be private.

@myronmarston
RSpec member

Thanks to both of you for working on this!

The spec added by @JonRowe is nice (we want specs for this kind of stuff to prevent regressions), but I think I prefer the simplicity of the @thomas-holmes's fix. I had originally made the method public as a convenience...but the solution of redefining define_method to make it public seems more complicated then just using send.

JonRowe and others added some commits Apr 18, 2013
@JonRowe JonRowe test that we dont leak define_method into public methods fbc33d6
@thomas-holmes thomas-holmes No longer make define_method public, use send instead.
MemoizedHelpers::ClassMethods module was making define_method
public for ease of use within the module. No longer do this and
use send instead. This fixes rspec/rspec-core#873.
4d3a64d
@JonRowe
RSpec member
JonRowe commented Apr 18, 2013

Ok I've picked across @thomas-holmes' fix, just rerunning the specs then I'll merge it.

@myronmarston
RSpec member

@JonRowe -- thanks! Please add a changelog entry, too.

@thomas-holmes
RSpec member

Thanks guys!

@JonRowe JonRowe merged commit 5fd60b5 into master Apr 18, 2013
@myronmarston
RSpec member

So this is interesting:

https://bugs.ruby-lang.org/issues/8284

I started a discussion on the ruby rogues mailing list about this, someone reporte the bug to the ruby issue tracker and it got fixed. Turns out it's been a bug in MRI since ruby 1.0.

@JonRowe
RSpec member
JonRowe commented Apr 18, 2013

I liked this bit

I don't know if it will be, but in any case there is always
`singleton_class.send :public, :define_method`
instead or equivalent.

It's a bit more clear, IMO, as using "public_class_method" on a Module is a bit strange...
Modules don't really have "class methods".

FWIW, there are no specs in RubySpec about "public_class_method" on a Module.
@kalabiyau kalabiyau referenced this pull request in b4mboo/git-review Jul 17, 2013
Closed

Add ruby 2.0 to travis test list #53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.