Compiled method bodies do not scope defn/defs correctly #1239

Closed
headius opened this Issue Nov 18, 2013 · 9 comments

Projects

None yet

2 participants

@headius
Member
headius commented Nov 18, 2013

Moved from http://jira.codehaus.org/browse/JRUBY-7128, reported by @nirvdrum.

This might be a bug in rspec-expectations, but I'm unable to force JIT my app due to a bad :remove_method call. Specifically, in my rspec (2.13.0) setup, I've tried to disable the old should syntax with something like the following:

RSpec.configure do |config|
  config.expect_with :rspec do |c|
    c.syntax = :expect
  end
end

This causes rspec to execute this method:

def self.disable_deprecated_should
  return unless deprecated_should_enabled?

  remove_method :should
  remove_method :should_not

  self.deprecated_should_enabled = false
end

And when that's executed things blow up like so:

 ~/d/w/mogotest /master> rspec spec/models/user_spec.rb 
NameError: method 'should' not defined in RSpec::Expectations::ExpectationTarget
              remove_method at org/jruby/RubyModule.java:2331
  disable_deprecated_should at /home/nirvdrum/.rbenv/versions/jruby-1.7.2/lib/ruby/gems/shared/gems/rspec-expectations-2.13.0/lib/rspec/expectations/expectation_target.rb:71
             disable_should at /home/nirvdrum/.rbenv/versions/jruby-1.7.2/lib/ruby/gems/shared/gems/rspec-expectations-2.13.0/lib/rspec/expectations/syntax.rb:74
                    syntax= at /home/nirvdrum/.rbenv/versions/jruby-1.7.2/lib/ruby/gems/shared/gems/rspec-expectations-2.13.0/lib/rspec/matchers/configuration.rb:29

I'm raising the issue because I don't see this behavior when I don't force JIT. I don't know if it has to do with loading semantics or if force JIT should be resilient to errors like this. It could also just be an rspec bug that I can take up with them.

@headius
Member
headius commented Nov 19, 2013

Reproduced with 1.7.8.

@headius
Member
headius commented Nov 19, 2013

Trivial reproduction:

$ ruby -e "class Foo; def self.define; def bar; end; end; def self.remove; remove_method :bar; end; end; Foo.define; Foo.bar; Foo.remove"
NameError: method 'bar' not defined in Foo
  remove_method at org/jruby/RubyModule.java:2340
         remove at -e:1
         (root) at -e:1

It appears remove_method is not working properly when called from a compiled body.

@headius
Member
headius commented Nov 19, 2013

Actually, that doesn't work interpreted either...but it works in MRI.

@nirvdrum
Contributor

The sample project there should work in interpreted mode. I assume you mean your trivial reproduction doesn't work in interpreted mode. But if you're seeing othewise with the project, let me know.

@headius
Member
headius commented Nov 19, 2013

Ok, so the issue here appears to be various problems with how we determine the class into which "def" methods will go.

The interpreter does appear to be doing the right thing, and behavior matches MRI for the following script:

lass Foo
  def self.define
    def bar; end
  end

  def self.remove
    remove_method :bar
  end
end

Foo.define
Foo.remove

Interestingly, the method does not appear to go into Foo's metaclass as you'd expect, and calling Foo.bar fails to work.

However, in the compiler, the method does go into Foo's metaclass, causing the subsequent call to remove_method to not find it. This is likely just a problem in our pre/post-method wrapper logic for compiled methods, not pushing the appropriate thread-local context for method definition. Nearly there.

@headius headius closed this in 4c9c7e4 Nov 19, 2013
@headius
Member
headius commented Nov 19, 2013

Fixed; test coming. The problem was that defn/defs bodies (normal and singleton method definitions) need the current class ("rubyClass" in ThreadContext) to come from the scope rather than from the class in which the surrounding method is defined. Within a class body, this is no problem since all class bodies get a scope. In a method body, however, we try to optimize the scope away, so it was not used for rubyClass.

This fix will slightly impact performance of methods that have def in them, but those cases are pretty rare and generally limited to boot-time metaprogramming.

@headius headius added a commit that referenced this issue Nov 19, 2013
@headius headius Test for #1239. c0f54a7
@headius
Member
headius commented Nov 19, 2013

Sorry this one sat on the shelf so long.

@nirvdrum
Contributor

No problem. Thanks for taking care of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment