Kernel-cmm.1190.mcz is my proposed fix for the bug identified by the
new #testDuplicateInstanceVariableError, for your review.

But while working on that I was shocked to notice what
testMoveVarFromSubToSuperclass is doing and testing for.  Resuming the
DuplicateVariableException so it could, truly, make interrogations
about an object that shouldn't exist (right?).  Very strange!

Best,
  Chris

On Wed, Aug 29, 2018 at 9:20 PM <commits at source.squeak.org> wrote:
>
> Chris Muller uploaded a new version of Kernel to project The Inbox:
> http://source.squeak.org/inbox/Kernel-cmm.1190.mcz
>
> ==================== Summary ====================
>
> Name: Kernel-cmm.1190
> Author: cmm
> Time: 29 August 2018, 9:20:01.027647 pm
> UUID: b5a78e5d-c7fe-4e5d-9673-ee6208534ae1
> Ancestors: Kernel-eem.1189
>
> - Remove unnecessary string literals from ClassBuilder>>#reservedNames.
> - Fix and simplify ClassBuilder>>#validateInstvars:from:forSuper:.
>         ClassBuilder>>#validateClassvars:from:forSuper: may benefit from a similar change.
>
> =============== Diff against Kernel-eem.1189 ===============
>
> Item was changed:
>   ----- Method: ClassBuilder>>reservedNames (in category 'private') -----
>   reservedNames
>         "Return a list of names that must not be used for variables"
> +       ^#(self super thisContext #true #false #nil)!
> -       ^#('self' 'super' 'thisContext' 'true' 'false' 'nil'
> -               self super thisContext #true #false #nil).!
>
> Item was changed:
>   ----- Method: ClassBuilder>>validateInstvars:from:forSuper: (in category 'validation') -----
> + validateInstvars: instVarArray from: oldClass forSuper: newSuper
> - validateInstvars: instVarArray from: oldClass forSuper: newSuper
>         "Check if any of the instVars of oldClass conflict with the new superclass"
> +       | usedNames |
> -       | instVars usedNames temp |
> -       instVarArray isEmpty ifTrue:[^true]. "Okay"
> -       newSuper allowsSubInstVars ifFalse: [
> -               self error: newSuper printString, ' does not allow subclass inst vars. See allowsSubInstVars.'. ^ false].
> -
> -       "Validate the inst var names"
>         usedNames := instVarArray asSet.
> +       oldClass ifNotNil: [ oldClass withAllSubclassesDo: [ : each | each == oldClass ifFalse: [usedNames addAll: each instVarNames] ] ].
> +       "Anything to possibly conflict?"
> +       usedNames isEmpty ifTrue: [ ^ true ].
> +       newSuper allowsSubInstVars ifFalse:
> +               [ self error: newSuper printString , ' does not allow subclass inst vars. See allowsSubInstVars.'.
> +               ^ false ].
> +       instVarArray asSet size = instVarArray size ifFalse:
> +               [ self error: instVarArray asBag sortedCounts first value, ' is multply defined.'.
> +               ^ false ].
> +       (usedNames intersection: self reservedNames) ifNotEmpty:
> +               [ : reservedWords | self error: reservedWords anyOne , ' is a reserved name'.
> +               ^ false ].
> +       newSuper ifNotNil:
> +               [ | offendingVars |
> +               "If any variable names in subclasses conflict names in the new superclass, report the offending class and instVar name."
> +               newSuper withAllSuperclasses
> +                       detect: [ : each | (offendingVars := each instVarNames intersection: usedNames) notEmpty ]
> +                       ifFound:
> +                               [ : offendingSuperclass |
> +                               DuplicateVariableError new
> +                                        superclass: offendingSuperclass ;
> +                                        variable: offendingVars anyOne ;
> +                                        signal: offendingVars anyOne , ' is already defined in ' , offendingSuperclass name ]
> +                       ifNone: [ "no name conflicts" ] ].
> +       ^ true!
> -       usedNames size = instVarArray size
> -               ifFalse:[       instVarArray do:[:var|
> -                                       usedNames remove: var ifAbsent:[temp := var]].
> -                               self error: temp,' is multiply defined'. ^false].
> -       (usedNames includesAnyOf: self reservedNames)
> -               ifTrue:[        self reservedNames do:[:var|
> -                                       (usedNames includes: var) ifTrue:[temp := var]].
> -                               self error: temp,' is a reserved name'. ^false].
> -
> -       newSuper == nil ifFalse:[
> -               usedNames := newSuper allInstVarNames asSet.
> -               instVarArray do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               newSuper withAllSuperclassesDo:[:cl|
> -                                       (cl instVarNames includes: iv) ifTrue:[temp := cl]].
> -                               (DuplicateVariableError new)
> -                                       superclass: temp;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in ', temp name]]].
> -       oldClass == nil ifFalse:[
> -               usedNames := Set new: 20.
> -               oldClass allSubclassesDo:[:cl| usedNames addAll: cl instVarNames].
> -               instVars := instVarArray.
> -               newSuper == nil ifFalse:[instVars := instVars, newSuper allInstVarNames].
> -               instVars do:[:iv|
> -                       (usedNames includes: iv) ifTrue:[
> -                               (DuplicateVariableError new)
> -                                       superclass: oldClass;
> -                                       variable: iv;
> -                                       signal: iv,' is already defined in a subclass of ', temp name]]].
> -       ^true!
>
>