On Sun, Apr 16, 2023 at 2:18 PM Lénárd Szolnoki via Std-Discussion
<std-discussion_at_[hidden]> wrote:
>
> On Sun, 2023-04-16 at 12:38 -0400, Jason McKesson via Std-Discussion
> wrote:
> > ---------- Forwarded message ---------
> > From: Jason McKesson <jmckesson_at_[hidden]>
> > Date: Sun, Apr 16, 2023 at 12:38 PM
> > Subject: Re: [std-discussion] Some feedback on scope guards
> > To: Andrey Semashev <andrey.semashev_at_[hidden]>
> >
> >
> > On Sun, Apr 16, 2023 at 11:36 AM Andrey Semashev
> > <andrey.semashev_at_[hidden]> wrote:
> > >
> > > On April 16, 2023 5:29:27 PM Jason McKesson via Std-Discussion
> > > <std-discussion_at_[hidden]> wrote:
> > >
> > > > On Sun, Apr 16, 2023 at 7:34 AM Edward Catmur via Std-Discussion
> > > > <std-discussion_at_[hidden]> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On Sun, 16 Apr 2023 at 08:11, Ville Voutilainen
> > > > > <ville.voutilainen_at_[hidden]> wrote:
> > > > > >
> > > > > > On Sun, 16 Apr 2023 at 13:40, Edward Catmur
> > > > > > <ecatmur_at_[hidden]> wrote:
> > > > > > > > > Fine, and how do we specify the facility that such
> > > > > > > > > moves have
> > > > > > > > > undefined/unspecified behavior but the cases we want to
> > > > > > > > > allow do? I can't
> > > > > > > > > see a way.
> > > > > > > >
> > > > > > > > We don't. If I want to store these things in a vector and
> > > > > > > > move the
> > > > > > > > vector around, that's my business. I'll need to disable
> > > > > > > > the elements
> > > > > > > > if the vector
> > > > > > > > has been moved. If I want to wrap these things in a
> > > > > > > > shared_ptr after
> > > > > > > > allocating them dynamically, and then store those
> > > > > > > > shared_ptrs into a
> > > > > > > > vector,
> > > > > > > > that's my business, and none of the library's to prevent.
> > > > > > >
> > > > > > >
> > > > > > > And we need to know what happens when you do that, or know
> > > > > > > that it is
> > > > > > > unspecified. What I'm saying is that the behavior needs to
> > > > > > > be specified, or
> > > > > > > explicitly unspecified. If the behavior is specified in
> > > > > > > terms of
> > > > > > > uncaught_exceptions(), that takes care of it, but leaves
> > > > > > > the coroutine case
> > > > > > > broken.
> > > > > >
> > > > > > I fail to see the problem. It's as if you're suggesting that
> > > > > > there's
> > > > > > some unknown specification challenge here, but we have the
> > > > > > specification
> > > > > > you speak of already, in the TS.
> > > > >
> > > > >
> > > > > And that specification breaks with coroutines.
> > > > >
> > > > > > > > No library type prevents using it as a subobject, or
> > > > > > > > prevents itself
> > > > > > > > completely from working with other library types. It sure
> > > > > > > > as hell
> > > > > > > > matters.
> > > > > > >
> > > > > > >
> > > > > > > std::typeinfo? std::chrono::time_zone?
> > > > > >
> > > > > > Yes? Neither of those needs other than pointers to them to
> > > > > > work. Scope
> > > > > > guards are not like them.
> > > > > > You found two cases that presumably prove my statement wrong,
> > > > > > but
> > > > > > (pointers to) those types can
> > > > > > be used as subobjects, that works fine, and they work with
> > > > > > other
> > > > > > library types without problems, since
> > > > > > for them it's truly so that you don't need by-value
> > > > > > subobjects of them.
> > > > > >
> > > > > > Try harder.
> > > > >
> > > > >
> > > > > And you can take pointers to scope guards. Consider also
> > > > > immovable types
> > > > > like scoped_lock - those won't work with most library
> > > > > components.
> > > > >
> > > > > > > > > Working well can be dealt with by the vendor since
> > > > > > > > > these are library types.
> > > > > > > > > Wrong - maybe, but this is a scope guard! The name
> > > > > > > > > indicates it should be
> > > > > > > > > used in a scope.
> > > > > > > > Yeah, which does include packaging them any which way I
> > > > > > > > please and
> > > > > > > > passing them down to helper functions for further
> > > > > > > > processing,
> > > > > > > > or just having a collection-bunch of them in a scope,
> > > > > > > > rather than
> > > > > > > > having to have individual objects of the scope guard type
> > > > > > > > directly,
> > > > > > > > without
> > > > > > > > the possibility to bunch them.
> > > > > > > I've never seen code where that would actually be useful.
> > > > > >
> > > > > > Uhh.. you've never seen code that packages multiple
> > > > > > individual objects
> > > > > > into a bunch, to pass them as one object to be processed
> > > > > > by a helper function? You can't fathom code where you'd take
> > > > > > a pack of
> > > > > > callbacks and wrap scope guards on top of them, variadically?
> > > > >
> > > > >
> > > > > No, since you may as well wrap the lot into a single scope
> > > > > guard.
> > > > >
> > > > > > Yeah, sure, I can show you that example you asked for. In my
> > > > > > copious free time.
> > > > > >
> > > > > > > > The current design in the TS is just fine. It specifies
> > > > > > > > how the
> > > > > > > > destructor of these things works, and tells you clearly
> > > > > > > > enough what
> > > > > > > > happens if you move a container of these things around
> > > > > > > > into a scope
> > > > > > > > that has a different value for uncaught_exceptions() than
> > > > > > > > the original scope had.
> > > > > > > It's not fine, because it breaks with coroutines.
> > > > > >
> > > > > > Well, gee whizz, that's an acceptable cost, compared to
> > > > > > preventing the
> > > > > > use of scope guards as subobjects, which is not an
> > > > > > acceptable cost.
> > > > >
> > > > >
> > > > > Using scope_guard outside block scope is a code smell, though
> > > > > it can be
> > > > > justified in some rare cases. Using scope_success or
> > > > > scope_failure outside
> > > > > block scope would not pass code review where I work.
> > > >
> > > > Essentially, you're declaring that there is a class of types
> > > > which
> > > > should always be block scoped. I think this is a reasonable
> > > > thought.
> > > > However, if you are writing such a type, then... doesn't it make
> > > > sense
> > > > that this type can *itself* contain members of similar
> > > > block-scoped-only types? It's a transitive property: if type X
> > > > must
> > > > always be block scoped, and type Y must always be block scoped,
> > > > then
> > > > type Y can contain a member of type X and the invariant will
> > > > remain.
> > > >
> > > > So why is it a code smell?
> > > >
> > > > It'd be great if we had some kind of attribute that would make it
> > > > a
> > > > warning to use such a type in a non-block scoped way, or outside
> > > > of
> > > > other block-scoped types. But otherwise, the concept is valid.
> > > > And so
> > > > long as that concept is valid, there is no reason to a priori
> > > > forbid
> > > > their use as members of other block-scoped types.
> > >
> > > No, the concept doesn't look valid to me.
> >
> > I think we may have mixed up the "concept" in question. The attribute
> > thing was just an off-the-cuff suggestion. "The concept" was the
> > validity of having block-scoped types being a member of another
> > object
> > which itself is block-scoped.
> >
> > > What about having a block-scoped
> > > unique_ptr pointing to such type? Presumably, unique_ptr won't be
> > > marked as
> > > block-scoped, so should this be forbidden?
> >
> > "Forbidden" is a strong word, but should it be discouraged? Yes.
> >
> > Let's break this down. What we have is a type that is, by design,
> > intended to be used in a statically block-scoped way. The type is
> > non-moveable, so users cannot transfer ownership from its originating
> > named object (prvalues allow them to escape a function call, but at
> > some point, it gets a name and cannot be moved from there). This is a
> > conceptual invariant: once they get a name, their destructor is
> > statically bound to the C++ language scope of that name (outside of
> > doing oddball things like directly calling the destructor).
> >
> > There are three cases for such a type:
> >
> > 1. "Direct block scoped": Direct use as a block-scoped object.
> > 2. "Manifestly Block Scoped": The type is a by-value subobject of
> > some
> > other object that is itself declared as a block-scoped object. This
> > can propagate up a hierarchy of by-value subobjects, but the most
> > derived object is being created as a block-scoped object.
> > 3. "Indirectly Block Scoped": There is a containment relationship
> > manufactured out of code. That is, the object contains some
> > block-scope intended object, and its destructor will call that
> > object's destructor.
> >
> > My contention is that the only ways Manifestly Block Scoped can break
> > are the ways that Direct Block Scoped can break. They are equally
> > reliant on the same language features, so a type that intends you to
> > use it in a Direct Block Scoped way should be equally usable in a
> > Manifestly Block Scoped way. So if we wanted to have a language
> > feature to formalize a "block scoped type", it ought to cover both of
> > these.
> >
> > By contrast, Indirectly Block Scoped feels... fragile. It no longer
> > relies directly on the language to statically ensure block scope; it
> > relies on runtime behavior. Your `unique_ptr<T>` example allows
> > ownership of `T` to be moved out of the function it was created in.
> > `T` can escape the static scope of its creation. A particular *usage*
> > of `unique_ptr<T>` may not actually move it. But it opens up
> > vulnerabilities that Manifestly Block Scoped objects simply do not
> > allow.
> >
> > As such, if we were to have some language feature that allows you to
> > declare a block-scoped type, it should warn on attempts to use it in
> > an Indirectly Block Scoped way.
> >
> > You can have an `array` of manifestly block scoped objects. You can
> > have a `tuple` of manifestly block scoped objects`. But once you
> > start
> > using `unique_ptr` and such things, you have left the space of
> > linguistic safety that block-scoped types are intended to support.
> >
> > And that's where we get into the grey areas: types which use runtime
> > execution to transform Indirect Block Scope back into Manifestly
> > Block
> > Scoped. `optional<T>` has to manually run a destructor on the object.
> > But otherwise, it propagates `T`s behavior; if `T` is block-scoped,
> > it
> > uses runtime mechanisms to ensure that it cannot leave that scope.
> > Unlike `unique_ptr<T>`, it doesn't allow `T` to leak. And yes, you
> > can
> > destroy the `T`, but you could do that with a stack variable of type
> > `T` by calling the destructor, so it's not like there's some new
> > vulnerability at play. So conceptually, it should be Manifestly Block
> > Scoped. But it wouldn't be, and only because it must manufacture the
> > language protections through runtime code.
>
> No, I don't think it makes sense to exempt std::optional<T>. While it
> does put an upper bound to the lifetime of a managed object, it's not
> enough. The lifetime should span exactly from the declaration to the
> end of the containing block.
>
> With optional<T> it's possible to start the lifetime of a managed
> object at the point of a declaration and then end it in the middle of
> some inner catch block with reset(). Or conversely start the lifetime
> of the managed object with emplace() in an inner catch block and end
> its lifetime outside of it. Both of these break scope_fail and
> scope_success in interesting ways.

```
scope_guard foo = ...;

foo.~decltype(foo)();
new(&foo) decltype(foo)(...);
```

Oh sure, this is terrible code. But it is just as viable and correct
as using `optional<scope_guard>::emplace`. Yes, `emplace` is certainly
more convenient, and `reset` is not nearly as fragile as direct
destructor calls. But my point is that, while `optional` does have
ways to circumvent block scoping, those ways are exactly equivalent to
*existing* ways that you can circumvent block scoping. So while it
does lower the bar to undoing block scope, it only does so in the same
ways that existing tools can.

By contrast, there is no equivalent to using `unique_ptr<T>`'s move
constructor to move the scope out of the scope. So while
`unique_ptr<T>` is dubious, I would consider `boost::scoped_ptr<T>` to
be as safe as using `T` directly.