Bug 17869 – scope class object no longer deleted when created via factory function

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

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.

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

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.

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.