Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Module.const_get for strings beginning with "::" #2339

Closed
wants to merge 1 commit into from

Conversation

lumeet
Copy link
Contributor

@lumeet lumeet commented Dec 21, 2014

Yes, #1487 seems super easy.

@headius
Copy link
Member

headius commented Jan 12, 2015

I'm not sure this is the right fix. For example, the following case should get the top-level Foo, and I don't think it will in your patch:

Foo = 1
module Bar
end
Bar.const_get('::Foo')

@lumeet
Copy link
Contributor Author

lumeet commented Jan 12, 2015

Hmm... am I missing something here?

test.rb:

Foo = 1
module Bar
end
puts Bar.const_get('::Foo')

module A 
  class B 
  end 
end

put Object.const_get('::A::B')

Using MRI 2.2 and patched JRuby:

$ rbenv shell 2.2.0
$ ruby test.rb
1
A::B

$ bin/jruby test.rb
1
A::B

@headius
Copy link
Member

headius commented Jan 12, 2015

Object.const_get('::A::B') is equivalent to Object.const_get('A::B'). Your other case does appear to show it working right, so perhaps I reviewed it poorly. Will try it locally.

@headius
Copy link
Member

headius commented Jan 12, 2015

Ok, I have a case that better illustrates the problem:

Foo = 1
module Bar; Foo = 2; end
Bar.const_get('::Foo')

Basically a leading :: should cause it to start searching at Object, rather than from the target class.

@lumeet
Copy link
Contributor Author

lumeet commented Jan 12, 2015

Now this is a perfect example of sticking to test cases instead of thinking. I'll take a look at this again. :)

@headius
Copy link
Member

headius commented Jan 12, 2015

@lumeet There are very likely test cases in MRI's suite (rake test:mri, test/mri/socket/) or in RubySpec (rake test:ruby:fast, spec/ruby/core/socket/) if you want to try getting them to run. If so, you can also remove tags/excludes from spec/tags or test/mri/excludes.

@headius
Copy link
Member

headius commented Jan 12, 2015

BTW, you can get access to Object with runtime.getObject.

@headius
Copy link
Member

headius commented Jan 14, 2015

Fixed via 5868779.

@headius headius closed this Jan 14, 2015
@headius headius added this to the Invalid or Duplicate milestone Jan 14, 2015
@headius headius self-assigned this Jan 14, 2015
@lumeet lumeet deleted the 1487 branch January 27, 2015 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants