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

Already on GitHub? Sign in to your account

overriding "y" in kernel causes strange behavior in parslet #50

Closed
ghost opened this Issue Mar 4, 2012 · 7 comments

Comments

2 participants
@ghost

ghost commented Mar 4, 2012

In core_ext.rb, psych monkeypatches Kernel to remove the "y" method and replace it with the "psych_y" method, which prints the object as YAML. I'm using Parslet to transform a grammar. Parslet uses a "context" object that inherits from BlankSlate to support automatically binding variables in the context of the transformation. I created a parser like this:

transformer = Parslet::Transform.new do
rule(y: simple(:y)) {
y
}
end

Parslet uses method_missing to dynamically bind variables. When I call the "y" method it should look up "y" in the list of bindings and return it. That code looks like this:

def method_missing(sym, *args, &block)
super unless args.empty?
super unless @bindings.has_key?(sym.to_sym)

@bindings[sym]
end

But since y is getting overridden for every object, this instead triggers psych to print out the object. This happens in any app that requires "psych" at any level, which includes anything running inside rails or using activesupport. This caused a huge amount of painful debugging in the course of upgrading to 1.9.3. Would it be possible to not add that method to Kernel? Or at least do something less dangerous, it's not apparent at all what's happening when this crops up. I don't think having a method that prints something is worthy of being patched into every object.

@ghost

ghost commented Mar 4, 2012

test case: https://gist.github.com/1974317 (sorry, i tried to include it a couple times but didn't know what I was doing)

Owner

tenderlove commented Mar 4, 2012

I'm not totally opposed, but y has been defined on kernel since 1.8 (even when requiring syck):

[aaron@higgins rails (master)]$ ruby -v -ryaml -e'p method(:y)'
ruby 1.8.7 (2010-01-10 patchlevel 249) [universal-darwin11.0]
#<Method: Object(Kernel)#y>
[aaron@higgins rails (master)]$

Do you have suggestions for a replacement that would be backwards compatible, yet still provide the y method?

@ghost

ghost commented Mar 4, 2012

I'm not sure if there's a way to provide the y method without adding it to the Kernel module. There is an easy way to fix it from the Parslet side: the Context class extends from BlankSlate, but in the future it should probably extend BasicObject (this prevents Kernel from being included and fixes the problem). I'm guessing it's using BlankSlate for now for backwards compatibility reasons; BasicObject was only added in 1.9.3.

This only manifests itself in 1.9.3 because Psych became the default yml parser. I'm questioning the usefulness of this method, given that it's added as private and would probably only be used for debugging purposes (though admittedly, I'm new to Psych and haven't gone too deep into anything that uses it). Maybe add a deprecation warning when that method is called, and then removing it in a future release?

Owner

tenderlove commented Mar 4, 2012

I'm guessing if you do require "syck" on 1.9.3, you'll get the same issue. I don't think it's unique to just psych.

I think we could push the y method up to Object and everything would still work, but I'd rather just remove the method. The only time I use this method is inside irb, so maybe conditionally loading it if irb is present would be a sufficient compromise.

kschiess commented Mar 5, 2012

I'll fix it on the parslet side as well. This kind of namespace pollution is rather frequent in Ruby libraries and really hinders interop. My fix will be to really define methods instead of using method_missing.

Join the holy crusade: jimweirich/rake#81

;)

@ghost

ghost commented Mar 5, 2012

I'm fine with whatever works - the irb solution sounds fine, if there's an easy way to detect that. I've already patched the code on my end, so I'm not too worried about it; just wanted to make sure that nobody would run into the same issue.

kschiess commented Mar 5, 2012

The newest release of parslet (1.3.0) contains the fix.

@tenderlove tenderlove closed this in 6914073 Mar 5, 2012

@utensil utensil referenced this issue in bundler/bundler May 28, 2013

Closed

attempts on loading 'psych' pollutes Kernel #2486

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