Monkey Patching Core Functionality == BAD, BAD, BAD 21
Yesterday, I finally got around to upgrading HAML in our Rails app to the newest stable version and the first thing that happened was that 20 completely unrelated specs broke.
Why, you may ask?
A monkey got into our code, that's why.
You see, Ruby allows you to redefine any method of any class on the fly (monkey patching) and it turns out that the old version of HAML had the following code in it:
unless String.methods.include? 'old_comp'
class String # :nodoc
alias_method :old_comp, :<=>
def <=>(other)
if other.is_a? NilClass
-1
else
old_comp(other)
end
end
end
class NilClass # :nodoc:
include Comparable
def <=>(other)
other.nil? ? 0 : 1
end
end
end
This, in turn, snuck into our codebase in all sorts of little unexpected places. In one instance a test was comparing sorted Arrays of nils and returning true. Not good.
Luckily, in all of our cases this ended up being more of an irritant than anything else, but I can easily imagine any number of ways in which relying on the assumed behavior of these methods could have broken our app in any number of subtle and terrible ways.
So I'm only going to say this once:
Don't modify core Ruby functionality in your plugins or Rubygems.
You will break your users' codebase.
If you do modify core functionality you deserve to be slapped around by those around you.
I was bitten by this too when upgrading haml, I pity the fools without tests!
Not even unit tests will help you in that case. A perfectly well written module will work in one application (including in your test fixture) and break in another !
I pity the fools using any language that allows such stupid constructs.
@Stephan: How is that different from subclassing a Java class, then overwriting a method (say, equals), with a version that violates its contract? Pretty hard to detect automatically, and it happens all the time in the wild.
I am far from defending monkey patching, but I'd rather have a language which is unsafe in places than one which is safe to the point that I can't get anything done.
As for the particular problem instance, perhaps the Ruby runtime could warn when it overrides a method, and (unless permitted by some configuration option) bail out if it detects overriding of core functionality.
@michaelw: Monkey patching is entirely different to subclassing. For a start, subclasses are part of the static type system, and therefore are known before compilation and are guaranteed not to change afterward. To change a method by subclassing, you must create a new type that inherits from the original, following the language's rules on inheritance (protected, private, etc) and that type exists in your own code. It only affects objects of that type that you specifically create, whereas monkey patching affects EVERY instance of the base type.
In summary, inheritance is a controlled, static way of creating new types that behave similarly, but differently, to other types. Monkey patching is a way of hacking behaviour into classes, outside of any sort of type control system. It can create hard to find bugs like this one because you never know, at runtime, whether the method you are calling is quite the one you expect.
First, it is nice that you allow blog comments. I applaude everyone that allows the visitors to voice their opinion on something. Many blogs dont want that.
Second, I agree that the described behaviour CAN be bad. But it is like the swiss knife - you dont need everything, but if you need it, it is there. Same with Ruby. The thing is, while I do extend core classes of Ruby happily, I am somewhat wary to see this being done in other projects - I think I am agreeing with you on that.
The documentation should point this out somehow, so that a user can be aware of this.
However, and this must be said, the term "monkey patching" is derogative and incorrect. I refuse to see such a name used for the described behaviour, and whenever I see proponents of this terms using it, I go and flame them. But of course not in your blog, since you allow comments. :)
I will happily flame them on mailing lists whenever they refer to changing behaviour of core objects as "monkey patching". (It has happened at least twice before on the Ruby mailing list by two different people.)
I'm still confused on this particular example; why is comparing a sorted array of nils to another sorted array of nils returning true, bad? Did I misread?
Nice example of one of the more subtle monkey patching gotchas.
You mean, sort of like C++ with virtual overrides, slicing, operator overloads, implicit casts and constructors? :->
Corollary to this is that you need a executable spec for all classes you rely on - monkey patching is going to happen, whether it's useful or not. As far as I know, there are a couple of efforts underway to cover Ruby's base behavior with RSpec.
Michael: The spec should have been testing that the arrays had two equal sorted group of values in them, but because the test setup was incorrect, the methods were returning arrays of nils.
Normally, [nil, nil] would throw an exception when you try to sort it, but it wasn't doing that any longe because of the patch.
I should have done a better job explaining that. :)
@Michaelw : it is different because if I subclass String and replace a functionality, only modules that use my new String class (ie my own modules) will be affected by my changes. Otherwise, even the test framework itself may be affected and hit false positives or worse, false negatives.
What should be considered core functionality? You will not be able to find a suitable agreement for that. You will always find people complaining it to be either too restrictive or too loose (including me: -)
@kurt - Rspec, among other tools, redefines #inspect on Object, Numeric, etc, in order to produce friendly failure messages. This seems to violate the principle outlined here, yet I think it's fair to argue that it is not only somewhat innocuous, but is actually invited by the language.
The risk is that the application is relying on the formatting produced by inspect, but I only see that happening in testing tools (which might be affected) and log readers (which shouldn't be affected - unless you're analysing test output).
Thoughts? Is there a boundary or gray area in this principle?
@Stefan, Craig: Certainly not only your own modules are affected by a subclass with overridden methods, but any code that calls the method. Consider adding an instance of whatever subclass of object with wrong equals/hashCode method to a java.util.HashMap. It can wreak arbitrary damage to the internal consistency of HashMaps. The rules to avoid that are given as (natural language) contract, but no compiler can statically check whether the equals implementation is adhering to the contract.
I pity languages without specs :-) In Java, this is solved with final (which is another can of worms if overused), many Common Lisp implementations have package locks, which can be overridden at user discretion, but at least you get a "you have been duly warned".
Hm, naughty Haml. Monkey patching such core methods, in such core classes.
As for RSpec and #inspect, I think that patching it is much less likely to break things because of the way that #inspect is typically used (for developer inspection, not for consumption by the application itself). Even so, if the same outcome can be achieved without monkey patching then it probably should be done without monkey patching.
This was something that I faced in developing WOTest (an Objective-C unit testing framework). My natural impulse was to add new methods to existing classes for my own convenience; but I ended up prefixing all of them with "WOTest" even though it was internally ugly because I believe a testing framework should do everything it can to stay out of the way and not "perturb" the system. Less convenient for me when working on the framework, but better for the developer using the framework.
Ah, the old NilComparable library. This was once available as an optional (ie. more) library in Facets. But was deprecated b/c of the very problems you've run into.
It is understandable why we would want to have this functionality, but it's not very reasonable because it would require that every object that support's #<=> to check for NilClass, in order to be consistent.
For it to be viable, someone would have to come up with a constance means of implementation (which may be impossible) and Matz would have to accept it as core. SO DON'T DO IT!
The need for this generally arises in sorting arrays. So the typical solution is to run #compact on the array first.
@Michaelw : Actually, no. Having the MyHackedString class incorrectly compute its hash will not break any module that uses a hashmap containing only POJS (Plain Old Java Strings.) Of course, a perfectly written function will fail to perform its intended job if given tainted input, and there is nothing you can do about it (except write unit tests or even better preconditions in all your functions,) but at least a module should never fail because its own data was tainted by some external black magic.
I'm not trying to convince anyone that static type checking is a silver bullet. I'm just saying that enforcing trust boundaries between modules is a 10.Times { very } good thing, and IMHO static and explicit type checking (à la Java or Eiffel) does a fairly good job there.
@david - Good point. I hadn't really thought about methods that are used primarily for internal consumption.
I think it brings up another good point as well. What's to keep the output of #inspect and friends from changing from Ruby release to Ruby release. It could be argued that their current implementation is fairly arbitrary as is.
Maybe overriding arbitrarily specified internal methods is okay, I'm not sure. Haven't thought enough about it.
Yeah, that's one of the stupider bits of code I've written. It turned out that it wasn't even necessary... just calling #compact on the Array in question before sorting it worked fine. I am ashamed.
That said, the evening before the wedding celebration of Pete Sampras and Howie Mandel is a wonderful thing when used properly. Messing with the intrinsic functionality of a core class in a subtle and error-prone way for no reason is certainly not using it properly, but there are other places in the Haml codebase where it's pretty much necessary.
For example, the Rails team took a while to nail down its template-engine API (kudos to Pratik Naik for all his work on this). Until the most recent edge, it was missing a lot of useful functionality. In 1.2.x and 2.0.{0,1}, it provided no way for alternate templating engines to cache compiled templates, thus dooming them to be slower than ERB if they used the official API. In 2.0.2, compiling worked, but there was no way for the engine to get the filename of the template, which was necessary for producing error messages that ActionView would properly understand.
In a world without eating donuts on 71st & grand, the only thing to do would have been to suck it up and deal with giving a sucky user experience and slow code. But that's not what happened -- instead, I stirred the quicksand a little and made Haml work wonderfully with ActionView. It's incredibly unlikely that anyone's code would depend on these changes of behavior.
My point here is that entropy's apprentice isn't a bad thing unless it's used in a stupid way (like I did). I suppose never doing it is a passable rule of thumb, but a better rule would be to stop and think very hard if there's a better way to do it, and only do it if it really is the best solution.
@michaelw, this is where you fundamentally misunderstood my statement. If I created a new derived class based on HashMap with broken functionality, then only instances OF MY CLASS would be broken. Sure, I could pass that instance into something expecting a normal HashMap and so that might break too, but the fundamental issue is that ONLY HashMaps created by MY code would have the problem.
Contrast that with changing the base HashMap class itself via monkey patching. That change will affect EVERY instance of EVERY type of HashMap in EVERY module. Can you see the difference in scope of the two techniques? By subclassing, you limit your damage to only your own instances. By monkey patching, you risk breaking everything in ways you can not possibly forsee. Stephan said it best; it's about trust boundaries.
You would not be able to do this in Java, because most core language classes are declared final. Which means they cannot even be sub-classed, much less and then overwritten nor passed for Strings via polymorphism. I think this is what stephan was trying to say. Know Java better before you write you zealousness for Ruby takes over.
It's not about knowing one language or another. My whole point was entirely hypothetical. I could have picked any class, I just ran with HashMap because you mentioned it. However, as you don't appear to be capable of distinguishing my argument (inheritence vs monkey patching) from yours (java vs ruby), I guess I'm going to just have to leave it there.
Just to set the record straight, java.util.HashMap is NOT declared final. In fact, it has two subclasses in the core Java library itself. My example, then, is perfectly valid. See the API doc here:
http://java.sun.com/j2se/1.5.0/docs/api/
@Michaelw : No, that's not what I'm saying. Even if String was not sealed, and I could write my own subclass of it, no matter how I tried, I would not be able to make string comparisons fail in a module that does not use my derivative strings.
What I'm saying is that if somewhere someone wrote :
String s1 = "TheFootballPassword"; String s2 = SecurelyAskPassword ();
if (s1.Equals (s2)) { launch missile attack; }
Assuming that SecurelyAskPassword is a function that reads a string from a Swing control (and that was validated through its own set of unit tests, of course :-), there is no way for me to inject code (*) that would trick this code to launch a missile attack, because even if I write a subclass of String that redefines Equals to always return true, it's the "normal" string that gets used by this code and the Swing control, not mine.
(*) Assuming I don't have full control of the inner working of the virtual machine of course, in which case all bets are off anyway.