fix Issue 14246 - RAII - proper destruction of partially constructed …

changelog/reboot14246.dd

@@ -0,0 +1,13 @@
+fix Issue 14246 - RAII - proper destruction of partically constructed objects
+
+Since destructors for object fields can now get called by the constructor for that
+object if the constructor fails, the attributes of the destructors must be covariant
+with the attributes for the constructor.
+
+I.e. if the constructor is marked @safe, the fields' destructors must also be @safe
+or @trusted.
+
+Since this behavior was never checked before, this fix causes breakage of existing
+code. Hence, enabling this behavior is behind the new `transition=dtorfields` compiler
+switch. It will eventually become the default behavior, so it is recommended to
+fix any of these destructor attribute issues.

src/dmd/aggregate.d

@@ -115,6 +115,7 @@ extern (C++) abstract class AggregateDeclaration : ScopeDsymbol
     DtorDeclaration dtor;   // aggregate destructor
     DtorDeclaration primaryDtor; // non-deleting C++ destructor, same as dtor for D
     DtorDeclaration tidtor; // aggregate destructor used in TypeInfo (must have extern(D) ABI)
+    FuncDeclaration fieldDtor;   // aggregate destructor for just the fields
 
     Expression getRTInfo;   // pointer to GC info generated by object.RTInfo(this)
 

src/dmd/aggregate.h

@@ -127,6 +127,7 @@ class AggregateDeclaration : public ScopeDsymbol
     DtorDeclaration *dtor;      // aggregate destructor
     DtorDeclaration *primaryDtor; // non-deleting C++ destructor, same as dtor for D
     DtorDeclaration *tidtor;    // aggregate destructor used in TypeInfo (must have extern(D) ABI)
+    FuncDeclaration *fieldDtor;   // aggregate destructor for just the fields
 
     Expression *getRTInfo;      // pointer to GC info generated by object.RTInfo(this)
 

src/dmd/cli.d

@@ -605,6 +605,8 @@ dmd -cov -unittest myprog.d
             "list all non-mutable fields which occupy an object instance"),
         Transition("10378", "import", "bug10378",
             "revert to single phase name lookup"),
+        Transition("14246", "dtorfields", "dtorFields",
+            "destruct fields of partially constructed objects"),
         Transition(null, "checkimports", "check10378",
             "give deprecation messages about 10378 anomalies"),
         Transition("14488", "complex", "vcomplex",

src/dmd/clone.d

@@ -963,6 +963,7 @@ extern (C++) DtorDeclaration buildDtor(AggregateDeclaration ad, Scope* sc)
             ad.dtors.shift(dd);
             ad.members.push(dd);
             dd.dsymbolSemantic(sc);
+            ad.fieldDtor = dd;
         }
     }
 

src/dmd/globals.d

@@ -151,6 +151,8 @@ struct Param
                             // https://issues.dlang.org/show_bug.cgi?id=16997
     bool vsafe;             // use enhanced @safe checking
     bool ehnogc;            // use @nogc exception handling
+    bool dtorFields;        // destruct fields of partially constructed objects
+                            // https://issues.dlang.org/show_bug.cgi?id=14246
     /** The --transition=safe switch should only be used to show code with
      * silent semantics changes related to @safe improvements.  It should not be
      * used to hide a feature that will have to go through deprecate-then-error

src/dmd/globals.h

@@ -124,6 +124,8 @@ struct Param
                         // https://issues.dlang.org/show_bug.cgi?id=16997
     bool vsafe;         // use enhanced @safe checking
     bool ehnogc;        // use @nogc exception handling
+    bool dtorFields;        // destruct fields of partially constructed objects
+                            // https://issues.dlang.org/show_bug.cgi?id=14246
     bool showGaggedErrors;  // print gagged errors anyway
     bool manual;            // open browser on compiler manual
     bool usage;             // print usage and exit

src/dmd/semantic3.d

@@ -1255,6 +1255,68 @@ private extern(C++) final class Semantic3Visitor : Visitor
         //fflush(stdout);
     }
 
+    override void visit(CtorDeclaration ctor)
+    {
+        //printf("CtorDeclaration()\n%s\n", ctor.fbody.toChars());
+        if (ctor.semanticRun >= PASS.semantic3)
+            return;
+
+        /* If any of the fields of the aggregate have a destructor, add
+         *   scope (failure) { this.fieldDtor(); }
+         * as the first statement. It is not necessary to add it after
+         * each initialization of a field, because destruction of .init constructed
+         * structs should be benign.
+         * https://issues.dlang.org/show_bug.cgi?id=14246
+         */
+        AggregateDeclaration ad = ctor.toParent2().isAggregateDeclaration();
+        if (ad && ad.fieldDtor && global.params.dtorFields)
+        {
+            /* Generate:
+             *   this.fieldDtor()
+             */
+            Expression e = new ThisExp(ctor.loc);
+            e.type = ad.type.mutableOf();
+            e = new DotVarExp(ctor.loc, e, ad.fieldDtor, false);
+            e = new CallExp(ctor.loc, e);
+            auto sexp = new ExpStatement(ctor.loc, e);
+            auto ss = new ScopeStatement(ctor.loc, sexp, ctor.loc);
+
+            version (all)
+            {
+                /* Generate:
+                 *   try { ctor.fbody; }
+                 *   catch (Exception __o)
+                 *   { this.fieldDtor(); throw __o; }
+                 * This differs from the alternate scope(failure) version in that an Exception
+                 * is caught rather than a Throwable. This enables the optimization whereby
+                 * the try-catch can be removed if ctor.fbody is nothrow. (nothrow only
+                 * applies to Exception.)
+                 */
+                Identifier id = Identifier.generateId("__o");
+                auto ts = new ThrowStatement(ctor.loc, new IdentifierExp(ctor.loc, id));
+                auto handler = new CompoundStatement(ctor.loc, ss, ts);
+
+                auto catches = new Catches();
+                auto ctch = new Catch(ctor.loc, getException(), id, handler);
+                catches.push(ctch);
+
+                ctor.fbody = new TryCatchStatement(ctor.loc, ctor.fbody, catches);
+            }
+            else
+            {
+                /* Generate:
+                 *   scope (failure) { this.fieldDtor(); }
+                 * Hopefully we can use this version someday when scope(failure) catches
+                 * Exception instead of Throwable.
+                 */
+                auto s = new OnScopeStatement(ctor.loc, TOK.onScopeFailure, ss);
+                ctor.fbody = new CompoundStatement(ctor.loc, s, ctor.fbody);
+            }
+        }
+        visit(cast(FuncDeclaration)ctor);
+    }
+
+
     override void visit(Nspace ns)
     {
         if (ns.semanticRun >= PASS.semantic3)

src/dmd/statement.d

@@ -60,6 +60,18 @@ TypeIdentifier getThrowable()
     return tid;
 }
 
+/**
+ * Returns:
+ *      TypeIdentifier corresponding to `object.Exception`
+ */
+TypeIdentifier getException()
+{
+    auto tid = new TypeIdentifier(Loc.initial, Id.empty);
+    tid.addIdent(Id.object);
+    tid.addIdent(Id.Exception);
+    return tid;
+}
+
 
 /***********************************************************
  * Specification: http://dlang.org/spec/statement.html

test/runnable/sdtor.d

@@ -1,4 +1,5 @@
 // PERMUTE_ARGS: -unittest -O -release -inline -fPIC -g
+// REQUIRED_ARGS: -transition=dtorfields
 
 import core.vararg;
 
@@ -4229,6 +4230,38 @@ int test14860()
 }
 static assert(test14860());
 
+/**********************************/
+// https://issues.dlang.org/show_bug.cgi?id=14246
+
+struct A14246 {
+     int a = 3;
+     static string s;
+     this( int var ) { printf("A()\n"); a += var; s ~= "a"; }
+
+     ~this() { printf("~A()\n"); s ~= "b"; }
+}
+
+struct B14246 {
+     int i;
+     A14246 a;
+
+     this( int var ) {
+         A14246.s ~= "c";
+         a = A14246(var+1);
+         throw new Exception("An exception");
+     }
+}
+
+void test14246() {
+    try {
+         auto b = B14246(2);
+    } catch( Exception ex ) {
+        printf("Caught ex\n");
+        A14246.s ~= "d";
+    }
+    assert(A14246.s == "cabd");
+}
+
 /**********************************/
 // 14696
 
@@ -4563,6 +4596,22 @@ void test18045() nothrow
 
 /**********************************/
 
+struct S66
+{
+    ~this() { }
+}
+
+nothrow void notthrow() { }
+
+class C66
+{
+    S66 s;
+
+    this() nothrow { notthrow(); }
+}
+
+/**********************************/
+
 int main()
 {
     test1();
@@ -4687,6 +4736,7 @@ int main()
     test14815();
     test16197();
     test14860();
+    test14246();
     test14696();
     test14838();
     test63();