Interesting Decompiler bug

> See
> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>
> This bug was found by a newbie on pharo 1.1, but it's still there in 4.5
> trunk image
>
> Try to decompile this (no matter in which class)
>
> < aNumberWithUnits
>     (self compareUnits: aNumberWithUnits)
>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
> ifTrue: [^true] ifFalse: [^false]).]
>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>
> Thanks to the newbie!
> I would just have hated that!
> Learning from one's own error is great, but from a dysfunctional system not
> so :(

> On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
>> See
>> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>>
>> This bug was found by a newbie on pharo 1.1, but it's still there in 4.5
>> trunk image
>>
>> Try to decompile this (no matter in which class)
>>
>> < aNumberWithUnits
>>     (self compareUnits: aNumberWithUnits)
>>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
>> ifTrue: [^true] ifFalse: [^false]).]
>>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>>
>> Thanks to the newbie!
>> I would just have hated that!
>> Learning from one's own error is great, but from a dysfunctional system not
>> so :(
>
> Good bug catch! Here is a reduced version of the method that produces the
> same error when decompiled:
>
>
>   Foo>>decompilerBug
>       self value: (true
>           ifTrue: [^ true]
>           ifFalse: [^ false])

> The bug is that (true ifTrue:[^true] ifFalse:[^false]) won't push any value
> on the stack (same as emitEffect: rather than emitValue:)
> The decompiler wants to find the argument of self value: ... on the stack
> and it's not there.
>
> So either we should forbid such construct at compile time
> (emit an 'un-reachable code' error when emitValue: does not emit any value)
>
> Or patch Decompiler like mad to work around...
> (but Decompiler's methods are yet well below usual quality in term of
> complexity - long methods, many instance variables, incredibly complex
> state, ...)
>
> I agree, this is going to be a rare problem only for newbies, because an
> experimented Smalltalker will avoid writing unreachable code.
> So, the third solution is DO NOTHING, or more exactly add a test and an
> expected failure.
> Like that we'll say next time, hey bad luck, this is a known limitation.

> Nicolas
>
>
>
> 2013/6/20 Frank Shearar <[hidden email]>
>>
>> On 20 June 2013 02:12, David T. Lewis <[hidden email]> wrote:
>> > On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
>> >> See
>> >>
>> >> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>> >>
>> >> This bug was found by a newbie on pharo 1.1, but it's still there in
>> >> 4.5
>> >> trunk image
>> >>
>> >> Try to decompile this (no matter in which class)
>> >>
>> >> < aNumberWithUnits
>> >>     (self compareUnits: aNumberWithUnits)
>> >>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
>> >> ifTrue: [^true] ifFalse: [^false]).]
>> >>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>> >>
>> >> Thanks to the newbie!
>> >> I would just have hated that!
>> >> Learning from one's own error is great, but from a dysfunctional system
>> >> not
>> >> so :(
>> >
>> > Good bug catch! Here is a reduced version of the method that produces
>> > the
>> > same error when decompiled:
>> >
>> >
>> >   Foo>>decompilerBug
>> >       self value: (true
>> >           ifTrue: [^ true]
>> >           ifFalse: [^ false])
>>
>> I'm guessing that this is because
>>
>> true
>>   ifTrue: [^ true]
>>   ifFalse: [^ false]
>>
>> decompiles as
>>
>> true ifTrue: [^ true]
>> ^ false
>>
>> ?
>>
>> I'm not terribly worried about claims of a "dysfunctional system".
>> This bug is so severe that noone has noticed it before! I agree that
>> we need to fix the bug, but the original code makes no sense in the
>> first place. #value: is unreachable.
>>
>> frank
>>
>> > Dave
>> >
>> >
>>
>
>
>
>

>
> Or patch Decompiler like mad to work around...
> (but Decompiler's methods are yet well below usual quality in term of
> complexity - long methods, many instance variables, incredibly complex
> state, ...)
>
> I agree, this is going to be a rare problem only for newbies, because an
> experimented Smalltalker will avoid writing unreachable code.
> So, the third solution is DO NOTHING, or more exactly add a test and an
> expected failure.
> Like that we'll say next time, hey bad luck, this is a known limitation.
>
> Nicolas
>
>
>
> 2013/6/20 Frank Shearar <[hidden email]>
>
> > On 20 June 2013 02:12, David T. Lewis <[hidden email]> wrote:
> > > On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
> > >> See
> > >>
> > http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
> > >>
> > >> This bug was found by a newbie on pharo 1.1, but it's still there in 4.5
> > >> trunk image
> > >>
> > >> Try to decompile this (no matter in which class)
> > >>
> > >> < aNumberWithUnits
> > >>     (self compareUnits: aNumberWithUnits)
> > >>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
> > >> ifTrue: [^true] ifFalse: [^false]).]
> > >>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
> > >>
> > >> Thanks to the newbie!
> > >> I would just have hated that!
> > >> Learning from one's own error is great, but from a dysfunctional system
> > not
> > >> so :(
> > >
> > > Good bug catch! Here is a reduced version of the method that produces the
> > > same error when decompiled:
> > >
> > >
> > >   Foo>>decompilerBug
> > >       self value: (true
> > >           ifTrue: [^ true]
> > >           ifFalse: [^ false])
> >
> > I'm guessing that this is because
> >
> > true
> >   ifTrue: [^ true]
> >   ifFalse: [^ false]
> >
> > decompiles as
> >
> > true ifTrue: [^ true]
> > ^ false
> >
> > ?
> >
> > I'm not terribly worried about claims of a "dysfunctional system".
> > This bug is so severe that noone has noticed it before! I agree that
> > we need to fix the bug, but the original code makes no sense in the
> > first place. #value: is unreachable.
> >
> > frank
> >
> > > Dave
> > >
> > >
> >
> >

> On Thu, Jun 20, 2013 at 12:20:00PM +0200, Nicolas Cellier wrote:
>> The bug is that (true ifTrue:[^true] ifFalse:[^false]) won't push any value
>> on the stack (same as emitEffect: rather than emitValue:)
>> The decompiler wants to find the argument of self value: ... on the stack
>> and it's not there.
>>
>> So either we should forbid such construct at compile time
>> (emit an 'un-reachable code' error when emitValue: does not emit any value)
>
> The original code example (before I reduced it to a trivial version)
> was this:
>
>     < aNumberWithUnits
>         (self compareUnits: aNumberWithUnits)
>             ifTrue: [self value: (aNumberWithUnits value < self value
>                             ifTrue: [^ true]
>                             ifFalse: [^ false])]
>             ifFalse: [^ Error new signal: 'Incompatible unit types.']
>
> This does not have the unreachable branch, and it displays the same
> decompiler error.

> Dave
>
>
>
>>
>> Or patch Decompiler like mad to work around...
>> (but Decompiler's methods are yet well below usual quality in term of
>> complexity - long methods, many instance variables, incredibly complex
>> state, ...)
>>
>> I agree, this is going to be a rare problem only for newbies, because an
>> experimented Smalltalker will avoid writing unreachable code.
>> So, the third solution is DO NOTHING, or more exactly add a test and an
>> expected failure.
>> Like that we'll say next time, hey bad luck, this is a known limitation.
>>
>> Nicolas
>>
>>
>>
>> 2013/6/20 Frank Shearar <[hidden email]>
>>
>> > On 20 June 2013 02:12, David T. Lewis <[hidden email]> wrote:
>> > > On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
>> > >> See
>> > >>
>> > http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>> > >>
>> > >> This bug was found by a newbie on pharo 1.1, but it's still there in 4.5
>> > >> trunk image
>> > >>
>> > >> Try to decompile this (no matter in which class)
>> > >>
>> > >> < aNumberWithUnits
>> > >>     (self compareUnits: aNumberWithUnits)
>> > >>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
>> > >> ifTrue: [^true] ifFalse: [^false]).]
>> > >>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>> > >>
>> > >> Thanks to the newbie!
>> > >> I would just have hated that!
>> > >> Learning from one's own error is great, but from a dysfunctional system
>> > not
>> > >> so :(
>> > >
>> > > Good bug catch! Here is a reduced version of the method that produces the
>> > > same error when decompiled:
>> > >
>> > >
>> > >   Foo>>decompilerBug
>> > >       self value: (true
>> > >           ifTrue: [^ true]
>> > >           ifFalse: [^ false])
>> >
>> > I'm guessing that this is because
>> >
>> > true
>> >   ifTrue: [^ true]
>> >   ifFalse: [^ false]
>> >
>> > decompiles as
>> >
>> > true ifTrue: [^ true]
>> > ^ false
>> >
>> > ?
>> >
>> > I'm not terribly worried about claims of a "dysfunctional system".
>> > This bug is so severe that noone has noticed it before! I agree that
>> > we need to fix the bug, but the original code makes no sense in the
>> > first place. #value: is unreachable.
>> >
>> > frank
>> >
>> > > Dave
>> > >
>> > >
>> >
>> >
>
>>
>
>

> On 20 June 2013 12:58, David T. Lewis <[hidden email]> wrote:
> > On Thu, Jun 20, 2013 at 12:20:00PM +0200, Nicolas Cellier wrote:
> >> The bug is that (true ifTrue:[^true] ifFalse:[^false]) won't push any value
> >> on the stack (same as emitEffect: rather than emitValue:)
> >> The decompiler wants to find the argument of self value: ... on the stack
> >> and it's not there.
> >>
> >> So either we should forbid such construct at compile time
> >> (emit an 'un-reachable code' error when emitValue: does not emit any value)
> >
> > The original code example (before I reduced it to a trivial version)
> > was this:
> >
> >     < aNumberWithUnits
> >         (self compareUnits: aNumberWithUnits)
> >             ifTrue: [self value: (aNumberWithUnits value < self value
> >                             ifTrue: [^ true]
> >                             ifFalse: [^ false])]
> >             ifFalse: [^ Error new signal: 'Incompatible unit types.']
> >
> > This does not have the unreachable branch, and it displays the same
> > decompiler error.
>
> The #value: send to self is unreachable because both of the inner
> #ifTrue:ifFalse: blocks force a return.
>

> On Thu, 20 Jun 2013, Nicolas Cellier wrote:
>
>> The bug is that (true ifTrue:[^true] ifFalse:[^false]) won't push any
>> value on the stack (same as emitEffect: rather than emitValue:)
>> The decompiler wants to find the argument of self value: ... on the stack
>> and it's not there.
>>
>> So either we should forbid such construct at compile time
>> (emit an 'un-reachable code' error when emitValue: does not emit any
>> value)
>
>
> I think this is the right thing to do, but the problem is a bit more
> complex.
>
> The following doesn't break the decompier, but it won't be decompiled
> correctly, because the decompiler avoids creating branches:
>
> decompilerBug2
>
>         true ifTrue: [ ^1 ] ifFalse: [ ^1 ].
>         ^2

> The decompiler will produce this:
>
> decompilerBug2
>         true
>                 ifTrue: [^ 1].
>         ^ 1.
>         ^ 2
>
> Which can't be compiled anymore.
>
>
> The original bug also appears with #caseOf:otherwise: :
>
> decompilerBug3
>
>         self foo: (
>                 true
>                         caseOf: { [ false ] -> [ ^1 ] }
>                         otherwise: [ ^1 ])
>
> #caseOf: :
>
> decompilerBug4
>
>         self foo: (true caseOf: { [ false ] -> [  ^1 ] })
>
> and #repeat :
>
> decompilerBug5
>
>         self foo: [ ^self ] repeat
>
>
> Levente
>
>
>>
>> Or patch Decompiler like mad to work around...
>> (but Decompiler's methods are yet well below usual quality in term of
>> complexity - long methods, many instance variables, incredibly complex
>> state, ...)
>>
>> I agree, this is going to be a rare problem only for newbies, because an
>> experimented Smalltalker will avoid writing unreachable code.
>> So, the third solution is DO NOTHING, or more exactly add a test and an
>> expected failure.
>> Like that we'll say next time, hey bad luck, this is a known limitation.
>>
>> Nicolas
>>
>>
>>
>> 2013/6/20 Frank Shearar <[hidden email]>
>>       On 20 June 2013 02:12, David T. Lewis <[hidden email]> wrote:
>>       > On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
>>       >> See
>>       >>
>> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>>       >>
>>       >> This bug was found by a newbie on pharo 1.1, but it's still there
>> in 4.5
>>       >> trunk image
>>       >>
>>       >> Try to decompile this (no matter in which class)
>>       >>
>>       >> < aNumberWithUnits
>>       >>     (self compareUnits: aNumberWithUnits)
>>       >>         ifTrue: [self value: ((aNumberWithUnits value) < (self
>> value)
>>       >> ifTrue: [^true] ifFalse: [^false]).]
>>       >>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>>       >>
>>       >> Thanks to the newbie!
>>       >> I would just have hated that!
>>       >> Learning from one's own error is great, but from a dysfunctional
>> system not
>>       >> so :(
>>       >
>>       > Good bug catch! Here is a reduced version of the method that
>> produces the
>>       > same error when decompiled:
>>       >
>>       >
>>       >   Foo>>decompilerBug
>>       >       self value: (true
>>       >           ifTrue: [^ true]
>>       >           ifFalse: [^ false])
>>
>> I'm guessing that this is because
>>
>> true
>>   ifTrue: [^ true]
>>   ifFalse: [^ false]
>>
>> decompiles as
>>
>> true ifTrue: [^ true]
>> ^ false
>>
>> ?
>>
>> I'm not terribly worried about claims of a "dysfunctional system".
>> This bug is so severe that noone has noticed it before! I agree that
>> we need to fix the bug, but the original code makes no sense in the
>> first place. #value: is unreachable.
>>
>> frank
>>
>> > Dave
>> >
>> >
>>
>>
>>
>
>
>

> On 20 June 2013 15:09, Levente Uzonyi <[hidden email]> wrote:
>> On Thu, 20 Jun 2013, Nicolas Cellier wrote:
>>
>>> The bug is that (true ifTrue:[^true] ifFalse:[^false]) won't push any
>>> value on the stack (same as emitEffect: rather than emitValue:)
>>> The decompiler wants to find the argument of self value: ... on the stack
>>> and it's not there.
>>>
>>> So either we should forbid such construct at compile time
>>> (emit an 'un-reachable code' error when emitValue: does not emit any
>>> value)
>>
>>
>> I think this is the right thing to do, but the problem is a bit more
>> complex.
>>
>> The following doesn't break the decompier, but it won't be decompiled
>> correctly, because the decompiler avoids creating branches:
>>
>> decompilerBug2
>>
>>         true ifTrue: [ ^1 ] ifFalse: [ ^1 ].
>>         ^2
>
> This is what I was talking about. The Compiler inlines
> #ifTrue:ifFalse: and produces bytecodes that, when decompiled, don't
> match the original source.

>
> frank
>
>> The decompiler will produce this:
>>
>> decompilerBug2
>>         true
>>                 ifTrue: [^ 1].
>>         ^ 1.
>>         ^ 2
>>
>> Which can't be compiled anymore.
>>
>>
>> The original bug also appears with #caseOf:otherwise: :
>>
>> decompilerBug3
>>
>>         self foo: (
>>                 true
>>                         caseOf: { [ false ] -> [ ^1 ] }
>>                         otherwise: [ ^1 ])
>>
>> #caseOf: :
>>
>> decompilerBug4
>>
>>         self foo: (true caseOf: { [ false ] -> [  ^1 ] })
>>
>> and #repeat :
>>
>> decompilerBug5
>>
>>         self foo: [ ^self ] repeat
>>
>>
>> Levente
>>
>>
>>>
>>> Or patch Decompiler like mad to work around...
>>> (but Decompiler's methods are yet well below usual quality in term of
>>> complexity - long methods, many instance variables, incredibly complex
>>> state, ...)
>>>
>>> I agree, this is going to be a rare problem only for newbies, because an
>>> experimented Smalltalker will avoid writing unreachable code.
>>> So, the third solution is DO NOTHING, or more exactly add a test and an
>>> expected failure.
>>> Like that we'll say next time, hey bad luck, this is a known limitation.
>>>
>>> Nicolas
>>>
>>>
>>>
>>> 2013/6/20 Frank Shearar <[hidden email]>
>>>       On 20 June 2013 02:12, David T. Lewis <[hidden email]> wrote:
>>>      > On Thu, Jun 20, 2013 at 01:26:14AM +0200, Nicolas Cellier wrote:
>>>      >> See
>>>      >>
>>> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>>>      >>
>>>      >> This bug was found by a newbie on pharo 1.1, but it's still there
>>> in 4.5
>>>      >> trunk image
>>>      >>
>>>      >> Try to decompile this (no matter in which class)
>>>      >>
>>>      >> < aNumberWithUnits
>>>      >>     (self compareUnits: aNumberWithUnits)
>>>      >>         ifTrue: [self value: ((aNumberWithUnits value) < (self
>>> value)
>>>      >> ifTrue: [^true] ifFalse: [^false]).]
>>>      >>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>>>      >>
>>>      >> Thanks to the newbie!
>>>      >> I would just have hated that!
>>>      >> Learning from one's own error is great, but from a dysfunctional
>>> system not
>>>      >> so :(
>>>      >
>>>      > Good bug catch! Here is a reduced version of the method that
>>> produces the
>>>      > same error when decompiled:
>>>      >
>>>      >
>>>      >   Foo>>decompilerBug
>>>      >       self value: (true
>>>      >           ifTrue: [^ true]
>>>      >           ifFalse: [^ false])
>>>
>>> I'm guessing that this is because
>>>
>>> true
>>>   ifTrue: [^ true]
>>>   ifFalse: [^ false]
>>>
>>> decompiles as
>>>
>>> true ifTrue: [^ true]
>>> ^ false
>>>
>>> ?
>>>
>>> I'm not terribly worried about claims of a "dysfunctional system".
>>> This bug is so severe that noone has noticed it before! I agree that
>>> we need to fix the bug, but the original code makes no sense in the
>>> first place. #value: is unreachable.
>>>
>>> frank
>>>
>>>> Dave
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>

> See
> http://stackoverflow.com/questions/17129583/smalltalk-collection-is-empty-error-when-saving/17202938#17202938
>
> This bug was found by a newbie on pharo 1.1, but it's still there in 4.5
> trunk image
>
> Try to decompile this (no matter in which class)
>
> < aNumberWithUnits
>     (self compareUnits: aNumberWithUnits)
>         ifTrue: [self value: ((aNumberWithUnits value) < (self value)
> ifTrue: [^true] ifFalse: [^false]).]
>         ifFalse: [^Error new signal: 'Incompatible unit types.'].
>
> Thanks to the newbie!
> I would just have hated that!
> Learning from one's own error is great, but from a dysfunctional system
> not
> so :(
>
>