Tcl Source Code

This might smell a bit similar to [d87cb1820]?

Replacing the [next] calls with [set next next; $next] removes the error.

dgp identified [4492d3b64521] as the checkin which identified this
behaviour with bytecoded [next].

    oo::class create Foo {
        method five {} {
            tailcall expr 5
        }
    }
    oo::class create Unfoo {
        superclass Foo
        method five {} {
            return -[next]
        }
    }
    Unfoo create fu
    puts "[fu five] should be -5?"
    # 8.6.2..trunk:  5 should be -5?
    # 8.6.1:         -5 should be -5?

For added hilarity:

    oo::class create Foo {
        method five {} {
            tailcall puts 2
        }
    }
    oo::class create Unfoo {
        superclass Foo
        method five {} {
            puts 1
            next
            puts 3
        }
    }
    Unfoo create fu
    fu five
    # 1
    # 3
    # 2

I've merged the changes. There may be other things wrong too, but this at least reduces the scope of wrong. We can open issues for the other things if they bother us too much.

I agree about it being a candidate to merge: it fixes at least one bug, right? 

Donal is the best person to look at the [next] compiler and at least make it do the same thing as the non-compiled version. Or does the fix already achieve that much? Also the testnrelevels interactions, but I could try to help there.

Interaction of tailcall with catch and uplevel is indeed tricky, hard to determine what we actually want to happen.

tests added to bug fix branch.

Still think it's a candidate to merge.

Starting from dkf's examples, I have tried to make a comprehensive test suite
for tcloo + tailcall interaction.  This should cover all the significant parts
visible on the script level .. if I've missed anything, feel free to add it!

These tests all pass if compilation of [next] is inhibited by uncommenting the
block near the top.  Otherwise, every test involving [next] fails in exactly the same way.  This suggests to me we're dealing with a single bug here, and
the proposed fix at [5c3a32ed23] is sound.

Things I still don't understand:

  * testnrelevels - clearly needs to be included with these tests, but I'm not
    clear how to set up good tests (ref [0527225cc]).
  * tailcall under catch, uplevel or both requires careful consideration, but
    I don't think that directly impacts this issue.  I'm investigating those
    separately.

======
# dkf's "Principles Leading to a Fix"
#
#   A method ought to work "the same" whether or not it has been overridden by
#   a subclass. A tailcalled command ought to have as parent stack the same
#   thing you'd get with uplevel 1. A subclass will often expect the
#   superclass's result to be the result that would be returned if the subclass
#   was not there. 
#
package require tcltest
namespace import -force ::tcltest::*

;# uncomment this to inhibit compilation of [next] and [nextto]
;# .. which permits all the tests to pass
;# A simpler, per-test inhibition is possible by using
;# [[string cat next]] inline.
if 0 {
    rename ::oo::Helpers::next ::oo::Helpers::_next
    proc ::oo::Helpers::next args {
        tailcall _next {*}$args
    }
    rename ::oo::Helpers::nextto ::oo::Helpers::_nextto
    proc ::oo::Helpers::nextto args {
        tailcall _nextto {*}$args
    }
}

;# common setup:
;#  any invocation of bar should emit "abc\nhi\n" then return to its caller
set testopts {
    -setup {
        oo::class create Foo {
            method bar {} {
                puts abc
                tailcall puts hi
                puts xyz
            }
        }
    }
    -cleanup {
        catch {Foo destroy}
        catch {Foo2 destroy}    ;# created by some tests
    }
}

;# these succeed, showing that without [next] the bug doesn't fire
test next-tailcall-simple-1 "trivial case with one method" {*}$testopts -body {
    [Foo create foo] bar
} -output [join {abc hi} \n]\n

test next-tailcall-simple-2 "my bar" {*}$testopts -body {
    oo::define Foo method baz {} {
        puts a
        my bar
        puts b
    }
    [Foo create foo] baz
} -output [join {a abc hi b} \n]\n

test next-tailcall-simple-3 "\[self\] bar" {*}$testopts -body {
    oo::define Foo method baz {} {
        puts a
        [self] bar
        puts b
    }
    [Foo create foo] baz
} -output [join {a abc hi b} \n]\n

test next-tailcall-simple-4 "foo bar" {*}$testopts -body {
    oo::define Foo method baz {} {
        puts a
        foo bar
        puts b
    }
    [Foo create foo] baz
} -output [join {a abc hi b} \n]\n


;# everything from here on uses [next], and fails on 8.6.4 with compilation
test next-tailcall-superclass-1 "next superclass" {*}$testopts -body {
    oo::class create Foo2 {
        superclass Foo
        method bar {} {
            puts a
            next
            puts b
        }
    }
    [Foo2 create foo] bar
} -output [join {a abc hi b} \n]\n

test next-tailcall-superclass-2 "nextto superclass" {*}$testopts -body {
    oo::class create Foo2 {
        superclass Foo
        method bar {} {
            puts a
            nextto Foo
            puts b
        }
    }
    [Foo2 create foo] bar
} -output [join {a abc hi b} \n]\n


test next-tailcall-mixin-1 "class mixin" {*}$testopts -body {
    oo::class create Foo2 {
        method Bar {} {
            puts a
            next
            puts b
        }
        filter Bar
    }
    oo::define Foo mixin Foo2
    Foo create foo
    foo bar
} -output [join {a abc hi b} \n]\n

test next-tailcall-objmixin-1 "object mixin" {*}$testopts -body {
    oo::class create Foo2 {
        method Bar {} {
            puts a
            next
            puts b
        }
        filter Bar
    }
    Foo create foo
    oo::objdefine foo mixin Foo2
    foo bar
} -output [join {a abc hi b} \n]\n

test next-tailcall-filter-1 "filter method" {*}$testopts -body {
    oo::define Foo method Filter {} {
        puts a
        next
        puts b
    }
    oo::define Foo filter Filter
    [Foo new] bar
} -output [join {a abc hi b} \n]\n

test next-tailcall-forward-1 "forward method" {*}$testopts -body {
    proc foobar {} {
        puts "abc"
        tailcall puts "hi"
        puts "xyz"
    }
    oo::define Foo forward foobar foobar
    oo::class create Foo2 {
        superclass Foo
        method foobar {} {
            puts a
            next
            puts b
        }
    }
    [Foo2 new] foobar
} -output [join {a abc hi b} \n]\n

test next-tailcall-constructor-1 "next in constructor" -body {
    oo::class create Foo {
        constructor {} {
            puts abc
            tailcall puts hi
            puts xyz
        }
    }
    oo::class create Foo2 {
        superclass Foo
        constructor {} {
            puts a
            next
            puts b
        }
    }
    list [Foo new] [Foo2 new]
    return ""
} -cleanup {
    Foo destroy
} -output [join {abc hi a abc hi b} \n]\n


test next-tailcall-destructor-1 "next in destructor" -body {
    oo::class create Foo {
        destructor {
            puts abc
            tailcall puts hi
            puts xyz
        }
    }
    oo::class create Foo2 {
        superclass Foo
        destructor {
            puts a
            next
            puts b
        }
    }
    Foo  create foo
    Foo2 create foo2
    foo destroy
    foo2 destroy
} -output [join {abc hi a abc hi b} \n]\n

======

I'm not going to recommend merging anything until we figure out what the behaviour <i>should</i> be. To do that, we need a few cases.
<h2>Simple case</h2>
<pre>
oo::class create Foo {
    method bar {} {
        tailcall puts hi
        puts xyz
    }
}
[Foo create foo] bar
</pre>
This should replace the invoke of bar on foo with a call to puts. Expected output: <pre>hi</pre>
<h2>Complex case 1</h2>
<pre>
oo::class create Foo {
    method bar {} {
        tailcall puts hi
        puts xyz
    }
}
oo::class create Foo2 {
    superclass Foo
    method bar {} {
        puts a
        next
    }
}
[Foo2 create foo] bar
</pre>
Expected output:
<pre>
a
hi
</pre>
<h2>Complex case 2</h2>
<pre>
oo::class create Foo {
    method bar {} {
        tailcall puts hi
        puts xyz
    }
}
oo::class create Foo2 {
    method Bar {} {
        puts a
        next
    }
    filter Bar
}
Foo create foo
oo::objdefine foo mixin Foo2
foo bar
</pre>
Same expected output as above.
<p>
But what if the filter wants to process the result? What if there's a [<tt>catch { next } msg opt</tt>] in there? My head aches when I try to think about these things.
<h1>Possible Principles Leading to a Fix</h1>
A method ought to work "the same" whether or not it has been overridden by a subclass. A tailcalled command ought to have as parent stack the same thing you'd get with <tt>uplevel 1</tt>. A subclass will often expect the superclass's result to be the result that would be returned if the subclass was not there.
<p>
I ought to look what happens with <tt>catch { tailcall foo }</tt>...

Correction suggested by miguel works better.
Candidate for trunk merge.

Branch bug-0f42ff7871 has patch that fixes the
behavior of the submitted scripts, but breaks
test nre-oo.4

I do not know what the correct behaviour even is, or whether <i>both</i> 8.6.1 (interpreted path) and 8.6.4 (compiled path) are wrong.