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

[WalterBright]

…objects/structs

[dlang-bot]

Fix	Bugzilla	Description
✓	14246	RAII - proper destruction of partially constructed objects/structs

[UplinkCoder]

@WalterBright doesn't this change language semantics quite a bit ?

[yebblies]

Where are the tests? And do we still have constructs that can't be used inside of try/catch blocks that will trip up here?

[WalterBright]

Where are the tests?

I forgot to commit it. Done.

And do we still have constructs that can't be used inside of try/catch blocks that will trip up here?

I don't think so. It's the whole point of scope (failure).

[WalterBright]

doesn't this change language semantics quite a bit ?

It's expected behavior, so I consider it fixing a bug.

[andralex]

Spec pull: dlang/dlang.org#1658.

Question: with the current code, is a dtor call issued for members that are not initialized by means of a ctor (i.e. those implicitly initialized with .init)?

[WalterBright]

with the current code, is a dtor call issued for members that are not initialized by means of a ctor (i.e. those implicitly initialized with .init)?

With this PR, yes. Since a .init initialized object is a valid object, running the destructor on it must be benign. This is, of course, inefficient as it is not necessary to run the destructor on them. However, it is also inefficient to otherwise generate all the different exception handling contexts, one for each constructor run.

I decided to do it this way based on the notion that throwing exceptions during construction is a rare event, and not the path that should be optimized.

Just to be clear, destructors are only run for fields that are of types with destructors.

[andralex]

@WalterBright ok, updated dlang/dlang.org#1658

[andralex]

@WalterBright please review dlang/dlang.org#1658

[braddr]

Removed auto-merge to de-prioritize the build until it passes tests.

[CyberShadow]

This pull request broke libasync and other projects that depend on this, incl. vibe.d. It's one of the reasons Jenkins is currently failing.

Reduced test case:

struct S
{
    ~this() {}
}

class C
{
    S s;

    this() nothrow {}
}

Compiler now complains:

test.d(10): Error: destructor 'test.C.~this' is not nothrow
test.d(10): Error: nothrow constructor 'test.C.this' may throw

[WalterBright]

Bug reports (including regressions) should be reported to Bugzilla. I did this one: https://issues.dlang.org/show_bug.cgi?id=17493

[wilzbach]

This pull request broke libasync and other projects that depend on this, incl. vibe.d. It's one of the reasons Jenkins is currently failing.

Just checking whether there are more hidden regressions on Jenkins or whether this is the last one:
#6899

[wilzbach]

So D-YAML and EMSI containers are affected as well from this PR.

Jenkins does report the error for EMSI containers, it just doesn't trigger an error:

CC @MartinNowak

edit: it seems that EMSI had linkage errors before and thus was disabled (|| echo failed) the the project tester.

[wilzbach]

Reduction for D-YAML

class Scanner
{
    import std.container.array : Array;
    Array!int indents_;

    this() @safe {}
}

Yields:

scanner.d(6): Error: @safe constructor 'scanner.Scanner.this' cannot call @system destructor 'scanner.Scanner.~this'

edit, without Phobos:

struct Array
{
    int[] _payload;
    ~this()
    {
        import core.stdc.stdlib : free;
        free(_payload.ptr);
    }
}

class Scanner
{
    Array arr;
    this() @safe {}
}

[wilzbach]

Reducing EMSI wasn't that successful, but maybe still helpful?

struct TreeMap(K)
{
	this() @disable;

	import ttree;
	TTree!int tree;
}

unittest
{
    TreeMap!int;
    TreeMap!string;
}

ttree.d

struct TTree(T)
{
	this() @disable;
~}

Since this PR:

ttree.d(4): Error: found '}' when expecting 'this'
ttree.d(5): Error: found 'EOF' when expecting '('
ttree.d(5): Error: found 'EOF' when expecting ')'
ttree.d(5): Error: semicolon expected following function declaration
ttree.d(5): Error: } expected following members in struct declaration at ttree.d(1)
treemap.d(12): Error: template instance treemap.TreeMap!int error instantiating
treemap.d(3): Error: field tree must be initialized in constructor
treemap.d(13): Error: template instance treemap.TreeMap!string error instantiating

With 2.074.0:

ttree.d(4): Error: found '}' when expecting 'this'
ttree.d(5): Error: found 'EOF' when expecting '('
ttree.d(5): Error: found 'EOF' when expecting ')'
ttree.d(5): Error: semicolon expected following function declaration
ttree.d(5): Error: } expected following members in struct declaration at ttree.d(1)
treemap.d(12): Error: template instance treemap.TreeMap!int error instantiating
treemap.d(13): Error: type has no effect in expression (TreeMap!string)

[WalterBright]

~}

The extra ~ looks like the problem.

[CyberShadow]

Reducing EMSI wasn't that successful, but maybe still helpful?

It looks like there is an error in your reduction test script.

[wilzbach]

It looks like there is an error in your reduction test script.

Ah, yeah - I gave it a couple of new tries and ended with this nice reduction for EMSI containers:

struct TreeMap
{
	this() @disable;
	this(TTree tree) { this.tree = tree; }
	TTree tree;
}

struct TTree
{
	this() @disable;
	this(int foo) {}
	~this() {}
}

void main()
{
    auto k = TreeMap(TTree(1));
}

treemap.d(3): Error: field tree must be initialized in constructor

It compiles fine with (2.074.0).

[wilzbach]

@WalterBright as you like bugzilla and these seem to be individual issues, I opened two more issues, s.t. it can be tracked better:

D-YAML: Issue 17505 - [REG2.075] @safe constructor requires the deconstructor to be safe as well
EMSI: Issue 17506 - [REG2.075] @disable constructor requires members to be initialized

[MartinNowak]

This is also responsible for the regression in libasync where it fails with a nothrow constructor in a final class (that doesn't seem to have a dtor). Please advice how to proceed @WalterBright.
https://github.com/etcimon/libasync/blob/a56cf33720731de717ef5d6c5f1c0d51341da145/source/libasync/events.d#L57

[MartinNowak]

It's expected behavior, so I consider it fixing a bug.

Most popular excuse for breaking lots of production code 😄.

[WalterBright]

as you like bugzilla

It's not about me liking bugzilla. Bugzilla is the procedure we use for tracking bugs.

and these seem to be individual issues, I opened two more issues, s.t. it can be tracked better:

Thank you.

[WalterBright]

Please advice how to proceed

Since there are some consequences to this PR with no obvious remedy, I suggest reverting it for the moment.

[MartinNowak]

Since there are some consequences to this PR with no obvious remedy, I suggest reverting it for the moment.

Came to the same conclusion, see #6913.