DIP44: scope(class) and scope(struct)

[H. S. Teoh]

On Wed, Aug 28, 2013 at 01:02:27AM +0400, Dmitry Olshansky wrote:
> 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.

I'm not sure I understand you. Since the compiler tells me that
std.algorithm.move can't be made nothrow, I, from a user's POV, have no
choice but to believe that there are *some* circumstances in which it
*will* throw.


> >(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.

Yes, but if you could do something today to prevent bugs in the future,
why not?


> >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.

How is it flawed?


> >>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

Yes, this is a common bug. So it's a real possibility that move() may
throw. And now we know that if File's were involved, it could actually
happen.  ;-)

I'd argue that, independently of DIP44, destroy() should be marked
nothrow:

	void destroy(T)(ref T t) nothrow {
		...
		try {
			callDtor(t);
			// or whatever the syntax is, I don't remember
			// now.
		} catch(Error e) {
			// If we get here, we're royally screwed anyway,
			// so might as well give up.
			assert(0);
		}
	}

I mean, if an object is going out of scope or being GC'd and the dtor is
invoked, and the dtor throws an exception, then what you are gonna do?
Just catch the exception and try to destroy the object again? You can't,
because by the time the stack unwinds the object doesn't exist anymore.

Or, in the case of move(), the code cannot continue because it needs to
destroy the previous object but the destruction failed, so what is the
catcher going to do? It may not have access to the scope in which the
object is defined, so it can't possibly do any real cleanup. And if the
object was created in a scope below the catch block, the unwinding code
will encounter the dtor exception a second time... in any case, we're
completely screwed so there's no point in continuing.

Once move() is nothrow, then I agree you have a point that you can
initialize resources as ctor local variables first, then assign them to
member fields.

Ultimately, though, in machine code it's not really any different from
putting scope(failure) __cleanup() in the ctor. Just the same solution
implemented in different ways.


> >>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.

Does the compiler keep track of what has been initialized? If it does,
then I agree that it should call dtors on "cooked" fields on ctor
failure. That would eliminate a large part of the reason for DIP44.

As it stands, though, partially-initialized objects are just left as-is,
with no attempt to cleanup (as I've shown in a previous post). The only
way around this currently is to manually keep track of what has been
initialized and use scope(failure) (or equivalent) to cleanup.


> > 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:)

Well, OK, so make the definition of __cleanups nothrow:

	alias CleanupFunc = void delegate() nothrow;
	CleanupFunc[] __cleanups;

Then the compiler can statically reject any cleanup code that may throw.
Currently, I can't say the same for std.algorithm.move().


T

-- 
Маленькие детки - маленькие бедки.