Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0001405 [Squeak] Kernel minor always 07-02-05 00:51 07-21-13 02:14
Reporter BGaertner View Status public  
Assigned To
Priority normal Resolution fixed  
Status closed   Product Version 3.8
Summary 0001405: [BUG] ClassBuilder cannot change class to variable subclass
Description 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.
Additional Information
Attached Files  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

- Relationships

- Notes
(0001705 - 826 - 1021 - 1065 - 1065 - 1065 - 1065)
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 - 1614 - 2137 - 2181 - 2181 - 2181 - 2181)
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 - 420 - 496 - 538 - 538 - 538 - 538)
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 - 2352 - 2760 - 2804 - 2804 - 2804 - 2804)
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 - 440 - 523 - 564 - 564 - 564 - 564)
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 - 253 - 315 - 359 - 359 - 359 - 359)
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 - 365 - 389 - 389 - 389 - 389 - 389)
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 - 112 - 112 - 112 - 112 - 112 - 112)
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.
 

- Issue History
Date Modified Username Field Change
07-02-05 00:51 KenCausey New Issue
07-02-05 00:51 KenCausey File Added: ClassBuilderCheck-bg.1.cs.gz
07-02-05 00:51 KenCausey Reporter KenCausey => BGaertner
07-02-05 00:51 KenCausey Note Added: 0001705
07-02-05 00:52 KenCausey File Added: ClassBuilderCheck-bg.3.cs.gz
07-02-05 00:53 KenCausey Note Added: 0001706
07-02-05 00:54 KenCausey Note Added: 0001707
07-02-05 00:54 KenCausey File Added: ClassBuilderCheck-bg.4.cs.gz
07-02-05 00:56 KenCausey Note Added: 0001708
07-02-05 00:57 KenCausey Note Added: 0001709
07-02-05 00:58 KenCausey File Added: ClassTypeChangeTest.4.cs.gz
07-02-05 00:58 KenCausey Note Added: 0001710
07-02-05 01:00 KenCausey Note Added: 0001711
10-25-05 20:53 BGaertner Issue Monitored: BGaertner
07-21-13 02:14 tim Status new => closed
07-21-13 02:14 tim Note Added: 0014390
07-21-13 02:14 tim Resolution open => fixed
07-21-13 02:14 tim Fixed in Version  => 4.4


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
76 total queries executed.
47 unique queries executed.
Powered by Mantis Bugtracker