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!

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20150120/972ff052/attachment.htm