Hi Chris,

On Wed, Aug 29, 2018 at 7:33 PM Chris Muller <asqueaker at gmail.com> wrote:

> 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!
>

Imagine this:

Object subclass: #Fu
           instanceVariableNames: ''.

Fu subclass: #Bar
           instanceVariableNames: 'fubar'.

and we want to refactor to this:

Object subclass: #Fu
           instanceVariableNames: 'fubar'.

Fu subclass: #Bar
           instanceVariableNames: ''.

either we provide a class builder that can morph more than one class at a
time or we have a problem.

If we refactor via this route:

1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar
instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar
instanceVariableNames: '' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar
instanceVariableNames: '' }
then fubar becomes undeclared (which is a single l-value in Undeclared),
and hence all instances of Bar but the first one have their inst var trashed

If we refactor via this route:
2.
1. { Object subclass: #Fu instanceVariableNames: ''. Fu subclass: #Bar
instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar
instanceVariableNames: 'fubar' }
=> { Object subclass: #Fu instanceVariableNames: 'fubar'. Fu subclass: #Bar
instanceVariableNames: '' }

we have a DuplicateVariableException to handle, but state could conceivably
be preserved across the refactoring.

Does that explain the problem?

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!
> >
> >
>
>

-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20180831/f0dd736c/attachment.html>