<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 20, 2015 at 12:26 PM, Eliot Miranda <span dir="ltr"><<a href="mailto:eliot.miranda@gmail.com" target="_blank">eliot.miranda@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Chris, Hi Colin,<div><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Jan 15, 2015 at 2:48 PM, Chris Muller <span dir="ltr"><<a href="mailto:ma.chris.m@gmail.com" target="_blank">ma.chris.m@gmail.com</a>></span> wrote:<br></span><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">I did a minor class-hierarchy refactoring today which resulted in a an<br>
invalid super pointer, resulting in a confusing image lock up due to<br>
tight recursion. The script, below, demonstrates the issue in trunk:<br>
<br>
Object compile: 'test ^''Hello from Object'''.<br>
(Object subclass: #MyClass instanceVariableNames: ''<br>
classVariableNames: '' poolDictionaries: '' category: 'Test') compile:<br>
'test ^ super halt test, ''my super call lands on myself!'''.<br>
(Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.<br>
((Smalltalk classNamed: #MyNewAbstraction) subclass: #MyClass<br>
instanceVariableNames: '' classVariableNames: '' poolDictionaries: ''<br>
category: 'Test').<br>
(Smalltalk classNamed: #MyNewAbstraction) new test<br>
<br>
But an existing method in the original MyClass had a call to super<br>
which now lands in itself, resulting in a tight recursion.<br>
<br>
The same script without creating the new subclass results in an<br>
instant VM crash (both interpreter 2357 and cog 3205):<br>
<br>
Object compile: 'test ^''Hello from Object'''.<br>
(Object subclass: #MyClass instanceVariableNames: ''<br>
classVariableNames: '' poolDictionaries: '' category: 'Test') compile:<br>
'test ^ super halt test, ''my super call lands on itself!'''.<br>
(Smalltalk classNamed: #MyClass) rename: 'MyNewAbstraction'.<br>
(Smalltalk classNamed: #MyNewAbstraction) new test<br>
<br>
I even tried recompiling my entire class hierarchy which was renamed,<br>
but it didn't help.<br>
<br>
How can this be? I _KNOW_ I've used rename class many times in the<br>
past... A regression?<br></blockquote><div><br></div></div></div><div>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.</div><div><br></div><div>Here's what the old code did:</div><div><br></div><div>SystemDictionary>>renameClass: aClass from: oldName to: newName</div><div><span style="white-space:pre-wrap"> </span>"Rename the class, aClass, to have the title newName."</div><div><br></div><div><span style="white-space:pre-wrap"> </span>| oldref category |</div><div><span style="white-space:pre-wrap"> </span>category := SystemOrganization categoryOfElement: oldName.</div><div><span style="white-space:pre-wrap"> </span>self organization classify: newName under: category suppressIfDefault: true.</div><div><span style="white-space:pre-wrap"> </span>self organization removeElement: oldName.</div><div><span style="white-space:pre-wrap"> </span>oldref := self associationAt: oldName.</div><div><span style="white-space:pre-wrap"> </span>self removeKey: oldName.</div><div><span style="white-space:pre-wrap">** </span>oldref key: newName.</div><div><span style="white-space:pre-wrap"> </span>self add: oldref. "Old association preserves old refs"</div><div><span style="white-space:pre-wrap"> </span>Smalltalk renamedClass: aClass from: oldName to: newName.</div><div><span style="white-space:pre-wrap"> </span>self flushClassNameCache.</div><div><span style="white-space:pre-wrap"> </span>SystemChangeNotifier uniqueInstance classRenamed: aClass from: oldName to: newName inCategory: category </div><div><br></div><div>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).</div><div><br></div><div>The new code simply forgets to do this:</div><div><br></div><div><div>Environment>>renameClass: aClass from: oldName to: newName</div><div><span style="white-space:pre-wrap"> </span>"Rename the class, aClass, to have the title newName."</div><div><br></div><div><span style="white-space:pre-wrap"> </span>| binding category |</div><div><span style="white-space:pre-wrap"> </span>category := self organization categoryOfElement: oldName.</div><div><span style="white-space:pre-wrap"> </span>self organization classify: newName under: category suppressIfDefault: true.</div><div><span style="white-space:pre-wrap"> </span>self organization removeElement: oldName.</div><div><span style="white-space:pre-wrap"> </span></div><div><span style="white-space:pre-wrap"> </span>binding := self declarationOf: oldName.</div><div><span style="white-space:pre-wrap"> </span>declarations removeKey: oldName.</div><div><span style="white-space:pre-wrap"> </span>self binding: binding removedFrom: self.</div><div><span style="white-space:pre-wrap"> </span></div><div><span style="white-space:pre-wrap"> </span>binding := newName => aClass.</div><div><span style="white-space:pre-wrap"> </span>declarations add: binding. </div><div><span style="white-space:pre-wrap"> </span>self binding: binding addedTo: self.</div><div><span style="white-space:pre-wrap"> </span></div><div><span style="white-space:pre-wrap"> </span>Smalltalk renamedClass: aClass from: oldName to: newName.</div><div><span style="white-space:pre-wrap"> </span>SystemChangeNotifier uniqueInstance </div><div><span style="white-space:pre-wrap"> </span>classRenamed: aClass </div><div><span style="white-space:pre-wrap"> </span>from: oldName </div><div><span style="white-space:pre-wrap"> </span>to: newName </div><div><span style="white-space:pre-wrap"> </span>inCategory: category</div></div><div><br></div><div><br></div><div>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!</div></div></div></div></div></blockquote><div><br></div><div>and if the fix should indeed go in Environment>>renameClass:from:to: , it is as simple as</div><div><br></div><div>Environment>>renameClass: aClass from: oldName to: newName</div><div><span class="" style="white-space:pre"> </span>"Rename the class, aClass, to have the title newName."</div><div><br></div><div><span class="" style="white-space:pre"> </span>| binding category |</div><div><span class="" style="white-space:pre"> </span>category := self organization categoryOfElement: oldName.</div><div><span class="" style="white-space:pre"> </span>self organization classify: newName under: category suppressIfDefault: true.</div><div><span class="" style="white-space:pre"> </span>self organization removeElement: oldName.</div><div><span class="" style="white-space:pre"> </span></div><div><span class="" style="white-space:pre"> </span>binding := self declarationOf: oldName.</div><div><span class="" style="white-space:pre"> </span>declarations removeKey: oldName.</div><div><span class="" style="white-space:pre"> </span>self binding: binding removedFrom: self.</div><div><span class="" style="white-space:pre"> </span></div><div><span class="" style="white-space:pre"> </span>binding := newName => aClass.</div><div><span class="" style="white-space:pre"> </span>declarations add: binding. </div><div><span class="" style="white-space:pre"> </span>self binding: binding addedTo: self.</div><div><br></div><div><span class="" style="white-space:pre">** </span>aClass updateMethodBindingsTo: binding.</div><div><span class="" style="white-space:pre"> </span></div><div><span class="" style="white-space:pre"> </span>Smalltalk renamedClass: aClass from: oldName to: newName.</div><div><span class="" style="white-space:pre"> </span>SystemChangeNotifier uniqueInstance </div><div><span class="" style="white-space:pre"> </span>classRenamed: aClass </div><div><span class="" style="white-space:pre"> </span>from: oldName </div><div><span class="" style="white-space:pre"> </span>to: newName </div><div><span class="" style="white-space:pre"> </span>inCategory: category</div><div><br></div><div>I'll make this change and we can alter later if Colin thinks it's in the wrong place.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>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.</div><div><br></div><div><br></div><div>Here's the code I used to test this:</div><div><br></div><div><div>| mca1 mca2 |</div><span class=""><div>Object compile: 'test ^''Hello from Object'''.</div><div>(Object</div><div><span style="white-space:pre-wrap"> </span>subclass: #MyClass instanceVariableNames: ''</div><div><span style="white-space:pre-wrap"> </span>classVariableNames: ''</div><div><span style="white-space:pre-wrap"> </span>poolDictionaries: ''</div><div><span style="white-space:pre-wrap"> </span>category: 'Test')</div><div><span style="white-space:pre-wrap"> </span>compile: 'test ^ super halt test, ''my super call lands on itself!'''.</div></span><div>mca1 := ((Smalltalk classNamed: #MyClass)>>#test) methodClassAssociation.</div><span class=""><div>(Smalltalk classNamed: #MyClass)</div><div><span style="white-space:pre-wrap"> </span>rename: 'MyNewAbstraction'.</div></span><div>mca2 := ((Smalltalk classNamed: #MyNewAbstraction)>>#test) methodClassAssociation.</div><div>{ mca1. mca2. mca1 == mca2. mca1 == (Smalltalk bindingOf: #MyNewAbstraction). mca2 == (Smalltalk bindingOf: #MyNewAbstraction)}</div></div><div><br></div><div>It answers { #MyClass=>nil . #MyClass=>nil . true . false . false}</div><div>i.e. the binding isn't updated, but changes from #MyClass=>MyClass to #MyClass=>nil in binding:removedFrom:.</div><span class=""><font color="#888888"><div></div></font></span></div><span class=""><font color="#888888"><div><br></div><div><br></div>-- <br><div>HTH,<div>Eliot</div></div>
</font></span></div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature">best,<div>Eliot</div></div>
</div></div>