http://d.puremagic.com/issues/show_bug.cgi?id=5115

           Summary: std.typecons.scoped problems
           Product: D
           Version: D2
          Platform: x86
        OS/Version: Windows
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: bearophile_hugs@eml.cc


--- Comment #0 from bearophile_hugs@eml.cc 2010-10-24 19:10:07 PDT ---
The D type system doesn't know about the scoped nature of objects built by
std.typecons.scoped, this gives two main disadvantages over the (deprecated)
"scope" classes:
- The class destructor isn't called
- The compiler doesn't and can't enforce that references to the class don't
escape the scope.


The first problem may be seen with this test program, changing the values of the three static if:


import std.typecons: scoped;
import std.conv: emplace;
import std.c.stdio: puts;
import std.c.stdlib: exit;

class Foo {
    this() { puts("constructor"); }
    ~this() { puts("destructor"); }
}

void main() {
    {
        static if (1) {
            scope Foo f = new Foo(); // shows "destructor"
        }

        static if (0) {
            Foo f = scoped!Foo(); // doesn't show "destructor"
        }

        static if (0) {
            // doesn't show "destructor"
            ubyte[__traits(classInstanceSize, Foo)] buf = void;
            Foo f = emplace!(Foo)(cast(void[])buf);
        }
    }

    //exit(0);
}


A temporary patch to this problem is to add a template constraint to std.typecons.scoped, that refuses to instantiate the template if the given class has a destructor. This means replacing:

@system auto scoped(T, Args...)(Args args) if (is(T == class))
{


With:

@system auto scoped(T, Args...)(Args args) if (is(T == class) &&
                                               !__traits(hasMember, T,
"__dtor"))
{

---------------------------

The second problem may be seen with this program:


import std.typecons: scoped;
import std.conv: emplace;

class Foo {}

Foo bar() {
    static if (1) {
        // shows: Error: escaping reference to scope local f
        scope Foo f = new Foo();
    }

    static if (0) {
        // shows no errors
        Foo f = scoped!Foo();
    }

    static if (0) {
        // shows no errors
        ubyte[__traits(classInstanceSize, Foo)] buf = void;
        Foo f = emplace!(Foo)(cast(void[])buf);
    }

    return f;
}

void main() {}


In the first case the DMD 2.049 compiler catches the bug, while in the two other cases no errors are generated.

-------------------------------

I guess that once std.typecons.scoped works well, the std.conv.emplace may be removed from Phobos.

-------------------------------

The main purpose of "scoped" is to increase performance, allowing stack allocation of a class instance (the Java VM solves this problem becoming smarter, performing escape analysis and stack-allocating object instances that don't escape the current scope. But this is not exactly the same thing as a well implemented scope/scoped feature, because the scoped documents that the programmer wants a class to be stack-allocated, this means that the compiler has to generate an error if a reference to this class escapes. While in Java if the programmer wants a scoped class instance, then the programmer has to be careful that no references escape, otherwise the class instance will be silently heap allocated).

So this is benchmark code that may be used to compare the performance of the various implementations. To give a meaningful reference, the timings of this D benchmark must be compared to the baseline timings of the Java code below, run on a updated JavaVM using the -release switch too.



// D2 code
import std.typecons: scoped;
import std.conv: emplace;

final class Test { // 32 bytes each instance
    int i1, i2, i3, i4, i5, i6;
    this(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
        this.i1 = ii1;
        this.i2 = ii2;
        this.i3 = ii3;
        this.i4 = ii4;
        this.i5 = ii5;
        this.i6 = ii6;
    }
    void doSomething(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
    }
}

void main() {
    enum int N = 10_000_000;

    int i;
    while (i < N) {
        static if (1) { // 2.70 seconds, without testObject=null; 2.71 seconds
            Test testObject = new Test(i, i, i, i, i, i);
        }

        static if (0) { // 0.66 seconds, without testObject=null; 1.31 seconds
            scope Test testObject = new Test(i, i, i, i, i, i);
        }

        static if (0) { // 0.42 seconds, without testObject=null; 0.42 seconds
                        // 0.24 seconds if I comment out the enforces inside
emplace!()
            ubyte[__traits(classInstanceSize, Test)] buf = void;
            Test testObject = emplace!(Test)(cast(void[])buf, i, i, i, i, i,
i);
        }

        static if (0) { // 0.69 // without testObject=null; 0.69 seconds
            Test testObject = scoped!Test(i, i, i, i, i, i);
        }

        static if (0) { // (Test is a struct) 0.19 seconds
            Test testObject = Test(i, i, i, i, i, i);
        }

        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject.doSomething(i, i, i, i, i, i);
        testObject = null;
        i++;
    }
}


--------------------------

// Java code
final class Obj {
    int i1, i2, i3, i4, i5, i6;

    Obj(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
        this.i1 = ii1;
        this.i2 = ii2;
        this.i3 = ii3;
        this.i4 = ii4;
        this.i5 = ii5;
        this.i6 = ii6;
    }

    void doSomething(int ii1, int ii2, int ii3, int ii4, int ii5, int ii6) {
    }
}

final class Test {
    public static void main(String args[]) {
        final int N = 10_000_000;
        int i = 0;
        while (i < N) {
            Obj testObject = new Obj(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            testObject.doSomething(i, i, i, i, i, i);
            // testObject = null; // no difference
            i++;
        }
    }
}


--------------------------

Currently the version that uses emplace is the fastest in D if all enforce() are commented out from the code of emplace itself.

Being emplace a template (so it is not statically compiled into the Phobos lib, but compiled every time, this means that asserts are keeps inside it or removed according to the user compilation switches), and being its purpose essentially for performance, I suggest to replace all enforce() inside emplace() with normal asserts.


A possible solution to the problems of scoped is to add back some compiler support to this feature, so the compiler is more aware of the situation. (For example a @scoped type attribute, that just forbid a class reference to escape the current scope. So all objects instantiated by std.typecons.scoped automatically have this attribute.)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #1 from bearophile_hugs@eml.cc 2010-12-19 13:06:00 PST ---
An interesting example found by Andrej Mitrovic (modified):


import std.c.stdio: puts;
import std.typecons: scoped;
class Foo {
    ~this() {
        puts("Foo.dtor");
    }
}
void main() {
    Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
    auto f2 = scoped!Foo(); // calls dtor after the scope
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115


yebblies <yebblies@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yebblies@gmail.com


--- Comment #2 from yebblies <yebblies@gmail.com> 2010-12-20 21:00:29 PST ---
(In reply to comment #1)
> An interesting example found by Andrej Mitrovic (modified):
> 
> 
> import std.c.stdio: puts;
> import std.typecons: scoped;
> class Foo {
>     ~this() {
>         puts("Foo.dtor");
>     }
> }
> void main() {
>     Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
>     auto f2 = scoped!Foo(); // calls dtor after the scope
> }

This example (and the first half of this bug report) appears to be a duplicate of bug http://d.puremagic.com/issues/show_bug.cgi?id=3516

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #3 from bearophile_hugs@eml.cc 2010-12-21 00:22:23 PST ---
(In reply to comment #2)

> > void main() {
> >     Foo f1 = scoped!Foo();  // doesn't call dtor after the scope
> >     auto f2 = scoped!Foo(); // calls dtor after the scope
> > }
> 
> This example (and the first half of this bug report) appears to be a duplicate of bug http://d.puremagic.com/issues/show_bug.cgi?id=3516

But both f1 and f2 gets assigned. I don't fully understand issue 3516 so I can't tell.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115


Brad Roberts <braddr@puremagic.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |braddr@puremagic.com


--- Comment #4 from Brad Roberts <braddr@puremagic.com> 2010-12-21 00:29:38 PST ---
Any issue that involves a struct dtor not being properly run is almost certainly an extension of bug 3516.  Walter's well aware of it, but it's a non-trivial problem to fix.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115


Lars T. Kyllingstad <bugzilla@kyllingen.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla@kyllingen.net


--- Comment #5 from Lars T. Kyllingstad <bugzilla@kyllingen.net> 2010-12-21 00:48:45 PST ---
Isn't the problem rather that the type of f2 is Scoped!Foo, while the type of f1 is just Foo?  The compiler doesn't know that it should call the destructor of a Foo at the end of the scope.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #6 from bearophile_hugs@eml.cc 2010-12-21 00:52:48 PST ---
(In reply to comment #5)
> Isn't the problem rather that the type of f2 is Scoped!Foo, while the type of f1 is just Foo?  The compiler doesn't know that it should call the destructor of a Foo at the end of the scope.

You are probably right. Then why isn't this a compile error?

Foo f1 = scoped!Foo();

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #7 from Lars T. Kyllingstad <bugzilla@kyllingen.net> 2010-12-21 00:56:37 PST ---
(In reply to comment #6)
> You are probably right. Then why isn't this a compile error?
> 
> Foo f1 = scoped!Foo();

Because Scoped!Foo has an 'alias this' to the underlying Foo, which is necessary for it to behave like a Foo.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #8 from yebblies <yebblies@gmail.com> 2010-12-21 01:00:19 PST ---
(In reply to comment #7)
> (In reply to comment #6)
> > You are probably right. Then why isn't this a compile error?
> > 
> > Foo f1 = scoped!Foo();
> 
> Because Scoped!Foo has an 'alias this' to the underlying Foo, which is necessary for it to behave like a Foo.

scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like
that).
This temporary is implicitly converted to Foo using alias this, but the
temporary never has it's destructor called due to bug 3516, which means Foo's
destructor is never called either.

It is fine for the temporary to be converted to Foo, so long as the destructor is called when the scope that 'scoped' was called in is exited.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=5115



--- Comment #9 from Kenji Hara <k.hara.pg@gmail.com> 2011-11-10 23:11:38 PST ---
(In reply to comment #8)
> scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like
> that).
> This temporary is implicitly converted to Foo using alias this, but the
> temporary never has it's destructor called due to bug 3516, which means Foo's
> destructor is never called either.
> 
> It is fine for the temporary to be converted to Foo, so long as the destructor is called when the scope that 'scoped' was called in is exited.

Now bug 3516 was fixed. Then comment#1 code prints two "Foo.dtor"

But allowing conversion from temporary type (e.g. Scoped!Foo) to Foo is unsafe, because the temporary has value type and its lifetime is limited in its scope, but Foo is class reference of the temporary and we can bring it out the scope.

Foo global_foo;
void test1() {
    auto foo = scoped!Foo();
    global_foo = foo;   // implicitly conversion from typeof(foo) to Foo
      // This line should be forbidden in compile time
}
void test2() {
    // use global_foo -> Access Violation!
}
void main() {
    test1();
    test2();
}

I think my ProxyOf mixin template is useful for this issue. https://github.com/D-Programming-Language/phobos/pull/300

--- Comment #10 from Kenji Hara <k.hara.pg@gmail.com> 2011-11-10 23:11:39 PST ---
(In reply to comment #8)
> scoped!Foo() returns a temporary of type scoped!(Foo).Scoped (or something like
> that).
> This temporary is implicitly converted to Foo using alias this, but the
> temporary never has it's destructor called due to bug 3516, which means Foo's
> destructor is never called either.
> 
> It is fine for the temporary to be converted to Foo, so long as the destructor is called when the scope that 'scoped' was called in is exited.

Now bug 3516 was fixed. Then comment#1 code prints two "Foo.dtor"

But allowing conversion from temporary type (e.g. Scoped!Foo) to Foo is unsafe, because the temporary has value type and its lifetime is limited in its scope, but Foo is class reference of the temporary and we can bring it out the scope.

Foo global_foo;
void test1() {
    auto foo = scoped!Foo();
    global_foo = foo;   // implicitly conversion from typeof(foo) to Foo
      // This line should be forbidden in compile time
}
void test2() {
    // use global_foo -> Access Violation!
}
void main() {
    test1();
    test2();
}

I think my ProxyOf mixin template is useful for this issue. https://github.com/D-Programming-Language/phobos/pull/300

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------