D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 17869 - scope class object no longer deleted when created via factory function
Summary: scope class object no longer deleted when created via factory function
Status: RESOLVED WONTFIX
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Windows
: P1 regression
Assignee: No Owner
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-10-01 15:00 UTC by Rainer Schuetze
Modified: 2017-10-09 22:08 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Rainer Schuetze 2017-10-01 15:00:58 UTC
import core.stdc.stdio;

void foo()
{
    scope baseKey = Key.getKey();
}

int main(string[] argv)
{
    printf("main init\n");
    foo();
    printf("main exit\n");
    return 0;
}

class Key
{
private:
    this() { printf("ctor\n"); }
    ~this() { printf("dtor\n"); }

    static Key getKey() { return new Key(); }
}

////////////////////////
This prints

main init
ctor
dtor
main exit

with dmd 2.074 and

main init
ctor
main exit
dtor

with dmd 2.075+, i.e. the class instance assigned to the scope variable is destroyed in the final collection, not in foo's scope. The 2.074 version is according to spec IMO: "... the destructor for an object is automatically called when the reference to it goes out of scope."

Probably introduced by https://github.com/dlang/dmd/pull/6826
Comment 1 Mathias Lang 2017-10-05 16:17:04 UTC
This should have never worked to begin with!
Slightly modifying your example to expose the bug:

```
import core.stdc.stdio;

void foo()
{
    scope baseKey = Key.getKey();
}

int main(string[] argv)
{
    printf("main init\n");
    foo();
    printf("main exit\n");
    assert(Key.global !is null);
    return 0;
}

class Key
{
private:
    this() { printf("ctor\n"); }
    ~this() { printf("dtor\n"); }

    static Key global;
    static Key getKey() { return (global = new Key()); }
}
```

As you can see, in here the compiler has no way to know it *should* destroy `baseKey` at the end of the `foo` function. And `scope` in this case is misleading, as the `scope` reference is GC-allocated anyway.
Comment 2 Rainer Schuetze 2017-10-05 17:50:02 UTC
scope class references are not supposed to be safe so far. The current spec explicitly says the destructor is called: "This means that the destructor for an object is automatically called when the reference to it goes out of scope. The destructor is called even if the scope is exited via a thrown exception, thus scope is used to guarantee cleanup."

That might not be consistent with scope as it is used by DIP1000, but changing its meaning should at least have some notification in the changelog. IMO -dip1000 should prohibit unsafe usage, though.

See also discussion in https://github.com/ldc-developers/ldc/pull/2252#issuecomment-333360596
Comment 3 Martin Nowak 2017-10-09 11:03:02 UTC
It's was destroyed twice formerly, destruction was never supposed to run for GC allocated classes.
Scope class always referred to RAII allocated classes on the stack.
The fact that you can assign heap allocated instances to variables with scope storage was unfortunately a bug, as was the double destruction of scope class instances assigned to other scope references.

Scope variables not pointing to stack allocated RAII classes have been repurposed to match the semantics of scope pointers.

Sorry for any unforseen troubles this may have caused, but the existing behavior seemed too buggy to preserve.
Comment 4 Rainer Schuetze 2017-10-09 22:08:56 UTC
I'm not convinced:

> It's was destroyed twice formerly, destruction was never supposed to run for GC allocated classes.

Sure it has been unsafe so far, just like writing "scope(exit) delete obj;" twice.

> Scope class always referred to RAII allocated classes on the stack.

Yes, but the spec does not say this explicitly about scope declarations.

> The fact that you can assign heap allocated instances to variables with scope
> storage was unfortunately a bug, as was the double destruction of scope class 
> instances assigned to other scope references.

So why is it now silently accepted? 

BTW: Even std.datetime.timezone still uses scope with factory functions, relying on GC caused bugs in LDC due to premature collection.

I'm not against changing the meaning of scope for the sake of DIP1000, but I wonder why it's ok in this case to break code without any warning that worked fine before.