On Tue, Jan 20, 2015 at 12:26 PM, Eliot Miranda <eliot.miranda at gmail.com>
wrote:

> Hi Chris, Hi Colin,
>
>
> On Thu, Jan 15, 2015 at 2:48 PM, Chris Muller <ma.chris.m at gmail.com>
> wrote:
>
>> I did a minor class-hierarchy refactoring today which resulted in a an
>> invalid super pointer, resulting in a confusing image lock up due to
>> tight recursion.  The script, below, demonstrates the issue in trunk:
>>
>> Object compile: 'test  ^''Hello from Object'''.
>> (Object subclass: #MyClass instanceVariableNames: ''
>> classVariableNames: '' poolDictionaries: '' category: 'Test') compile:
>> 'test  ^ super halt test, ''my super call lands on myself!'''.
>> (Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.
>> ((Smalltalk classNamed: #MyNewAbstraction) subclass: #MyClass
>> instanceVariableNames: '' classVariableNames: '' poolDictionaries: ''
>> category: 'Test').
>> (Smalltalk classNamed: #MyNewAbstraction) new test
>>
>> But an existing method in the original MyClass had a call to super
>> which now lands in itself, resulting in a tight recursion.
>>
>> The same script without creating the new subclass results in an
>> instant VM crash (both interpreter 2357 and cog 3205):
>>
>> Object compile: 'test  ^''Hello from Object'''.
>> (Object subclass: #MyClass instanceVariableNames: ''
>> classVariableNames: '' poolDictionaries: '' category: 'Test') compile:
>> 'test  ^ super halt test, ''my super call lands on itself!'''.
>> (Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.
>> (Smalltalk classNamed: #MyNewAbstraction) new test
>>
>> I even tried recompiling my entire class hierarchy which was renamed,
>> but it didn't help.
>>
>> How can this be?  I _KNOW_ I've used rename class many times in the
>> past...  A regression?
>>
>
> This is indeed an Environments regression.  The issue is where to fix the
> problem.  One way to look at it is that Environment>>renameClass:from:to:
> does not update bindings in the renamed class's methods.
>
> Here's what the old code did:
>
> SystemDictionary>>renameClass: aClass from: oldName to: newName
> "Rename the class, aClass, to have the title newName."
>
> | oldref category |
> category := SystemOrganization categoryOfElement: oldName.
> self organization classify: newName under: category suppressIfDefault:
> true.
> self organization removeElement: oldName.
> oldref := self associationAt: oldName.
> self removeKey: oldName.
> ** oldref key: newName.
> self add: oldref.  "Old association preserves old refs"
> Smalltalk renamedClass: aClass from: oldName to: newName.
> self flushClassNameCache.
> SystemChangeNotifier uniqueInstance classRenamed: aClass from: oldName to:
> newName inCategory: category
>
> The starred line changes the key on the old class's binding, meaning that
> all the methods whose methodClassAssociation is that binding get
> automatically updated (note that class methods have
> a methodClassAssociation that has no key and just points to the class
> object).
>
> The new code simply forgets to do this:
>
> Environment>>renameClass: aClass from: oldName to: newName
> "Rename the class, aClass, to have the title newName."
>
> | binding category |
> category := self organization categoryOfElement: oldName.
> self organization classify: newName under: category suppressIfDefault:
> true.
> self organization removeElement: oldName.
>  binding := self declarationOf: oldName.
> declarations removeKey: oldName.
> self binding: binding removedFrom: self.
>  binding := newName => aClass.
> declarations add: binding.
> self binding: binding addedTo: self.
>  Smalltalk renamedClass: aClass from: oldName to: newName.
> SystemChangeNotifier uniqueInstance
> classRenamed: aClass
> from: oldName
> to: newName
> inCategory: category
>
>
> But should the change go in Environment>>renameClass:from:to: or in the
> BindingPolicy?  i.e. it could be implemented either in
> Environment>>renameClass:from:to:, or in Environment>>showBinding:.  I
> guess the former, but I'm cc'ing Colin, because know it?, he wrote it!
>

and if the fix should indeed go in Environment>>renameClass:from:to: , it
is as simple as

Environment>>renameClass: aClass from: oldName to: newName
"Rename the class, aClass, to have the title newName."

| binding category |
category := self organization categoryOfElement: oldName.
self organization classify: newName under: category suppressIfDefault: true.
self organization removeElement: oldName.
 binding := self declarationOf: oldName.
declarations removeKey: oldName.
self binding: binding removedFrom: self.
 binding := newName => aClass.
declarations add: binding.
self binding: binding addedTo: self.

** aClass updateMethodBindingsTo: binding.
 Smalltalk renamedClass: aClass from: oldName to: newName.
SystemChangeNotifier uniqueInstance
classRenamed: aClass
from: oldName
to: newName
inCategory: category

I'll make this change and we can alter later if Colin thinks it's in the
wrong place.


>
> In any case there needs to be a test that checks that a method's
> methodClassAssociation is correctly updated after a rename.  I bet Colin
> would have not made this slip had there been a test.
>
>
> Here's the code I used to test this:
>
> | mca1 mca2 |
> Object compile: 'test  ^''Hello from Object'''.
> (Object
> subclass: #MyClass instanceVariableNames: ''
> classVariableNames: ''
> poolDictionaries: ''
> category: 'Test')
> compile: 'test  ^ super halt test, ''my super call lands on itself!'''.
> mca1 := ((Smalltalk classNamed: #MyClass)>>#test) methodClassAssociation.
> (Smalltalk classNamed: #MyClass)
> rename: 'MyNewAbstraction'.
> mca2 := ((Smalltalk classNamed: #MyNewAbstraction)>>#test)
> methodClassAssociation.
> { mca1. mca2. mca1 == mca2. mca1 == (Smalltalk bindingOf:
> #MyNewAbstraction). mca2 == (Smalltalk bindingOf: #MyNewAbstraction)}
>
> It answers { #MyClass=>nil . #MyClass=>nil . true . false . false}
> i.e. the binding isn't updated, but changes from #MyClass=>MyClass to
> #MyClass=>nil in binding:removedFrom:.
>
>
> --
> HTH,
> Eliot
>



-- 
best,
Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20150120/767b0b1a/attachment.htm