On 20.01.2015, at 21:33, 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.
>  
> 



Eliot, be sure to check my changes before you do that.
We try to fix the same problem!

Best
	-Tobias




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