• "H. S. Teoh" <hsteoh quickfur.ath.cx> Aug 24 2013
• "Ramon" <spam thanks.no> Aug 24 2013
• "Tobias Pankrath" <tobias pankrath.net> Aug 24 2013
• Dmitry Olshansky <dmitry.olsh gmail.com> Aug 27 2013
• Dmitry Olshansky <dmitry.olsh gmail.com> Aug 27 2013
• Dmitry Olshansky <dmitry.olsh gmail.com> Aug 27 2013
• Walter Bright <newshound2 digitalmars.com> Aug 24 2013
• Walter Bright <newshound2 digitalmars.com> Aug 24 2013
• Walter Bright <newshound2 digitalmars.com> Aug 25 2013
• "H. S. Teoh" <hsteoh quickfur.ath.cx> Aug 25 2013

"H. S. Teoh" <hsteoh quickfur.ath.cx> writes:

On Sat, Aug 24, 2013 at 01:01:12PM +0200, Artur Skawina wrote:
 On 08/24/13 08:30, Piotr Szturmaj wrote:
 H. S. Teoh wrote:
 I've written up a proposal to solve the partially-constructed
 object problem[*] in D in a very nice way by extending scope
 guards:

     http://wiki.dlang.org/DIP44




 2. allow referring to local variables and create the closure. This
 causes additional memory allocation and also reference to the
 closure must be stored somewhere, perhaps in the class hidden field.
 Of course, this is a "no go", I'm writing this here for comparison.


 That is what he's actually proposing. And, yes, it's not a good idea.
 Implementing it via runtime delegates, implicit captures/allocs and
 extra hidden fields inside aggregates would work, but the cost is too
 high. It's also not much of an improvement over manually registering
 and calling the delegates. Defining what happens when a ctor fails
 would be a good idea, having a cleanup method which defaults to
 `~this`, but can be overridden could help too.


The issue with that is that the initialization code and the cleanup code
has to be separated, potentially by a lot of unrelated stuff in between.
The genius of scope guards is that initialization and cleanup is written
in one place even though they actually happen in different places, so
it's very unlikely you will forget to cleanup correctly.


 There are other problems with that DIP, like making it harder to see
 what's actually going on, by splitting the dtor code and having it
 interleaved with another separate flow.


I think it's unhelpful to conflate scope(this) with dtors. Yes there is
some overlap, but if you treat them separately, then there is no
problem (assuming that a dtor is actually necessary).


 It *is* possible to implement a similar solution without any RT cost,
 but it would need:
 a) flow analysis - to figure out the cleanup order, which might not be 
     statically known (these cases have to be disallowed)
 b) a different approach for specifying the cleanup code, so that 
     implicit capturing of ctor state doesn't happen and it's not
     necessary to read the complete body of every ctor just to find
     out what a dtor does.


I think that's an unhelpful way of thinking about it. What about we
think of it this way: the ctor is acquiring X number of resources, and
by wrapping the resource-releasing code in scope(this), we guarantee
that these resources will be correctly released. Basically, scope(this)
will take care of invoking the release code whenever the object's
lifetime is over, whether it's unsuccessful construction, or
destruction.

It's just like saying scope guards are useless because it's equivalent
to a try-catch block anyway (and in fact, that's how the front end
implements scope guards). One may even argue scope guards are bad
because the cleanup code is sprinkled everywhere rather than collected
in one place. But it's not really about whether it's equivalent to
another language construct; it's about better code maintainability.
Separating the code that initializes something from the code that cleans
up something makes it harder to maintain, and more error-prone (e.g.
initialize 9 things, forget to clean up one of them). Keeping them
together in the same place makes code correctness clearer.


On Sat, Aug 24, 2013 at 02:48:53PM +0200, Tobias Pankrath wrote:
[...]
 Couldn't this problem be solved by using RAII and destructors? You
 would (only) need to make sure that every member is either correctly
 initialised or T.init.


How would you use RAII to solve this problem? If I have a class:

	class C {
		Resource1 res1;
		Resource2 res2;
		Resource3 res3;
		this() {
			...
		}
	}

How would you write the ctor with RAII such that if it successfully
inits res1 and res2, but throws before it inits res3, then only res1 and
res2 will be cleaned up?


On Sat, Aug 24, 2013 at 09:31:52PM +0400, Dmitry Olshansky wrote:
[...]
 Instead of introducing extra mess into an already tricky ctor/dtor
 situation. (Just peek at past issues with dtors not being called,
 being called at wrong time, etc.)
 
 I'd say just go RAII in bits and pieces. Unlike scope, there it
 works just fine as it has the right kind of lifetime from the get
 go. In function scope (where scope(exit/success/failure) shines)
 RAII actually sucks as it may prolong the object lifetime I you are
 not careful to tightly wrap it into { }.
 
 Start with this:
 
 class C {
     Resource1 res1;
     Resource2 res2;
     Resource3 res3;
 
     this() {
         res1 = acquireResource!1();
         res2 = acquireResource!2();
         res3 = acquireResource!3();
     }
 
     ~this() {
         res3.release();
         res2.release();
         res1.release();
     }
 }
 
 Write a helper once:
 
 struct Handler(alias acquire, alias release)
 {
 	alias Resource = typeof(acquire());
 	Resource resource;
 	this(int dummy) //OMG when 0-argument ctor becomes usable?
 	{
 		resource = acquire();
 	}
 
 	static auto acquire()
 	{
 		return Handler(0); //ditto
 	}
 
 	~this()
 	{
 		release(resource);
 	}
 	alias this resource;
 }
 
 
 Then:
 
 class C{
     Handler!(acquireResource!1, (r){ r.release(); }) res1;
     Handler!(acquireResource!2, (r){ r.release(); }) res2;
     Handler!(acquireResource!3, (r){ r.release(); }) res3;
     this(){
 	res1 = typeof(res1).acquire();
 	res2 = typeof(res2).acquire();
 	res3 = typeof(res3).acquire();
     }
 }


I don't see how code solves the problem. Suppose this() throws an
Exception after res1 and res2 have been initialized, but before res3 is
uninitialized. Now what? How would the language know to only clean up
res1 and res2, but not res3? How would the language know to only invoke
the dtors of res1 and res2, but not res3?


On Sat, Aug 24, 2013 at 12:27:37PM -0700, Walter Bright wrote:
[...]
 Not a bad idea, but it has some issues:
 
 1. scope(failure) takes care of most of it already
 
 2. I'm not sure this is a problem that needs solving, as the DIP
 points out, these issues are already easily dealt with. We should be
 conservative about adding more syntax.
 
 3. What if the destructor needs to do more than just unwind the
 transactions? Where does that code fit in?


I think it's unhelpful to conflate scope(this) with dtors. They are
related, but -- and I guess I was a bit too strong about saying dtors
are redundant -- if we allow both, then scope(this) can be reserved for
transactions, and you can still put code in ~this() to do non-trivial
cleanups.


 4. The worst issue is the DIP assumes there is only one constructor,
 from which the destructor is inferred. What if there is more than
 one constructor?


This is not a problem. If there is more than one constructor, then only
those scope(this) statements in the ctor that were actually encountered
will trigger when the object reaches the end of its lifetime.  You
already have to do this anyway, since if the ctor throws an Exception
before completely constructing the object, only those scope(this)
statements that have been encountered up to that point will be
triggered, not all of them. Otherwise, you'd still have the
partially-initialized object problem.


T

-- 
Without geometry, life would be pointless. -- VS

"Ramon" <spam thanks.no> writes:

Maybe I'm just plain too unexperienced or dumb but I see problems.

- What's a dtor's job in D? This is not C++ (where cleaning up 
often means freeing/not leaking), in D the GC saves us lots of 
problems. Actually, we do not even really free anything (except 
malloc'ed stuff) but rather hinting the runtime expressis verbis 
that sth. isn't needed anymore.

- scope(this) adds something new to the scope mechanism wich is 
per se bad and imo not justified for luxury.

- I see H.S. Teoh's point to in a way have stuff aquired or 
allocated in ctor kind of "registered" in dtor for proper 
cleanup, but that *can* be done with what we have right now.

- From what I understand the proposal implies some kind of a 
transactional "all or nothing" mechanism. Is a ctor and class 
level the right place to implement that?


 H.S. Teoh

Maybe it would helpful to explain why you think this is an 
*important* problem (rather than a nuisance) and more clearly 
define it (and why it can't be solved using what we have)?

Thanks

"Tobias Pankrath" <tobias pankrath.net> writes:

On Saturday, 24 August 2013 at 20:11:14 UTC, H. S. Teoh wrote:
 How would you use RAII to solve this problem? If I have a class:

 	class C {
 		Resource1 res1;
 		Resource2 res2;
 		Resource3 res3;
 		this() {
 			...
 		}
 	}

 How would you write the ctor with RAII such that if it 
 successfully
 inits res1 and res2, but throws before it inits res3, then only 
 res1 and
 res2 will be cleaned up?


Like someone else already proposed: Using a wrapper type W that 
releases the resources in it's destructor. W.init wouldn't 
release anything. So by definition (if I recall the rules 
correctly) every new instance of C would have res3 = 
Resource3.init prior to it's constructor called.

Now make sure that a) res3's destructor gets called (check) b) 
res3's destructor may be called on Resource3.init. That would be 
a new rule similar to 'no internal aliasing' and c) that every 
constructor of C either sets res3 to a destructable value or does 
not touch it at all ('transactional programming').

Dmitry Olshansky <dmitry.olsh gmail.com> writes:

27-Aug-2013 01:30, H. S. Teoh пишет:
 On Sun, Aug 25, 2013 at 12:18:27AM +0200, Tobias Pankrath wrote:
 On Saturday, 24 August 2013 at 20:11:14 UTC, H. S. Teoh wrote:
 How would you use RAII to solve this problem? If I have a class:

 	class C {
 		Resource1 res1;
 		Resource2 res2;
 		Resource3 res3;
 		this() {
 			...
 		}
 	}

 How would you write the ctor with RAII such that if it successfully
 inits res1 and res2, but throws before it inits res3, then only res1
 and res2 will be cleaned up?


 Like someone else already proposed: Using a wrapper type W that
 releases the resources in it's destructor. W.init wouldn't release
 anything. So by definition (if I recall the rules correctly) every
 new instance of C would have res3 = Resource3.init prior to it's
 constructor called.

 Now make sure that a) res3's destructor gets called (check) b)
 res3's destructor may be called on Resource3.init. That would be a
 new rule similar to 'no internal aliasing' and c) that every
 constructor of C either sets res3 to a destructable value or does
 not touch it at all ('transactional programming').



 But don't you still need to manually cleanup in the case of ctor
 failure? AFAIK, the dtor is not invoked on the partially-constructed
 object if the ctor throws. So you'd still have to use scope(failure) to
 manually release the resource.

 To prove my point, here is some sample code that (tries to) use RAII to
 cleanup:

 	import std.stdio;
 	
 	struct Resource {
 		int x = 0;
 		this(int id) {
 			x = id;
 			writefln("Acquired resource %d", x);
 		}
 		~this() {
 			if (x == 0)
 				writefln("Empty resource, no cleanup");
 			else
 				writefln("Cleanup resource %d", x);
 		}
 	}
 	
 	struct S {
 		Resource res1;
 		Resource res2;
 		Resource res3;
 	
 		this(int) {
 			res1 = Resource(1);
 			res2 = Resource(2);
 			assert(res1.x == 1);
 			assert(res2.x == 2);
 	
 			throw new Exception("ctor failed!");
 			res3 = Resource(3);	// not reached
 			assert(res3.x == 3);	// not reached
 		}
 	}
 	
 	void main() {
 		try {
 			auto s = S(123);
 		} catch(Exception e) {
 			writeln(e.msg);
 		}
 	}

 Here is the program output:

 	Acquired resource 1
 	Empty resource, no cleanup
 	Acquired resource 2
 	Empty resource, no cleanup
 	ctor failed!

 As you can see, the two resources acquired in the partially-constructed
 object did NOT get cleaned up. So, RAII doesn't work in this case.


Fixed?

 			auto r1 = Resource(1);
 			auto r2 = Resource(2);
 			assert(res1.x == 1);
 			assert(res2.x == 2);
 	
 			throw new Exception("ctor failed!");
 			auto r3 = Resource(3);	// not reached
 			assert(res3.x == 3);	// not reached


			res1 = move(r1);
			res2 = move(r2);
			res3 = move(r3);


Exception-safe code after all has to be exception safe.
Unlike in C++ we have no explicit initialization of members (and strict 
order of this) hence no automatic partial cleanup.

-- 
Dmitry Olshansky

Dmitry Olshansky <dmitry.olsh gmail.com> writes:

27-Aug-2013 18:25, H. S. Teoh пишет:
 On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:


 What if move(r2) throws? Then res1 won't get cleaned up, because r1 has
 already been nulled by the move.


Then something is terribly wrong :)

Rule #1 of move should be is that it doesn't throw.

It just blits the damn data. Even if this is currently broken..
At the very least 2-arg version certainly can avoid throw even if T has 
a bogus destructor that goes bottom up on T.init.

BTW same with swap and at least in C++ that's the only way to avoid 
exceptions creeping up from nowhere.


-- 
Dmitry Olshansky

Dmitry Olshansky <dmitry.olsh gmail.com> writes:

28-Aug-2013 00:41, H. S. Teoh пишет:
 On Wed, Aug 28, 2013 at 12:24:43AM +0400, Dmitry Olshansky wrote:
 27-Aug-2013 18:25, H. S. Teoh пишет:
 On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:


 What if move(r2) throws? Then res1 won't get cleaned up, because r1 has
 already been nulled by the move.


 Then something is terribly wrong :)

 Rule #1 of move should be is that it doesn't throw.


 std.algorithm.move is not nothrow. I tried to make it nothrow, and the
 compiler told me:

 std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
 std/algorithm.d(1596): Error: function
'std.algorithm.move!(DirIteratorImpl).move' is nothrow yet may throw
 std/typecons.d(3522): Error: template instance
std.algorithm.move!(DirIteratorImpl) error instantiating
 std/file.d(2526):        instantiated from here: RefCounted!(DirIteratorImpl,
cast(RefCountedAutoInitialize)0)
 std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
 std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is
nothrow yet may throw
 std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error
instantiating
 std/net/curl.d(2040):        instantiated from here: RefCounted!(Impl)
 std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
 std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is
nothrow yet may throw
 std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error
instantiating
 std/net/curl.d(2747):        instantiated from here: RefCounted!(Impl)
 std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
 std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is
nothrow yet may throw
 std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error
instantiating
 std/net/curl.d(3065):        instantiated from here: RefCounted!(Impl)

 Isn't that scary? This means that the case I presented above is not
 hypothetical -- if destroy throws, then you're screwed.


Sorry but this is hypocritical. Obviously nothrow attribute wouldn''t 
work as the only thing we must ensure is that T.init is safely destroyable.

 (And yes it's a very very rare case... but would you want to find out
 the hard way when a problem that triggers a throw in destroy slips past
 QA testing 'cos it's so rare, and shows up in a customer's environment
 where it may take hours or days or even months to debug?)


I'm not sold. Bugs are bugs in any case.

 Basically, the only way to be 100% sure is to use scope(this),


That yet has to be implemented and is full of interesting interplay with 
other features. No thanks.
  or the
 manual equivalent thereof (see below).


That is flawed or use a different code practice.

 It just blits the damn data. Even if this is currently broken..


 It also calls destroy, which is not nothrow. :)


Oh my. And how many things are by  safe but can't be.
The only assertion missing is that destroy of T.init can't throw.
It is btw a common bug...
https://github.com/D-Programming-Language/phobos/pull/1523

 At the very least 2-arg version certainly can avoid throw even if T
 has a bogus destructor that goes bottom up on T.init.

 BTW same with swap and at least in C++ that's the only way to avoid
 exceptions creeping up from nowhere.



 Well this is kinda getting away from the main point, which is that
 scope(this) allows us to hide all these dirty implementation details
 under a compiler-implemented solution that makes sure things are always
 done right. This is part of the reason I wrote DIP44: although it *is*
 possible to manually write code that behaves correctly, it's pretty
 tricky, and I doubt many people even realize the potential pitfalls.
 Ideally, straightforward D code should also be correct by default.


It's always seems easy to hide away an elephant in the compiler. Please 
let's stop this tendency.
Anyhow if we are to reach there a better solution that occurred to me 
would be to make compiler call destructors on "cooked" fields as it 
already keeps track of these anyway. It rides on the same mechanism as 
initializing immutables in constructor. Then original code with 3 RAII 
wrappers works out of the box and job is done.

 Anyway, none of this move/nothrow nonsense is needed if we write things
 this way:


Wrong. Make that 3 resources and then what if res2.release throws? - you 
never get to the first one. Or simply res2.release throws in destructor 
even with 2 of them, chain of cleanups doesn't work. You have to have 
primitives that don't throw at some level of this or continue wrapping 
in try/finally:)

	struct S {
		void delegate()[] __cleanups;
		Resource res1;
		Resource res2;
		Resource res3;

		this() {
			scope(failure) __cleanup();
			res1 = acquireResource();
			__cleanups ~= { res1.release(); };

			res2 = acquireResource();
			__cleanups ~= { res2.release(); };

			res3 = acquireResource();
			__cleanups ~= { res3.release(); };
		}

		~this() { __cleanup(); }

		private void __cleanup() {
			foreach_reverse (f; __cleanups)
				f();
		}
	}


-- 
Dmitry Olshansky

Walter Bright <newshound2 digitalmars.com> writes:

On 8/24/2013 1:09 PM, H. S. Teoh wrote:
 On Sat, Aug 24, 2013 at 12:27:37PM -0700, Walter Bright wrote:
 [...]
 Not a bad idea, but it has some issues:

 1. scope(failure) takes care of most of it already

 2. I'm not sure this is a problem that needs solving, as the DIP
 points out, these issues are already easily dealt with. We should be
 conservative about adding more syntax.

 3. What if the destructor needs to do more than just unwind the
 transactions? Where does that code fit in?


 I think it's unhelpful to conflate scope(this) with dtors. They are
 related, but -- and I guess I was a bit too strong about saying dtors
 are redundant -- if we allow both, then scope(this) can be reserved for
 transactions, and you can still put code in ~this() to do non-trivial
 cleanups.


If you take out automatic dtor generation, I see no difference between 
scope(this) and scope(failure).


 4. The worst issue is the DIP assumes there is only one constructor,
 from which the destructor is inferred. What if there is more than
 one constructor?


 This is not a problem. If there is more than one constructor, then only
 those scope(this) statements in the ctor that were actually encountered
 will trigger when the object reaches the end of its lifetime.


Do you mean multiple dtors will be generated, one for each constructor?

Walter Bright <newshound2 digitalmars.com> writes:

On 8/24/2013 10:35 PM, H. S. Teoh wrote:
 On Sat, Aug 24, 2013 at 06:50:11PM -0700, Walter Bright wrote:
 On 8/24/2013 1:09 PM, H. S. Teoh wrote:
 On Sat, Aug 24, 2013 at 12:27:37PM -0700, Walter Bright wrote:
 [...]
 Not a bad idea, but it has some issues:

 1. scope(failure) takes care of most of it already

 2. I'm not sure this is a problem that needs solving, as the DIP
 points out, these issues are already easily dealt with. We should be
 conservative about adding more syntax.

 3. What if the destructor needs to do more than just unwind the
 transactions? Where does that code fit in?


 I think it's unhelpful to conflate scope(this) with dtors. They are
 related, but -- and I guess I was a bit too strong about saying dtors
 are redundant -- if we allow both, then scope(this) can be reserved
 for transactions, and you can still put code in ~this() to do
 non-trivial cleanups.


 If you take out automatic dtor generation, I see no difference
 between scope(this) and scope(failure).


 4. The worst issue is the DIP assumes there is only one constructor,
from which the destructor is inferred. What if there is more than
 one constructor?


 This is not a problem. If there is more than one constructor, then
 only those scope(this) statements in the ctor that were actually
 encountered will trigger when the object reaches the end of its
 lifetime.


 Do you mean multiple dtors will be generated, one for each
 constructor?


 No. Given this code:

 	class C {
 		this(int) {
 			scope(this) writeln("A1");
 			scope(this) writeln("A2");
 		}

 		this(float) {
 			scope(this) writeln("B1");
 			scope(this) writeln("B2");
 		}
 	}

 The lowered code looks something like this:

 	class C {
 		this(int) {
 			scope(failure) __cleanup();

 			// scope(this) writeln("A1");
 			// Translated into:
 			__cleanups ~= { writeln("A1"); };

 			// scope(this) writeln("A2");
 			// Translated into:
 			__cleanups ~= { writeln("A2"); };
 		}

 		this(float) {
 			scope(failure) __cleanup();

 			// scope(this) writeln("B1");
 			// Translated into:
 			__cleanups ~= { writeln("B1"); };

 			// scope(this) writeln("B2");
 			// Translated into:
 			__cleanups ~= { writeln("B2"); };
 		}

 		void delegate()[] __cleanups;

 		void __cleanup() {
 			foreach_reverse (f; __cleanups)
 				f();
 		}

 		~this() {
 			__cleanup();
 		}
 	}

 So, there is only one dtor. But it automatically takes handles
 triggering the scope(this) statements of only the ctor that was actually
 used for creating the object. And if the ctor didn't successfully
 complete, it only triggers the scope(this) statements that have been
 encountered up to that point.

 Of course, the above lowered code is just to illustrate how it works.
 The actual implementation can be optimized by the compiler. E.g., if
 there is only one ctor and the sequence of scope(this) can be statically
 determined, then the cleanup statements can be put into the dtor
 directly without incurring the overhead of allocating the __cleanups
 array. If the cleanups don't need closure over ctor local variables,
 then they can just be function ptrs, not delegates. Only when you're
 doing something complicated do you actually need an array of delegates,
 which should be relatively rare.


Your example, again, is of an auto-generated dtor. But you said earlier that 
wasn't the point.

Without the auto-generated dtor, it is just scope(failure), which is already a
D 
feature.

I don't get it.

Walter Bright <newshound2 digitalmars.com> writes:

On 8/25/2013 12:14 AM, H. S. Teoh wrote:
[...]

Ok, you had thrown me off by saying it wasn't about dtors. It is all about
dtors 
:-) I think it is an implementable proposal.

However, I tend to agree with Andrei's comment on the utility of it.

"H. S. Teoh" <hsteoh quickfur.ath.cx> writes:

On Sat, Aug 24, 2013 at 11:30:26PM -0700, Walter Bright wrote:
[...]
 Your example, again, is of an auto-generated dtor. But you said
 earlier that wasn't the point.
 
 Without the auto-generated dtor, it is just scope(failure), which is
 already a D feature.
 
 I don't get it.


It's a way to avoid code duplication in the dtor. If you just had
scope(failure), you have to factor out a __cleanup method in order to do
cleanup on both a failed ctor call or a dtor call, as I did in the
example.

I don't know if it will clarify things, but if you already have a dtor,
then scope(this) just adds to it:

	class C {
		this() {
			scope(this) writeln("A");
			scope(this) writeln("B");
		}
		~this() {
			writeln("In dtor");
		}
	}

This would get lowered to the equivalent of:

	class C {
		this() {
			scope(failure) __cleanup();
			__cleanups ~= { writeln("A"); };
			__cleanups ~= { writeln("B"); };
		}

		void delegate()[] __cleanups;

		void __cleanup() {
			foreach_reverse (f; __cleanups)
				f();
		}

		~this() {
			writeln("In dtor");
			__cleanup();
		}
	}


T

-- 
Skill without imagination is craftsmanship and gives us many useful
objects such as wickerwork picnic baskets.  Imagination without skill
gives us modern art. -- Tom Stoppard