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

I'm not sure what to do with that, and the existence of such holes
makes the idea of formalizing the concept probably untenable. But that
doesn't change the idea that it is reasonable to consider Indirectly
Block Scoped usage of types that are intended to be block scoped to be
dubious.

> Why, other than "because it
> breaks the concept?" And unique_ptr is just one example.
>
>
>
> >
> >
> >> Meanwhile, using scope_success and scope_failure in coroutine block scope
> >> across suspension points is completely natural; the only problem is that it
> >> can break. We should not be adding footguns like that to the standard.
> >
> > Coroutines already break `thread_local` (details:
> > https://stackoverflow.com/questions/75448478/thread-local-variables-and-coroutines/75449918#75449918).
> > So I submit that coroutines already added footguns to the language.
> >
> > The avalanche has already started; it is too late for the pebbles to vote.
> > --
> > Std-Discussion mailing list
> > Std-Discussion_at_[hidden]
> > https://lists.isocpp.org/mailman/listinfo.cgi/std-discussion
>
>
>
>