Mantis - Squeak
Viewing Issue Advanced Details
1405 Kernel minor always 07-02-05 00:51 07-21-13 02:14
BGaertner  
 
normal  
closed 3.8  
fixed  
none    
none 4.4  
0001405: [BUG] ClassBuilder cannot change class to variable subclass
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.
 ClassBuilderCheck-bg.1.cs.gz [^] (743 bytes) 07-02-05 00:51
 ClassBuilderCheck-bg.3.cs.gz [^] (772 bytes) 07-02-05 00:52
 ClassBuilderCheck-bg.4.cs.gz [^] (785 bytes) 07-02-05 00:54
 ClassTypeChangeTest.4.cs.gz [^] (667 bytes) 07-02-05 00:58

Notes
(0001705)
KenCausey   
07-02-05 00:51   
"Boris Gaertner" <Boris.Gaertner@gmx.net>:

"> 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."
(0001706)
KenCausey   
07-02-05 00:53   
(This not refers to ClassBuilderCheck-bg.3.cs.gz)

"Boris Gaertner" <Boris.Gaertner@gmx.net>:

"> 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."
(0001707)
KenCausey   
07-02-05 00:54   
ducasse <ducasse@iam.unibe.ch>:

"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."
(0001708)
KenCausey   
07-02-05 00:56   
(This not is in reference to ClassBuilderCheck-bg.4.cs.gz)

"Boris Gaertner" <Boris.Gaertner@gmx.net>:

"> 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."
(0001709)
KenCausey   
07-02-05 00:57   
"Andreas Raab" <andreas.raab@gmx.de>:

"> 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."
(0001710)
KenCausey   
07-02-05 00:58   
(This note is in reference to ClassTypeChangeTest.4.cs.gz)

"Boris Gaertner" <Boris.Gaertner@gmx.net>:

"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."
(0001711)
KenCausey   
07-02-05 01:00   
(This is really Ken)

Let me note that there does still seem to be an issue in 3.8-6665. I tried out the original example and although I did not get an error it would not let me accept the change of subclass type.

I did not feel confident about being able to review this any further and decided to just post it as is. So I cannot comment on any of the fixes.
(0014390)
tim   
07-21-13 02:14   
The original problem no longer occurs; I'm going to assume that the fix was developed and included in the image.