From: Ken Bloom <kbloom@...>
Date: 2008-01-23T09:39:58+09:00
Subject: Re: Bugs in Facets Cloneable?

On Tue, 22 Jan 2008 17:14:55 -0500, Trans wrote:

> On Jan 22, 11:09 am, Ken Bloom <kbl...@gmail.com> wrote:
>> I was just looking at the Cloneable module from the Facets' library,
>> because it's useful, and noticed a couple of possible bugs. Is my
>> assessment of this correct?
>>
>> module Cloneable
>>   def clone
>>     sibling = self.class.new
>>     instance_variables.each do |ivar|
>>       value = self.instance_variable_get(ivar)
>>       sibling.instance_variable_set(ivar, value.dup) #rake_dup)
>>     end
>>     sibling
>>   end
>>   alias_method :dup, :clone
>> end
>>
>> Question 1: Shouldn't #clone use self.class.allocate instead of
>> self.class.new? Better yet, shouldn't the superclass's dup be called
>> somehow?
>>
>> Question 2: Shouldn't #clone and #dup be implemented differently, so
>> that #clone can freeze the newly created object?
> 
> Hmmm.. some good questions. This bit of code came from Rake a long time
> ago. I never really analysed it, and just accepted it at face value. I
> suspect the thing to do these days is to use #allocate and
> #initialize_copy instead.
> 
> I point out, if anyone is wondering, the difference between this and the
> built-in clone/dup is that the above dups the instance vars.
> 
> The difference between the two being that #dup carries over the tainted
> state while #clone carries over the tainted and frozen states
> (correct?). Since we are duplicating the instance vars here, there is no
> reason to consider taintedness. But frozen should still apply (yes?).

Basically, I think it should be possible for an object to encapsulate 
(and hide) member variables that should be copied (not shared) when the 
enclosing object is copied. Thus, a semi-deep copying version of #clone 
and #dup are in order. That seems to be what Cloneable is about.

Thanks for pointing out #initialize_copy. I didn't know it existed. Now I 
see it in PickAxe, but I think it's role has been greatly trivialized. 
Pickaxe only mentions it with regard to C extensions, empathetically says 
that it's only for C extensions, and completely overlooks the idea that 
an object might be hiding other kinds of information that should be 
copied, not shared between duplicates.

Might I suggest that instead of overriding #dup and #clone at all, 
Cloneable should just provide the following implementation for 
#initialize_copy, then we can make #dup and #clone will both behave 
properly by virtue of the fact that they descend from Object#dup and 
Object#clone:

module Cloneable
  def initialize_copy sibling
    #first duplicate my superclass' state. Note that if it's duplicating
    #instance variables, this will be overwritten, but this is important
    #because we could be dealing with a C extension with state hidden from
    #the Ruby interpreter
    super
    
    #we want to know if we're being dup'ed or clone'd, because we want to
    #preserve the state of our internals the same way our state is being
    #preserved.
    #(If we can't figure it out, we'll just use #dup)
    operation=caller.find{|x| x !~ /`initialize_copy'/}.
      match(/`(dup|clone)'/)[1] or :dup

    sibling.instance_variables.each do |ivar|
      value = sibling.instance_variable_get(ivar)

      #set my instance variable to be a #dup or #clone
      #or my sibling, depending on what's happening to me right now
      instance_variable_set(ivar, value.send(operation))
    end
  end
end

This will make the following test case work:

require 'test/unit'

class Foo
  include Cloneable
  def initialize
    @bar=[]
  end
  def bar_id
    @bar.object_id
  end
end


class TestCloneable < Test::Unit::TestCase
  def test_dup
    a=Foo.new
    b=a.dup
    assert_not_equal a.bar_id,b.bar_id

    a.taint
    b=a.dup
    assert b.tainted?, "b should be tainted"

    a.freeze
    b=a.dup
    assert !b.frozen?, "b should not be frozen"
  end
  def test_clone
    a=Foo.new
    b=a.clone
    assert_not_equal a.bar_id,b.bar_id

    a.taint
    b=a.dup
    assert b.tainted?, "b should be tainted"

    a.freeze
    b=a.clone
    assert b.frozen?, "b should be frozen"
  end
end



-- 
Ken (Chanoch) Bloom. PhD candidate. Linguistic Cognition Laboratory.
Department of Computer Science. Illinois Institute of Technology.
http://www.iit.edu/~kbloom1/