Re: [PATCH 1/2] rust: alloc: extend safety requirements of Vec::set_len()

[Danilo Krummrich <dakr@kernel.org>]

On Sun, Mar 16, 2025 at 05:40:29PM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 2:46 PM CET, Danilo Krummrich wrote:
> > On Sun, Mar 16, 2025 at 09:13:50AM -0400, Tamir Duberstein wrote:
> >> On Sun, Mar 16, 2025 at 9:01 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > On Sun, Mar 16, 2025 at 08:42:43AM -0400, Tamir Duberstein wrote:
> >> > > On Sun, Mar 16, 2025 at 8:31 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >> > >
> >> > > > Let's go with just "must be taken ownership of" then. Unless there's subsequent
> >> > > > feedback, I won't send a new version for this, since you both already gave your
> >> > > > (conditional) RB for this.
> 
> I didn't realize the `forget` argument immediately, so I sadly have to
> take my RB back.
> 
> >> > > What does it mean to take ownership, if not to run the destructor?
> >> >
> >> > Taking responsibility over the decision of what should happen to the value, i.e.
> >> > forget about the value, drop it right away, keep it alive at a different memory
> >> > location.
> >> >
> >> > > Given Benno's insight, I think the safety text is correct as it
> >> > > existed before this patch.
> >> >
> >> > Without the addition there's no requirement for the caller to take ownership,
> >> > but that's what we want here. Without the requirement it would be on set_len()
> >> > to take a decision on what should happen with the value.
> >> 
> >> It isn't a safety requirement, right? I think it's fine to document,
> >> but given that neglecting to run a destructor is safe, it doesn't seem
> >> to affect safety.
> >
> > I don't think there's a rule that safety requirements of unsafe functions must
> > only be used to guarantee memory safety, etc.
> 
> Well that's their only point. Safety documentation should only be
> concerned with memory safety.

Oops, I wanted to say "must not only be used to *directly* guarantee memory
safety, etc.".

For instance, it is also used to justify type invariants (that exist to prevent
UB).

So, what I wanted to say is that setting self.len by iteself is never directly a
safety critical thing.

> You can still put it in the normal documentation though.
> 
> > But you're right, if set_len() guarantees a default of what happens to the
> > value(s) when the caller doesn't take ownership, we could avoid the safety
> > requirement.
> 
> Yeah the default is to forget the value.

It's not really a defined default though, it's just what ultimately happens as
the consequence of how Vec in general and set_len() are implemented.

But let's define it then; what about:

"[`Vec::set_len`] takes (or kepps) ownership of all elements within the range
[0; `new_len`] and abandons ownership of all values outside of this range, if
any."

The caller may take ownership of the abandoned elements."

I'd argue that giving up ownership, while offering someone else to take it means
that it implies that otherwise we'll just end up forgetting about the value.