Re: [BUG] [ :a | ] value: 1

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [BUG] [ :a | ] value: 1

Eliot Miranda-2
Hi Lukas,

    can you forward my response to Pharo?  TIA.

On Wed, Dec 16, 2009 at 2:54 AM, Lukas Renggli <[hidden email]> wrote:
Hi Eliot

While working on a way to use the RB-AST with the standard compiler I
think I found a problem in your code. To reproduce evaluate the
following expression:

   [ :a | ] value: 1

According to my understanding that should return nil, not the first argument 1.

The generated bytecode is:

   pushTemp: 0
   blockReturn

I think it should be:

   pushConstant nil
   blockReturn

Cheers,
Lukas

:) :)  This is funny.  VisualWorks has exactly the same bug^H^H^Hfeature.  And if you look at the ANSI standard (ok, this is from the draft, but it is essentially the actual document) there is an exception for precisely this case,  See the last sentence in section 3.4.4 Blocks:

If a block has no <block body> or no <statements> in its <block body> then the value of the block is undefined. 

Which allows [] to be implemented as e.g. [Random new next].  In fact both VisualWorks and Squeak answer the last argument:
[:a ] value: 1 1
[:a :b|] value: 1 value: 2 2
[:a :b :c :d :e :f :g :h| ] valueWithArguments: (1 to: 8) 8
[:a | nil] value: 1 nil
[:a :b| nil] value: 1 value: 2 nil

At least it is consistent whether the block is inlined or not ;)

1+2 ifNotNil: [:arg|] 3

It as ever been so.  This from Smalltalk-80 V2:
BlockNode methods for initialize-release
arguments: argNodes statements: statementsCollection returns: returnBool from: encoder
sourceEnd: sourceEnd 
"compile"
sourceRange  _ sourceEnd to: sourceEnd.
arguments _ argNodes.
statements _ statementsCollection size > 0
ifTrue: [statementsCollection]
ifFalse: [argNodes size > 0
ifTrue: [statementsCollection copyWith: arguments last]
ifFalse: [Array with: NodeNil]].
returns _ returnBool

This from VW:
BlockNode methods for code generation
emitBody: codeStream
"The codeStream invokes this to compile
the body of the block, including the final return."

(body isEmpty and: [arguments isEmpty not])
ifTrue:
[codeStream noteSourceStart: body.
arguments last variable emitLocalReturn: codeStream from: body]
ifFalse: [body emitLocalReturn: codeStream from: body]

This from Squeak (my timestamp, but I just changed the _'s to :='s :) )

BlockNode methods for initialize-release
arguments: argNodes statements: statementsCollection returns: returnBool from: encoder
"Compile."

arguments := argNodes.
statements := statementsCollection size > 0
ifTrue: [statementsCollection]
ifFalse: [argNodes size > 0
ifTrue: [statementsCollection copyWith: arguments last]
ifFalse: [Array with: NodeNil]].
optimized := false.
returns := returnBool

So at least it is easy to fix ;)  The question you have to ask yourself is how much code is affected by this wafer thin change. I'm with you, I think this is a bug.  But perhaps it was well thought through.  What say you, Dan & Peter?  Did y'all agonise over this?  What say you Squeak & Pharo communities?  Should we fix this or make it a preference ;)

best
Eliot


P.S.  I'm caught between wanting to fix it and wanting to let sleeping dogs lie.  I'd like to know what all the other dialects do.  If a substantial majority have the same curio I think we have to live with this, uh, ornament.  Just to be clear, making it a preference is a sick joke on my part.


--
Lukas Renggli
http://www.lukas-renggli.ch



Reply | Threaded
Open this post in threaded view
|

Re: [BUG] [ :a | ] value: 1

Lukas Renggli
> :) :)  This is funny.  VisualWorks has exactly the same bug^H^H^Hfeature.
>  And if you look at the ANSI standard (ok, this is from the draft, but it is
> essentially the actual document) there is an exception for precisely this
> case,  See the last sentence in section 3.4.4 Blocks:
>
> If a block has no <block body> or no <statements> in its <block body> then
> the value of the block is undefined.

Ok, I did not check that document. I somehow expected that all empty
blocks would return nil.

> Which allows [] to be implemented as e.g. [Random new next].  In fact both
> VisualWorks and Squeak answer the last argument:
> [:a ] value: 1 1
> [:a :b|] value: 1 value: 2 2
> [:a :b :c :d :e :f :g :h| ] valueWithArguments: (1 to: 8) 8
> [:a | nil] value: 1 nil
> [:a :b| nil] value: 1 value: 2 nil
> At least it is consistent whether the block is inlined or not ;)
> 1+2 ifNotNil: [:arg|] 3
> It as ever been so.

Thank you for the additional insight. I learned something new on the
Smalltalk semantics today :-)

I will fix my code then.

> P.S.  I'm caught between wanting to fix it and wanting to let sleeping dogs
> lie.  I'd like to know what all the other dialects do.  If a substantial
> majority have the same curio I think we have to live with this, uh,
> ornament.  Just to be clear, making it a preference is a sick joke on my
> part.

I think we'd better let it sleep. In my image I have 61 empty blocks
with one or more argument. Some of them might depend on that hidden
behavior.

Lukas

--
Lukas Renggli
http://www.lukas-renggli.ch

Reply | Threaded
Open this post in threaded view
|

Re: [BUG] [ :a | ] value: 1

L Peter Deutsch
In reply to this post by Eliot Miranda-2
Good Heavens.  To think that that something like this would surface after
all these years....

I agree with Lukas.  The block should return nil.  The ANSI standard is
broken -- the fewer special cases a standard has, the better.

I didn't write the Smalltalk source-to-bytecode compiler, and if I had, I
would have made it return nil if I had had my wits about me.

OTOH, Dan may know of a historical reason why things were done this way --
perhaps there's some fairly common usage pattern that I've forgotten that
makes the return-the-last-argument definition worthwhile.

I agree that changing this is likely to break code.  All the same, I am
somewhat in favor of changing it in the ANSI standard.  If that is done, I
would hope someone would create a tool to do a pass over Smalltalk source
code and browse any occurrences of the problematic construct.  (Fixing them
automatically might not be a good idea, since the value that the block
returns might not actually be used, and if not, no fix would be needed.)
Fortunately, this is an example where it's easy, if a little unsightly, to
write code so that it will work under either interpretation (by explicitly
include the desired return value in the block).

                                                L Peter Deutsch

P.S. Writing standards is really very tricky.  If one writes a standard too
early, it's likely to get something important wrong simply from not enough
experience; but if you write it too late, it may be impossible to get
implementations to follow it, or the standard may have to compromise on
quality to conform to the implementations.  A depressing example of this is
HTML5, where I'm told the standards committee pretty much caved to whatever
the major browsers had actually done, regardless of how poorly designed it
was.
----------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: [BUG] [ :a | ] value: 1

Eliot Miranda-2
In reply to this post by Lukas Renggli


On Wed, Dec 16, 2009 at 9:44 AM, Lukas Renggli <[hidden email]> wrote:
> :) :)  This is funny.  VisualWorks has exactly the same bug^H^H^Hfeature.
>  And if you look at the ANSI standard (ok, this is from the draft, but it is
> essentially the actual document) there is an exception for precisely this
> case,  See the last sentence in section 3.4.4 Blocks:
>
> If a block has no <block body> or no <statements> in its <block body> then
> the value of the block is undefined.

Ok, I did not check that document. I somehow expected that all empty
blocks would return nil.

As do we all, which is why this is so horribly broken :)  The sad thing is, we've been living with it for thirty years.
 

> Which allows [] to be implemented as e.g. [Random new next].  In fact both
> VisualWorks and Squeak answer the last argument:
> [:a ] value: 1 1
> [:a :b|] value: 1 value: 2 2
> [:a :b :c :d :e :f :g :h| ] valueWithArguments: (1 to: 8) 8
> [:a | nil] value: 1 nil
> [:a :b| nil] value: 1 value: 2 nil
> At least it is consistent whether the block is inlined or not ;)
> 1+2 ifNotNil: [:arg|] 3
> It as ever been so.

Thank you for the additional insight. I learned something new on the
Smalltalk semantics today :-)

I will fix my code then.

> P.S.  I'm caught between wanting to fix it and wanting to let sleeping dogs
> lie.  I'd like to know what all the other dialects do.  If a substantial
> majority have the same curio I think we have to live with this, uh,
> ornament.  Just to be clear, making it a preference is a sick joke on my
> part.

I think we'd better let it sleep. In my image I have 61 empty blocks
with one or more argument. Some of them might depend on that hidden
behavior.


I like Peter's response.  How about making it a preference? ;) ;)  What I mean is we replace the code by something that compiles code that either raises an error or answers nil.  y default the preference is set to generate the error code.  Then we force people to write [:arg| nil] because [:arg| ] will generate a run-time error (with a verbose and helpful explanatory comment).  Then in a few months, years, decades we'll be in a position to remove the preference and generate code that answers nil because all the uses will have been caught.

I can imagine wanting to keep the preference and turning it on when importing code from other dialects for error checking.  

The downside to this approach is that empty blocks with arguments will include the "nil" noise.  But that;s very easy to remove automatically once the putsch is stable.
 
Lukas

--



Reply | Threaded
Open this post in threaded view
|

Re: Re: [BUG] [ :a | ] value: 1

Stephen Pair
On Wed, Dec 16, 2009 at 1:24 PM, Eliot Miranda <[hidden email]> wrote:

I like Peter's response.  How about making it a preference? ;) ;)  What I mean is we replace the code by something that compiles code that either raises an error or answers nil.  y default the preference is set to generate the error code.  Then we force people to write [:arg| nil] because [:arg| ] will generate a run-time error (with a verbose and helpful explanatory comment).  Then in a few months, years, decades we'll be in a position to remove the preference and generate code that answers nil because all the uses will have been caught.

I can imagine wanting to keep the preference and turning it on when importing code from other dialects for error checking.  

The downside to this approach is that empty blocks with arguments will include the "nil" noise.  But that;s very easy to remove automatically once the putsch is stable.

Egads!  I thought it was a really good joke at first, but now I realize you are half serious.

Make it work right, fix all the code in the base image, maybe write something that can reveal places where this is done in a particular class or package, then publish it while reminding people of: 

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.

- Stephen