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

Binding bug #1758

Closed
subbuss opened this issue Jun 20, 2014 · 11 comments
Closed

Binding bug #1758

subbuss opened this issue Jun 20, 2014 · 11 comments
Assignees
Milestone

Comments

@subbuss
Copy link
Contributor

@subbuss subbuss commented Jun 20, 2014

class C; end

C.class_eval do
end

top_b = binding
C.new.instance_eval do
eval "p self", top_b
end

This prints 'C' in jruby (1.7, master, all modes) instead of 'main'.

@subbuss
Copy link
Contributor Author

@subbuss subbuss commented Jun 21, 2014

Simpler test case:

class C; end
C.class_eval {}
eval "p self", binding

@subbuss
Copy link
Contributor Author

@subbuss subbuss commented Jun 21, 2014

This is another area that probably requires cloneBlockAndFrame() instead of just cloneBlock(). See git commit 901b47a for another instance where this got fixed. This patch fixes the test case.

private Block setupBlock(Block block) {
// FIXME: This is an ugly hack to resolve JRUBY-1381; I'm not proud of it

  •    block = block.cloneBlock();
    
  •    block = block.cloneBlockAndFrame();
    

block.getBinding().setSelf(this);

subbuss added a commit that referenced this issue Jun 21, 2014
* Related: see commit 901b47a
* Added a spec to prevent regressions.
@subbuss subbuss closed this Jun 21, 2014
@atambo
Copy link
Member

@atambo atambo commented Jun 22, 2014

Looks like this broke a couple rubyspecs in spec/ruby/language/defined_spec.rb:

https://s3.amazonaws.com/archive.travis-ci.org/jobs/28130557/log.txt

@atambo atambo reopened this Jun 22, 2014
mkristian added a commit that referenced this issue Jun 23, 2014
@subbuss subbuss added the core label Jun 23, 2014
subbuss added a commit that referenced this issue Jun 24, 2014
* Related: see commit 901b47a
* Added a spec to prevent regressions.
@headius
Copy link
Member

@headius headius commented Oct 20, 2014

This may be related to a discovery by "mvz" on Twitter: https://twitter.com/mvz/status/524271516251611136

Patch that remedies it, but is untested other than for this case: https://gist.github.com/headius/98fdf1f7e8a6c3b037b9

Problem is that we modify the frame to have a different self, and we never set it back. Frame is not cloned, so subsequent calls to the block execute with the wrong self.

This is a fatal bug, since it breaks any block passed to instance_eval. People must not use blocks like this very often.

@mvz
Copy link

@mvz mvz commented Oct 20, 2014

See #2060 for my specific case.

@os97673
Copy link

@os97673 os97673 commented Oct 24, 2014

People must not use blocks like this very often.

ruby-debug-base which is used to debug JRuby in RubyMine (and NetBeans) uses it to get value of object's fields). See https://youtrack.jetbrains.com/issue/RUBY-15864 :(
So, it would be nice to either have this fixed or have some alternative way to implement this functionality.

@os97673
Copy link

@os97673 os97673 commented Nov 22, 2014

@headius any plans to fix this? It would be great to be able to say our users when the problem is supposed to be fixed.

@headius
Copy link
Member

@headius headius commented Dec 2, 2014

@os97673 I'll fix it for 1.7.17, which we'll release later this week.

@headius headius self-assigned this Dec 2, 2014
@headius headius added this to the JRuby 1.7.17 milestone Dec 2, 2014
@ninkibah
Copy link

@ninkibah ninkibah commented Dec 2, 2014

@headius Oh thank you so much! If you get to Ireland in your travels, I owe you at least one beer!

@headius
Copy link
Member

@headius headius commented Dec 2, 2014

Bleh...of course I put the wrong issue number in the commit. Fixed in 17c67a6. Test added to MRI in ruby/ruby@0b31b7c. Looking to see what we have for specs.

@headius headius closed this Dec 2, 2014
@os97673
Copy link

@os97673 os97673 commented Dec 2, 2014

@headius any plans to fix this? It would be great to be able to say our users when the problem is supposed to be fixed.

great news, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants