Ugh, I'm behind the times, Tobias you've already fixed it; thanks!  Tobias,
I like your use of becomeForward:; it is more robust.  But I *think* the
undeclared removal is misplaced.  Don't you think that that code belongs in
the BindingPolicy?

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

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



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