Copying classes: breaking contract or not?

On May 12, 2012, at 12:05 PM, Mariano Martinez Peck wrote:

> Hi guys. The last literal of a instance-side compiled method always points to the SAME (#==) association of the class in Smalltalk global.
> The literals of a compiled method that refer to class variables refer to the same association (#==) of the classPool of its class:
>
> self assert: ((GRPlatform class >> #current) literalAt: 1) == (GRPlatform classPool associationAt: #Current).   -> true
>
> However, if I do a copy of the class:
>
> | copy |
> copy := GRPlatform copy.
> self assert: ((copy class >> #current) literalAt: 1) == (copy classPool associationAt: #Current).
>
> this is not true anymore. Of course, you can see this because of the Class >> #copy implementation:
>
> So my question is....is #copy breaking the contract (and therefore it has to be fixed) or this is not mandatory for the system (as it is with the last literal pointing to classes)?

since namespace binding are shared between method literal and namespace, I think that this is a bug.

Stef

>
> Thanks,
>
> --
> Mariano
> http://marianopeck.wordpress.com
>

Hi guys. The last literal of a instance-side compiled method always points to the SAME (#==) association of the class in Smalltalk global.
The literals of a compiled method that refer to class variables refer to the same association (#==) of the classPool of its class:

self assert: ((GRPlatform class >> #current) literalAt: 1) == (GRPlatform classPool associationAt: #Current).   -> true

However, if I do a copy of the class:

| copy |
copy := GRPlatform copy.
self assert: ((copy class >> #current) literalAt: 1) == (copy classPool associationAt: #Current).

this is not true anymore. Of course, you can see this because of the Class >> #copy implementation:

So my question is....is #copy breaking the contract (and therefore it has to be fixed) or this is not mandatory for the system (as it is with the last literal pointing to classes)?

It depends of the usage of copy...
If the purpose is to modify a class, then the copy will replace the
original in a short future, and you should better keep the
associations untouched and only change them in a final Namespace at:
className put: theClassCopy...
If the purpose is to create a new class, then yes, there's some
additional operation required.
But where will you bind the new association? Another namespace?

Nicolas

2012/5/13 Igor Stasenko <[hidden email]>:

> yes, this looks like a bug.
> But i would recompile all methods of copied class, so all these
> nuances is handled.
>
> On 12 May 2012 14:04, Mariano Martinez Peck <[hidden email]> wrote:
>>
>>
>> On Sat, May 12, 2012 at 12:41 PM, Stéphane Ducasse
>> <[hidden email]> wrote:
>>>
>>>
>>> On May 12, 2012, at 12:05 PM, Mariano Martinez Peck wrote:
>>>
>>> > Hi guys. The last literal of a instance-side compiled method always
>>> > points to the SAME (#==) association of the class in Smalltalk global.
>>> > The literals of a compiled method that refer to class variables refer to
>>> > the same association (#==) of the classPool of its class:
>>> >
>>> > self assert: ((GRPlatform class >> #current) literalAt: 1) ==
>>> > (GRPlatform classPool associationAt: #Current).   -> true
>>> >
>>> > However, if I do a copy of the class:
>>> >
>>> > | copy |
>>> > copy := GRPlatform copy.
>>> > self assert: ((copy class >> #current) literalAt: 1) == (copy classPool
>>> > associationAt: #Current).
>>> >
>>> > this is not true anymore. Of course, you can see this because of the
>>> > Class >> #copy implementation:
>>> >
>>> > So my question is....is #copy breaking the contract (and therefore it
>>> > has to be fixed) or this is not mandatory for the system (as it is with the
>>> > last literal pointing to classes)?
>>>
>>> since namespace binding are shared between method literal and namespace, I
>>> think that this is a bug.
>>>
>>
>> I did this workaround
>>
>> Class >> rebindClassVariablesInMethods
>>
>>     self class methodDict valuesDo: [ :aCompiledMethod |
>>         1 to: aCompiledMethod numLiterals-2 do: [ :index |
>>             | literal |
>>             literal := aCompiledMethod literalAt: index.
>>             literal isVariableBinding ifTrue: [
>>                 (self innerBindingOf: literal key) ifNotNil: [:aBinding |
>>                     aCompiledMethod literalAt: index put: aBinding
>>                 ]
>>             ]
>>         ]
>>     ]
>>
>>>
>>> Stef
>>> >
>>> > Thanks,
>>> >
>>> > --
>>> > Mariano
>>> > http://marianopeck.wordpress.com
>>> >
>>>
>>>
>>
>>
>>
>> --
>> Mariano
>> http://marianopeck.wordpress.com
>>
>
>
>
> --
> Best regards,
> Igor Stasenko.
>

On Sun, May 13, 2012 at 5:31 AM, Nicolas Cellier <[hidden email]> wrote:

It depends of the usage of copy...
If the purpose is to modify a class, then the copy will replace the
original in a short future, and you should better keep the
associations untouched and only change them in a final Namespace at:
className put: theClassCopy...
If the purpose is to create a new class, then yes, there's some
additional operation required.
But where will you bind the new association? Another namespace?

I agree.  If copy is supposed to create a fully-fledged copy then the Class>>copy method is broken in several ways:

- As Mariano noted, it doesn't copy CompiledMethods nor update their methodClass associations.

- It doesn't add the copy to the superclass's subclass

- the class has the same name as the original but isn't reachable from Smalltalk

But if the use case is some temporary transformation, as Nicholas notes, then the method is ok.  Except that "copy" is a really unhelpful name.  This is a specialised copy for certain circumstances and the selevtor should reflect that.  Anyone know what the use case this copy was written for is?

Nicolas

2012/5/13 Igor Stasenko <[hidden email]>:

> yes, this looks like a bug.
> But i would recompile all methods of copied class, so all these
> nuances is handled.
>
> On 12 May 2012 14:04, Mariano Martinez Peck <[hidden email]> wrote:
>>
>>
>> On Sat, May 12, 2012 at 12:41 PM, Stéphane Ducasse
>> <[hidden email]> wrote:
>>>
>>>
>>> On May 12, 2012, at 12:05 PM, Mariano Martinez Peck wrote:
>>>
>>> > Hi guys. The last literal of a instance-side compiled method always
>>> > points to the SAME (#==) association of the class in Smalltalk global.
>>> > The literals of a compiled method that refer to class variables refer to
>>> > the same association (#==) of the classPool of its class:
>>> >
>>> > self assert: ((GRPlatform class >> #current) literalAt: 1) ==
>>> > (GRPlatform classPool associationAt: #Current).   -> true
>>> >
>>> > However, if I do a copy of the class:
>>> >
>>> > | copy |
>>> > copy := GRPlatform copy.
>>> > self assert: ((copy class >> #current) literalAt: 1) == (copy classPool
>>> > associationAt: #Current).
>>> >
>>> > this is not true anymore. Of course, you can see this because of the
>>> > Class >> #copy implementation:
>>> >
>>> > So my question is....is #copy breaking the contract (and therefore it
>>> > has to be fixed) or this is not mandatory for the system (as it is with the
>>> > last literal pointing to classes)?
>>>
>>> since namespace binding are shared between method literal and namespace, I
>>> think that this is a bug.
>>>
>>
>> I did this workaround
>>
>> Class >> rebindClassVariablesInMethods
>>
>>     self class methodDict valuesDo: [ :aCompiledMethod |
>>         1 to: aCompiledMethod numLiterals-2 do: [ :index |
>>             | literal |
>>             literal := aCompiledMethod literalAt: index.
>>             literal isVariableBinding ifTrue: [
>>                 (self innerBindingOf: literal key) ifNotNil: [:aBinding |
>>                     aCompiledMethod literalAt: index put: aBinding
>>                 ]
>>             ]
>>         ]
>>     ]
>>
>>>
>>> Stef
>>> >
>>> > Thanks,
>>> >
>>> > --
>>> > Mariano
>>> > http://marianopeck.wordpress.com
>>> >
>>>
>>>
>>
>>
>>
>> --
>> Mariano
>> http://marianopeck.wordpress.com
>>
>
>
>
> --
> Best regards,
> Igor Stasenko.
>

--
best,

Eliot

On Tue, May 15, 2012 at 9:39 AM, Eliot Miranda <[hidden email]> wrote:

On Sun, May 13, 2012 at 5:31 AM, Nicolas Cellier <[hidden email]> wrote:

It depends of the usage of copy...
If the purpose is to modify a class, then the copy will replace the
original in a short future, and you should better keep the
associations untouched and only change them in a final Namespace at:
className put: theClassCopy...
If the purpose is to create a new class, then yes, there's some
additional operation required.
But where will you bind the new association? Another namespace?

I agree.  If copy is supposed to create a fully-fledged copy then the Class>>copy method is broken in several ways:

- As Mariano noted, it doesn't copy CompiledMethods nor update their methodClass associations.

- It doesn't add the copy to the superclass's subclass

- the class has the same name as the original but isn't reachable from Smalltalk

But if the use case is some temporary transformation, as Nicholas notes, then the method is ok.  Except that "copy" is a really unhelpful name.  This is a specialised copy for certain circumstances and the selevtor should reflect that.  Anyone know what the use case this copy was written for is?

It's clearly for the ClassBuilder.  See SystemNavigation new browseAllCallsOn: #copy localTo: ClassBuilder.  SO the comment could be more helpful, e.g.

copy 

"Answer a copy of the receiver without a list of subclasses.

 This copy is used by the ClassBuilder when mutating classes on redefinition.

 (SystemNavigation new browseAllCallsOn: #copy localTo: ClassBuilder)"

 

Nicolas

2012/5/13 Igor Stasenko <[hidden email]>:

> yes, this looks like a bug.
> But i would recompile all methods of copied class, so all these
> nuances is handled.
>
> On 12 May 2012 14:04, Mariano Martinez Peck <[hidden email]> wrote:
>>
>>
>> On Sat, May 12, 2012 at 12:41 PM, Stéphane Ducasse
>> <[hidden email]> wrote:
>>>
>>>
>>> On May 12, 2012, at 12:05 PM, Mariano Martinez Peck wrote:
>>>
>>> > Hi guys. The last literal of a instance-side compiled method always
>>> > points to the SAME (#==) association of the class in Smalltalk global.
>>> > The literals of a compiled method that refer to class variables refer to
>>> > the same association (#==) of the classPool of its class:
>>> >
>>> > self assert: ((GRPlatform class >> #current) literalAt: 1) ==
>>> > (GRPlatform classPool associationAt: #Current).   -> true
>>> >
>>> > However, if I do a copy of the class:
>>> >
>>> > | copy |
>>> > copy := GRPlatform copy.
>>> > self assert: ((copy class >> #current) literalAt: 1) == (copy classPool
>>> > associationAt: #Current).
>>> >
>>> > this is not true anymore. Of course, you can see this because of the
>>> > Class >> #copy implementation:
>>> >
>>> > So my question is....is #copy breaking the contract (and therefore it
>>> > has to be fixed) or this is not mandatory for the system (as it is with the
>>> > last literal pointing to classes)?
>>>
>>> since namespace binding are shared between method literal and namespace, I
>>> think that this is a bug.
>>>
>>
>> I did this workaround
>>
>> Class >> rebindClassVariablesInMethods
>>
>>     self class methodDict valuesDo: [ :aCompiledMethod |
>>         1 to: aCompiledMethod numLiterals-2 do: [ :index |
>>             | literal |
>>             literal := aCompiledMethod literalAt: index.
>>             literal isVariableBinding ifTrue: [
>>                 (self innerBindingOf: literal key) ifNotNil: [:aBinding |
>>                     aCompiledMethod literalAt: index put: aBinding
>>                 ]
>>             ]
>>         ]
>>     ]
>>
>>>
>>> Stef
>>> >
>>> > Thanks,
>>> >
>>> > --
>>> > Mariano
>>> > http://marianopeck.wordpress.com
>>> >
>>>
>>>
>>
>>
>>
>> --
>> Mariano
>> http://marianopeck.wordpress.com
>>
>
>
>
> --
> Best regards,
> Igor Stasenko.
>

--
best,

Eliot

--
best,

Eliot