Asp Forum - Bugs in Facets Cloneable?

Ken Bloom

1/22/2008 4:08:00 PM

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?

--Ken

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

5 Answers

Trans

1/22/2008 10:15:00 PM

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?).

Could anyone else with a bit more assuredness about Ruby's behavior
confirm this or offer the proper perspective if it is wrong?

Thanks,
T.

Ken Bloom

1/23/2008 12:38:00 AM

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...

Trans

1/23/2008 10:07:00 PM

On Jan 22, 7:39 pm, Ken Bloom <kbl...@gmail.com> wrote:
> 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

Nice. Makes sense to me. I will work on this a bit and incorporate --
with due credit.

Thanks Ken,
T.

Trans

2/6/2008 2:52:00 PM

On Jan 22, 7:39 pm, Ken Bloom <kbl...@gmail.com> wrote:

> 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

Thanks for this Ken. I've adopted this code as given --I could not
think of better way to determine if dup or clone was being used. If
anyone knows of a more robust way to determine dup vs. clone please
let me know.

Thanks,
T.

Dave Thomas

2/6/2008 3:12:00 PM

On Feb 6, 2008, at 8:52 AM, Trans wrote:

>> 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.

Yes, that was an oversight, and one that I'll fix in the new edition.

Dave

comp.lang.ruby

Bugs in Facets Cloneable?

Ken Bloom

Trans

Ken Bloom

Trans

Trans

Dave Thomas

x Login to ForumsZone