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

[WalterBright]

…objects/structs

This is a reboot of #6816 , but puts it behind the compiler switch
-transition=dtorfields so it will work with incorrect existing code.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	14246	major	RAII - proper destruction of partially constructed objects/structs

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8697"

[thewilsonator]

Copying from my comment from earlier:

IIUC, the main problem was running destructors on objects that were not constructed (yes destructors should able to handle a non-constructed object, but such is reality. It will stay like that until (if at all) the compiler inserts units to do so).

If it is at all desired for this to eventually be the default behaviour then this needs to come with compiler generated unittests to verify the behaviour of destructors on non-constructed objects.

This also doesn't fix the problem of destructors that throw in the scope (failure) { this.fieldDtor(); } (yes destructors shouldn't throw, but again, such is reality and if its not tested...)

[thewilsonator]

This appears to have regressed issue 9386.

[WalterBright]

Issue 9386 is in sdtor.d, and is apparently passing.

[WalterBright]

This also doesn't fix the problem of destructors that throw in the scope (failure) { this.fieldDtor(); }

It's no different from any other case of throwing inside a scope(failure). I.e. it's not a special case.

[andralex]

So to clarify, if an field is left default-initialized, will the destructor get called for it?

[thewilsonator]

Yes. Thats what broke lots of code last time, and why it is behind a switch this time. This will therefore only break code on request to fix the already broken code :)

[thewilsonator]

For the record I don't think this is the correct way to fix 14246 (it may function as a stop gap for Weka in the interim though):
While in theory a destructor should be able to handle a default constructed object, the reality is not universally applicable. This either also needs a switch that injects a unittest to verify the above on user code or it needs to track initialisation by field.

Note that this only needs to be done for constructors that are nothrow and have fields that have destructors. A -vthrowingctor switch would be a nice way to discourage throwing in constructors (ditto for destructors).

[WalterBright]

So to clarify, if an field is left default-initialized, will the destructor get called for it?

Yes.

[WalterBright]

Thats what broke lots of code last time

According to the last thread on it, what broke the code was having a nothrow constructor call a throwing destructor, a @nogc constructor calling a non-@nogc destructor, an @safe constructor calling an @System destructor, etc.

[dnadlinger]

According to the last thread on it, what broke the code was having a nothrow constructor call a throwing destructor, a @nogc constructor calling a non-@nogc destructor, an @safe constructor calling an @System destructor, etc.

Hence my suggestion that attributes on the ctor declaration should only apply to the user-defined body, not the dtor calls behind the scenes.

[WalterBright]

Hence my suggestion that attributes on the ctor declaration should only apply to the user-defined body, not the dtor calls behind the scenes.

That makes the attributes meaningless, though. It shouldn't be hard for people to fix their destructors, and with the switch they'll have plenty of time to do it. Most of the fixes will be simply adding the attribute.

[WalterBright]

While in theory a destructor should be able to handle a default constructed object, the reality is not universally applicable.

In D, a default initialized object is a valid object. To write code otherwise will be step-by-step fighting the language.

[dnadlinger]

That makes the attributes meaningless, though.

It doesn't – they would still be checked at the call site (i.e. where the object is constructed).

[WalterBright]

It doesn't – they would still be checked at the call site.

Traits are transitive. An @safe function calling code that is @System and still passing the checker is useless.

[dnadlinger]

Traits are transitive. An @safe function calling code that is @System and still passing the checker is useless.

This is obvious and does not need repeating.

In cases problematic w.r.t. backwards-compatibility, the issue is that from the user's perspective, none of their constructor code is actually @system/…, only the implicitly invoked dtor is. Hence the suggestion to handle attributes like the following:

struct Foo {
  this() @safe @nogc pure { … }
  static __ctor(ref Foo f) { // inferred attributes
    scope (failure) f.__fieldDtor();
    f.this();
  }
  …
}

This is obviously pseudo-code – there wouldn't actually be two functions, etc. –, but should clarify what is meant.

[WalterBright]

I understand your proposal. But the result is that the constructor's behavior will not conform to its attributes, and so will engender a critical bug report. I'm afraid I'd have to agree :-(
There are only two ways forward:

1. do not fix 14246
2. expect users to fix their code
3. do what C++ does and come up with a new kind of constructor and then have conferences and seminars trying to explain how great it is that there are two kinds of constructors and which kind users should use :-)

[thewilsonator]

4. Tell users how to fix their code with informative error messages

This is a result of a chain of consequences:

• The constructor may throw an exception
• The constructor is @safe/@nogc
• The struct (sub-objects)'s have destructors
• The (sub-)object's destructor is @system/gc

So the error message should be

`@safe @nogc` constructor of `Foo` may throw, calling `@system` garbage collecting destructor `Foo.somefield.__dtor()`
Note: use -transition=14246 to infer the constructor `@system` and garbage collecting 

When using -transition=14246, issue a warning/deprecation. Or issue a warning/deprecation by default and make -transition=14246 suppress it.

`@safe @nogc` constructor of `Foo` may throw, calling `@system` garbage collecting destructor `Foo.somefield.__dtor()`
It will be inferred `@system` and garbage collecting.

Note that this will not change the safeness or nogcness of

{
    auto foo = Foo();
}

since the destructor is called. Problems will arise when dynamically allocating a Foo, though.

[Shachar]

I have a reservation regarding this feature as implemented and =void fields. Essentially, the only way to make sure that a member that is =void does not get a destructor run on uninitialized data is to initialize it at the very beginning, and make sure (somehow?) that nothing throws until it's done.

[WalterBright]

Keep in mind that =void initialization is for @system code only, for good reason. It'll be up to the user to know what they're doing, and the compiler can't help. For example:

struct S { T* p; ~this() { whatever(p); } }
...
void foo() {  S s = void; }

is also going to fail miserably.

[Shachar]

Having it @system only is fine. Requiring me to know what I'm doing is fine. Presenting me with a problem I have no way of solving is a problem.

So, assuming @system, how do I solve the problem?

[WalterBright]

Some options:

1. don't use =void; initializers for objects with destructors
2. make sure the object is initialized before encountering code that can throw
3. don't use throwing constructors

Also, consider that the only point to =void; is efficiency. But adding an exception frame around every individual field (the C++ method) has its own runtime costs. (The C++ notion of zero-cost exceptions is a complete sham - they're expensive.) Remember also that zero initialization is quite cheap.

[Shachar]

I'm not sure I follow. Let's put in a concrete example:

struct StaticArrayContainer(T) {
private:
  T[10_000_000] data;
  size_t len = 0;
public:
  this(size_t initialLength) {
    // default initialize the first initialLength elements
    this.len = initialLength;
  }
}

How do you suggest we avoid unnecessarily initializing 10 million elements of unknown size?

[RazvanN7]

What is the purpose of this code? I don't see any instances of this class anywhere. Maybe it should be moved to the compilable directory?

[WalterBright]

What is the purpose of this code?

To compile successfully.

Maybe it should be moved to the compilable directory?

Perhaps, but I wanted the struct destructor tests in one file.

[WalterBright]

@Shachar is the destructor for T, or for StaticArrayContainer?

[Shachar]

@WalterBright IT's for T.

The idea is to have a container with a fixed maximal capacity, but which otherwise behaves like an array (with variable length). Setting length to longer produces elements initialized to init. Setting it to lower would destruct the existing elements.
You know nothing about T.

[WalterBright]

@Shachar One method is to type the array elements as void, and then provide access via overloading opSlice and opIndex that cast it to type T.

[RazvanN7]

Looks good to me.

[RazvanN7]

Is the check for ad really necessary? Isn't this already checked in semantic1 ?

[WalterBright]

Sometimes, as a result of previous errors and error recovery, the AST isn't always in a completely consistent state. Expression nodes could be ErrorExp, Type nodes could be Terror, and even that isn't always done properly.