[bug] Collection newWithAll: in 0.3.1

[Tim Olson]

On Dec 14, 2004, at 5:51 PM, Daniel John Crowe wrote:

>
> There is a missing period on the second last line in this method. 
> (Just looked at CVS and found this is fixed)
>
> I had trouble when trying to fix this in my image.
> I fixed collection.slate and then reloaded it, but the problem still 
> remained.  Note: I was using Set, but could not find any other 
> definitions of this method.
>
> Can you explain this to me?

Interesting problem -- it took some digging to find out what is 
actually going on.

I assume you mean the bug is in extensible.slate (ExtensibleCollection).

Looking at the bytecode for the various methods without the fix:

====
Slate 3> (Method findNamed: #newWithAll: on: {Set. {1. 2. 3}}) code.
{"ByteArray" 6. 1. 17. 32.
         34. 23. 22. 33.
         38. 17. 16. 32}
Slate 4> (Method findNamed: #newWithAll: on: {ExtensibleCollection. {1. 
2. 3}}) code.
{"ByteArray" 6. 1. 17. 32.
         34. 23. 22. 33.
         38. 17. 16. 32}
====

So we see that Set is using the same method defined in 
ExtensibleCollection.  If we load the fixed extensible.slate:

====
Slate 5> load: 'src/lib/extensible.slate'.
Loading 'src/lib/extensible.slate'
Nil
Slate 6> (Method findNamed: #newWithAll: on: {ExtensibleCollection. {1. 
2. 3}}) code.
{"ByteArray" 6. 1. 17. 32.
         34. 22. 33. 17.
         32. 39. 33}
Slate 7> (Method findNamed: #newWithAll: on: {Set. {1. 2. 3}}) code.
{"ByteArray" 6. 1. 17. 32.
         34. 23. 22. 33.
         38. 17. 16. 32}
====

So loading the new source has modified the method for 
ExtensibleCollection, but not for Set.  However, if we simply redefine 
the faulty method rather than loading all of extensible.slate, it works 
as expected.

The problem appears to be at the beginning of extensible.slate:

	collections addPrototype: #ExtensibleCollection derivedFrom: 
{Collection}.

which appears to be adding a new prototype without adjusting all 
previously-derived subtypes to point to the new prototype.

There is code in derivable.slate which looks like it should handle this 
particular case:

====
x@(Root traits) addPrototype: protoName derivedFrom: parents
"Creates a new prototype with the given name, handles derive calls
transparently, and sets the traits name slot to the name for 
convenience."
[| newProto |
   newProto: (parents size = 1
     ifTrue: [(parents at: 0) derive]
     ifFalse:
       [| others index |
         others: (parents newSize: parents size - 1).
         index: others size.
         [index = 0]
           whileFalse:
             [index: index - 1.
               others at: index put: (parents at: index + 1)].
         (parents at: 0) derive &mixins: others]).
   "ensureSlot:is: may return the old value if =, but always returns what
   is installed after the method is done."
   (x hasSlotNamed: protoName)
     ifTrue: [x ensureSlot: protoName is: newProto
                unlessSatisfies: [| :old | old traits delegateValues
                                    = newProto traits delegateValues]]
     ifFalse: [x addImmutableSlot: protoName valued: newProto].
====

The  ensureSlot:is:unlessSatisfies: should have handled this case, but 
the equality test between the old traits delegateValues and the 
newProto traits delegateValues fails because the order of the traits is 
different:

	old: delegateValues: {#traits. #parent0}
	new: delegateValues: {#parent0. #traits}

So it looks like we need to ensure that the delegateValues are returned 
in a canonical order for this code to do the expected thing.

	-- tim