On Fri, 21 Sep 2012, Eliot Miranda wrote:
On Fri, Sep 21, 2012 at 1:13 PM, Levente Uzonyi leves@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 forevernon-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