This bug report is for Squeak 3.7 alpha latest update: 5623
When you create a subclass, you cannot change it later into a variable subclass.
To see what goes wrong, please try this:
1. Open a class browser and select class Time. 2. Replace the class name 'Time' with 'TestClass' to create a subclass of Magnitude. Select 'accept' from the view menu to create the subclass. 3. Replace the 'subclass:' with 'variableSubclass:' and 'accept' again to change the just created subclass into a variable subclass. This should be possible because 'TestClass' has no subclasses at all (subclasses impose restrictions to the possible type of a class) and because Magnitude is a fixed-sized class.
You will get a error notifier saying: 'TestClass cannot be compiled'.
The problem is, as far as I can see, in the method ClassBuilder>>validateSubclassFormat:from: forSuper:extra:
Here we have a check that reads:
oldClass withAllSubclassesDo: [:sub | <omission> "If we get this far, check whether the immediate subclasses of oldClass can keep its layout." (newType ~~ #normal) ifTrue: [ self validateSubclass: sub canKeepLayoutFrom: oldClass forSubclassFormat: newType ] ].
The troublemaker is the "withAllSubclassesDo:". This method answers a collection that contains *the receiver class* and all its subclasses. Therefore, for one evaluation of 'self validateSubclass: ...' the identity sub == oldClass holds, and this is clearly not intended. For the test, we need a collection that contains only the subclasses of the receiver class, but not the receiver class itself. Such a collection is obtained with method 'allSubclassesDo:'.
Attached you find a proposal for a fix. I think that it will not cause problems elsewhere but your advice is very welcome.
Greetings, Boris
Boris Gaertner Boris.Gaertner@gmx.net wrote a bug report and he did it quick and dirty:
The troublemaker is the "withAllSubclassesDo:". This method answers a collection that contains *the receiver class* and all its subclasses.
This is wrong. The correct statement should read:
This method iterates over a collection that contains *the receiver class* and all its subclasses.
For the test, we need a collection that contains only the subclasses of the receiver class, but not the receiver class itself. Such a collection is obtained with method 'allSubclassesDo:'.
This is also carelessly written, a better wording is:
To iterate over that collection of subclasses, we have to use 'allSubclassesDo:'
My apologies for the bad wording. Note that the poposed fixed that I posted earlier is nevertheless worth being examined.
Greetings again, Boris
From: Boris Gaertner Boris.Gaertner@gmx.net To: squeak-dev@lists.squeakfoundation.org Sent: Sunday, January 04, 2004 7:00 PM Subject: [BUG] ClassBuilder cannot change class to variable subclass
This bug report is for Squeak 3.7 alpha latest update: 5623
<details omitted>
Attached you find a proposal for a fix. I think that it will not cause problems elsewhere but your advice is very welcome.
That proposed fix is also wrong, attached you find something better.
The iteration method 'withAllSubclassesDo:' that I called a troublemaker, does two tests: 1. It checks the new number of named instance variables. 2. It validates the subclasses.
The first test is needed for oldClass and all subclasses, the second test is needed only for the subclasses. In the attached proposal, I do the first test for oldClass before I do both tests for all the subclasses.
It is helpful to compare with Squeak 3.4 #5170, where two iterations are used to keep the tests separate:
(di 11/24/1999) < ... > oldClass withAllSubclassesDo: [:sub | sub instSize + deltaSize > 254 ifTrue: [ self error: sub name,' has more than 254 instance variables'. ^ false]]. newType ~~ #normal ifTrue: ["And check if the immediate subclasses of oldClass can keep its layout" oldClass subclassesDo:[:sub| oldType _ sub typeOfClass. oldType == newType ifFalse: [ self error: sub name,' cannot be recompiled'. ^ false]]]. < ... >
Later these two iterations were merged into one and that was wrong: The two iterations do not iterate over the same collection of classes! The first iteration checks one class that is not checked by the second iteration.
Greetings, Boris
Hi boris
When we started KCP, we were looking at withAllSubclassesDo: we realise that the order of iterations could be really important. This is a non-functional requirement that is fun but dangerous. I did not have the time to look at your changes now but I'm guessing that the order is really important. Is my hypothesis true?
If this is the case we should really document that.
Stef
On 5 janv. 04, at 02:36, Boris Gaertner wrote:
From: Boris Gaertner Boris.Gaertner@gmx.net To: squeak-dev@lists.squeakfoundation.org Sent: Sunday, January 04, 2004 7:00 PM Subject: [BUG] ClassBuilder cannot change class to variable subclass
This bug report is for Squeak 3.7 alpha latest update: 5623
<details omitted> > Attached you find a proposal for a fix. I think that it > will not cause problems elsewhere but your advice > is very welcome.
That proposed fix is also wrong, attached you find something better.
The iteration method 'withAllSubclassesDo:' that I called a troublemaker, does two tests:
- It checks the new number of named instance variables.
- It validates the subclasses.
The first test is needed for oldClass and all subclasses, the second test is needed only for the subclasses. In the attached proposal, I do the first test for oldClass before I do both tests for all the subclasses.
It is helpful to compare with Squeak 3.4 #5170, where two iterations are used to keep the tests separate:
(di 11/24/1999) < ... > oldClass withAllSubclassesDo: [:sub | sub instSize + deltaSize > 254 ifTrue: [ self error: sub name,' has more than 254 instance variables'. ^ false]]. newType ~~ #normal ifTrue: ["And check if the immediate subclasses of oldClass can keep its layout" oldClass subclassesDo:[:sub| oldType _ sub typeOfClass. oldType == newType ifFalse: [ self error: sub name,' cannot be recompiled'. ^ false]]]. < ... >
Later these two iterations were merged into one and that was wrong: The two iterations do not iterate over the same collection of classes! The first iteration checks one class that is not checked by the second iteration.
Greetings, Boris
<ClassBuilderCheck-bg.3.cs>
From: ducasse ducasse@iam.unibe.ch
Hi boris
When we started KCP, we were looking at withAllSubclassesDo: we realise that the order of iterations could be really important. This is a non-functional requirement that is fun but dangerous. I did not have the time to look at your changes now but I'm guessing that the order is really important. Is my hypothesis true?
I think it is not.
I compared earlier versions of the ClassBuilder with the current one and I think now, that both my proposals to fix the bug are bad. My first proposal is wrong and my second one is bad because it does unnecessary tests. Let me explain this in greater detail.
As I wrote last night (Jan 04), an earlier version of ClassBuilder>>validateSubclassFormat:from:forSuper:extra: used two iteration statements to keep two tests separate. This is a very important point, but yesterday I failed to fully understand it.
The first test checks the number of named instance variables. This test is needed for the modified class and all its subclasses. For this test the use of "withAllSubclassesDo:" is correct and the order of the tests is irrelevant.
The second test verifies that, with the new format, oldClass is general enough to be the superclass of its subclasses. Here it is sufficient to check the direct subclasses. This is also said in the comment that survived the changes that came with change set 5300: "And check if the immediate subclasses of oldClass can keep its layout" Here the use of "subclassesDo:" is sufficient and "subclassesDo:" iterates over a collection that is often smaller than the collection that "allSubclassesDo:" iterates over. (It is not necessary to check the subclasses of the subclasses because this was done for every subclass when it was defined or modified for the last time.)
I think that we should go back to the use of two iteration statements. The attached third proposal of a fix supersedes my two earlier proposals. (The proposal was of course checked against the tests that Andreas posted yesterday. Thank you, Andreas!)
I feel sorry that I needed three attempts and four mails to come up with a reasonably good proposal, but the stuff is really a bit tricky. Comments are therefore most welcome.
Greetings, Boris
Hi boris
When we started KCP, we were looking at withAllSubclassesDo: we realise that the order of iterations could be really important. This is a non-functional requirement that is fun but dangerous. I did not have the time to look at your changes now but I'm guessing that the order is really important. Is my hypothesis true?
I think it is not.
I compared earlier versions of the ClassBuilder with the current one and I think now, that both my proposals to fix the bug are bad. My first proposal is wrong and my second one is bad because it does unnecessary tests. Let me explain this in greater detail.
As I wrote last night (Jan 04), an earlier version of ClassBuilder>>validateSubclassFormat:from:forSuper:extra: used two iteration statements to keep two tests separate. This is a very important point, but yesterday I failed to fully understand it.
The first test checks the number of named instance variables. This test is needed for the modified class and all its subclasses. For this test the use of "withAllSubclassesDo:" is correct and the order of the tests is irrelevant.
The second test verifies that, with the new format, oldClass is general enough to be the superclass of its subclasses. Here it is sufficient to check the direct subclasses. This is also said in the comment that survived the changes that came with change set 5300: "And check if the immediate subclasses of oldClass can keep its layout" Here the use of "subclassesDo:" is sufficient and "subclassesDo:" iterates over a collection that is often smaller than the collection that "allSubclassesDo:" iterates over. (It is not necessary to check the subclasses of the subclasses because this was done for every subclass when it was defined or modified for the last time.)
I think that we should go back to the use of two iteration statements. The attached third proposal of a fix supersedes my two earlier proposals. (The proposal was of course checked against the tests that Andreas posted yesterday. Thank you, Andreas!)
I feel sorry that I needed three attempts and four mails to come up with a reasonably good proposal, but the stuff is really a bit tricky. Comments are therefore most welcome.
But the classBuilder is a beast. I was rewritten in VisualWorks and at least you try. I tend to avoid to face it :)
Stef
Greetings, Boris <ClassBuilderCheck-bg.4.cs>
Hi Boris,
I feel sorry that I needed three attempts and four mails to come up with a reasonably good proposal, but the stuff is really a bit tricky. Comments are therefore most welcome.
Only one comment: Since this stuff is indeed tricky, I would very much appreciate a test which documents the problem you've noticed. Having a test for each bug found will ensure we avoid it in future changes.
Cheers, - Andreas
----- Original Message ----- From: "Boris Gaertner" Boris.Gaertner@gmx.net To: "The general-purpose Squeak developers list" squeak-dev@lists.squeakfoundation.org Sent: Monday, January 05, 2004 9:04 PM Subject: Re: [BUG] ClassBuilder cannot change class to variable subclass
From: ducasse ducasse@iam.unibe.ch
Hi boris
When we started KCP, we were looking at withAllSubclassesDo: we realise that the order of iterations could be really important. This is a non-functional requirement that is fun but dangerous. I did not have the time to look at your changes now but I'm guessing that the order is really important. Is my hypothesis true?
I think it is not.
I compared earlier versions of the ClassBuilder with the current one and I think now, that both my proposals to fix the bug are bad. My first proposal is wrong and my second one is bad because it does unnecessary tests. Let me explain this in greater detail.
As I wrote last night (Jan 04), an earlier version of ClassBuilder>>validateSubclassFormat:from:forSuper:extra: used two iteration statements to keep two tests separate. This is a very important point, but yesterday I failed to fully understand it.
The first test checks the number of named instance variables. This test is needed for the modified class and all its subclasses. For this test the use of "withAllSubclassesDo:" is correct and the order of the tests is irrelevant.
The second test verifies that, with the new format, oldClass is general enough to be the superclass of its subclasses. Here it is sufficient to check the direct subclasses. This is also said in the comment that survived the changes that came with change set 5300: "And check if the immediate subclasses of oldClass can keep its layout" Here the use of "subclassesDo:" is sufficient and "subclassesDo:" iterates over a collection that is often smaller than the collection that "allSubclassesDo:" iterates over. (It is not necessary to check the subclasses of the subclasses because this was done for every subclass when it was defined or modified for the last time.)
I think that we should go back to the use of two iteration statements. The attached third proposal of a fix supersedes my two earlier proposals. (The proposal was of course checked against the tests that Andreas posted yesterday. Thank you, Andreas!)
I feel sorry that I needed three attempts and four mails to come up with a reasonably good proposal, but the stuff is really a bit tricky. Comments are therefore most welcome.
Greetings, Boris
---------------------------------------------------------------------------- ----
From: Andreas Raab andreas.raab@gmx.de wrote:
I feel sorry that I needed three attempts and four mails to come up with a reasonably good proposal, but the stuff is really a bit tricky. Comments are therefore most welcome.
Only one comment: Since this stuff is indeed tricky, I would very much appreciate a test which documents the problem you've noticed. Having a
test
for each bug found will ensure we avoid it in future changes.
Cheers,
- Andreas
You are right. Attached you find a test. It follows the structure of your tests and it would be possible to merge these tests into one class.
Greetings, Boris
squeak-dev@lists.squeakfoundation.org
-
Andreas Raab -
Boris Gaertner -
ducasse