On Fri, 21 Sep 2012, Eliot Miranda wrote:

> 
> 
> On Fri, Sep 21, 2012 at 1:13 PM, Levente Uzonyi <leves at elte.hu> wrote:
>       I guess we're going off the track with class renaming. The problem I'd like to solve is:
>       If we remove a class and then add a class to the system with the same name, then the methods which were referring to the class before the
>       removal are still referring to the same old class. This means that two bindings will coexist in the system with the same class name, but
>       pointing to different classes.
> 
> 
> Let me get this clear.  Surely the methods should still refer to the class named X after removing and adding back a class named X right?  That can (and
> should) be accomplished by moving the binding to Undeclared on removing it and usiong the binding when declaring the filed-in class.  This is the way the
> system used to work and should continue to work.

Right, this is how it should work, but the problem is that it's not 
working that way now (and I think it didn't in the past few years).

> 
>
>       How is class renaming related to this?
>       One of the possible solutions is to move the removed class's binding to Undeclared. When you rename a class, then Class >> #rename: will check
>       if there's a binding with the new class name in Undeclared. The problem here is that Undeclared doesn't release bindings automatically (when
>       they are not referenced by any methods anymore), so bindings gone long ago are still lingering in Undeclared, therefore Class >> #rename: (and
>       also Trait
>                   #rename:) will #inform: the user about it, which breaks one of the
>
>       tests.
> 
> 
> But that's not a problem.  The set-up for these tests can (and should) prune unreferenced bindings from Undeclared, either before running or before
> complaining of there being bindings in Undeclared.

It's not the tests which are complaining, but Class >> #rename:. If you 
patch SystemDictionary >> #forgetClass:logged: to move the binding to 
Undeclared and you run SystemChangeFileTest >> #testClassRenamed then 
when the code gets to Class >> #rename: the second time, then at the line

 	(Undeclared includesKey: newName)

there will be a binding in Undeclared which points to an unreferenced, 
already removed class.

Class >> #rename: aString
 	"The new name of the receiver is the argument, aString."

 	| oldName newName |
 	(newName := aString asSymbol) = (oldName := self name)
 		ifTrue: [^ self].
 	(self environment includesKey: newName)
 		ifTrue: [^ self error: newName , ' already exists'].
 	(Undeclared includesKey: newName)
 		ifTrue: [self inform: 'There are references to, ' , aString printString , 'from Undeclared. Check them after this change.'].
 	name := newName.
 	self environment renameClass: self from: oldName

The only solution I see to the issue is to perform #removeUnreferencedKeys 
before sending #includesKey: (or we can be tricky, and do it only if we 
find that the binding is there and recheck after to see if it's really 
there or not). And yes, I think it's a problem that Undeclared is holding 
onto that reference, because every place which uses #includesKey: or 
#at:ifAbsent: might get false information.

> 
>  
>       But there's another problem with this solution:
>       There's a test which tests if a removed class's method is still referencing to the class. So replacing the value of the binding with nil is
>       not possible.
> 
> 
> But that's a bogus test.  Of course the binding's value should be set to nil.  Either the class has been removed or renamed.  The only thing that makes

So a CompiledMethod should return nil for #methodClass if its class was 
removed instead of the obsolete Class object? (see CompiledMethodTest >> 
#testMethodClass)

> sense with remove is to set it to nil.  Rename is more difficult.  It makes sense to leave it pointing to the new class if the system is running
> interactively while the user edits methods to refer to the class under the new name. Tricky.
>
>       If we don't replace the value with nil, then the binding isn't really undeclared. So there will be bindings in Undeclared with non-nil value,
>       which might have side effects in the system.
>
>       How does the solution I suggested work:
>       - when a class is removed, then its binding will be thrown away
>       - all methods referring to the class are recompiled. This means that if any such method exists, then a new binding will be created in
>       Undeclared and all methods will use that new binding.
>       The drawback is that mass class removal will be slower. Removing a single class will have no noticable difference for the user (20-40ms+the
>       recompilation time).
> 
> 
> I don't see that this is usefully different from the existing behaviour.  There seem to me to be two cases, one is renaming a class that is key to the
> system functioning, say some class in the compiler.  If the binding is set to nil then the system will break on trying to compile edited versions of the
> methods that refer to the class.  That's tough, but one has to accept it.  The work-around is to clone the class under the new name (e.g. file-out, edit,
> file back in) and move all subclasses under the new class, and then remove the old class.  The other case is where the class isn't key and the system
> keeps running.  In either case, keeping the exsting binding and setting its value to nil is the correct behaviour; it maintains system integrity w.r.t.
> bindings.

My suggested solution only applies to class removal, I don't want to touch 
class renaming.

>  
>
>       For completeness, here's a list of problems I don't want to solve now:
>       - Undeclared holding onto bindings forever
> 
> 
> non-problem.  tests that test Undeclared must prune unreferenced bindings from Undeclared.  The cost is small.  F**ing up Undeclared to make the tests run
> fast is a case of the tail wagging the dog.

It's not about the tests at all, but the system and the way Undeclared is 
being used (see above). I had no intention to make any test faster, I just 
want them to pass. :)
And I think the way Undeclared is implemented is just wrong.

>  
>       - Undeclared introducing Associations for classes instead of ReadOnlyVariableBindings
> 
> 
> Again if the binding is preserved this won't happen.

Using #at:put: will create an Association, because Undeclared is just and 
IdentityDictionary.

>  
>       - how class renaming works
> 
> 
> has it changed?

I don't think so and I don't really want to deal with it right now, Colin 
will do it in 4.5 anyway.


Levente