Skip to content

Commit

Permalink
If we undef <=>, it solves a problem involving comparison. Please fig…
Browse files Browse the repository at this point in the history 
…ure out the root cause.
  • Loading branch information
@wycats
wycats committed Jun 8, 2010
1 parent 94ed39d commit 0042f41
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions activesupport/lib/active_support/multibyte/chars.rb
View file
Expand Up @@ -51,6 +51,8 @@ def initialize(string) #:nodoc:
end
end

undef <=>

# Forward all undefined methods to the wrapped string.
def method_missing(method, *args, &block)
if method.to_s =~ /!$/
Expand Down

9 comments on commit 0042f41

@radar
Copy link
Contributor

@radar radar commented on 0042f41 Jun 8, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that this "fixes" #4772: http://github.com/rails/rails/commit/0042f4166f783085eb909d69d542b5323d8af5d6 but it would be nice to have a better understanding of why it fixes it.

@deanmao
Copy link

@deanmao deanmao commented on 0042f41 Jun 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doh, this "undef" broke my tests... the "<=>" method doesn't exist, so it can't undef it. (I'm on edge, ruby 1.9.1p378 (2010-01-10 revision 26273)).

It looks like "<=>" is defined further down the file, so maybe the undef is too called too early?

@visoft
Copy link

@visoft visoft commented on 0042f41 Jun 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my tests (ruby 1.9.1p378 and rails beta 4), <=> doesn't exist, so the undef throws an error. Same problem as deanmao

@boof
Copy link

@boof boof commented on 0042f41 Jun 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is, imho, caused because of the way compar.c calls the spaceship:

static VALUE
cmp_eq(VALUE *a)
{
    VALUE c = rb_funcall(a[0], cmp, 1, a[1]);
    ...

ruby-1.9.2-head >
  BrokenComparable = Module.new { def ==(other) self <=> other end }
  BrokenClass = Class.new { include BrokenComparable }
  BrokenClass.new == nil
SystemStackError: stack level too deep

The current implementation of the spaceship operator ...

static VALUE
rb_mod_cmp(VALUE mod, VALUE arg)
{
    *snip*
    cmp = rb_class_inherited_p(mod, arg);
    ...

... tries to lookup the method in the parent classes but fails because it's
actually implemented in the Kernel module:

ruby-1.9.2-head > Object.instance_methods(false)
 => [] 
ruby-1.9.2-head > Object.superclass.instance_methods(false)
 => [:==, :equal?, :!, :!=, :instance_eval, :instance_exec, :__send__] 
ruby-1.9.2-head > Object.superclass.superclass.methods(false)
 => [] 
ruby-1.9.2-head > Object.superclass.superclass.class.instance_methods(false)
 => [:to_i, :to_f, :to_s, :to_a, :inspect, :&, :|, :^, :nil?, :to_r, :rationalize, :to_c] 
ruby-1.9.2-head > Object.superclass.superclass.class.superclass.instance_methods(false)
 => [] 
ruby-1.9.2-head > Object.superclass.superclass.class.superclass.superclass.instance_methods(false)
 => [:==, :equal?, :!, :!=, :instance_eval, :instance_exec, :__send__] 
...

ruby-1.9.2-head > Kernel.instance_methods(false)
 => [:nil?, :===, :=~, :!~, :eql?, :hash, :<=>, :class, :singleton_class, :clone, :dup, :initialize_dup, :initialize_clone, :taint, :tainted?, :untaint, :untrust, :untrusted?, :trust, :freeze, :frozen?, :to_s, :inspect, :methods, :singleton_methods, :protected_methods, :private_methods, :public_methods, :instance_variables, :instance_variable_get, :instance_variable_set, :instance_variable_defined?, :instance_of?, :kind_of?, :is_a?, :tap, :send, :public_send, :respond_to?, :respond_to_missing?, :extend, :display, :method, :public_method, :define_singleton_method, :__id__, :object_id, :to_enum, :enum_for]

But I'm neither familiar with C nor the ruby source so this is just a guess.

@plukevdh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this actually seems to break functionality though, even if its allowing tests to pass. I start seeing an error because of this change: http://gist.github.com/431583

@plukevdh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, so reading more from lighthouse and here again, is this a 1.9.2 fix?

@boof
Copy link

@boof boof commented on 0042f41 Jun 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a problem to simply delegate it?

class ActiveSupport::Multibyte::Chars
  def <=>(other)
    @wrapped_string <=> other
  end
end

@deanmao
Copy link

@deanmao deanmao commented on 0042f41 Jun 9, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@boof well, the actual implementation is pretty much what you wrote, around line 102, but it only gets included if RUBY_VERSION < "1.9"

@boof
Copy link

@boof boof commented on 0042f41 Jun 10, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yepp, just delegating the method to the wrapped object should work around the buggy core implementation of Kernel#<=>.

Please sign in to comment.