stop adding dropck outlives requirements for [T; 0]

[lcnr]

[T; 0] currently doesn't need drop glue by itself but still adds liveness requirements for T when used in a more complex value which already needs drop.

This behavior is identitical to PhantomData, which also does not need drop glue by itself but adds liveness requirements when used in values who do. I do not propose to change anything about PhantomData with this issue.

Example

playground

struct PrintOnDrop<'s>(&'s str);
impl<'s> Drop for PrintOnDrop<'s> {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

fn to_array_zero<T>(_: T) -> [T; 0] {
    []
}

pub fn array_zero_by_itself() {
    let mut x = [];
    {
        let s = String::from("temporary");
        let p = PrintOnDrop(&s);
        x = to_array_zero(p)
    }
    // implicitly dropping `x` here, no drop glue needed,
    // so ignoring outlives requirements. NO ERROR
}

pub fn array_zero_in_tuple() {
    let mut x = ([], String::new());
    {
        let s = String::from("temporary");
        let p = PrintOnDrop(&s);
        x.0 = to_array_zero(p)
    }
    // implicitly dropping `x` here, drop glue needed to drop the `String`,
    // adding outlives requirements for the zero array. ERROR
}

This behavior is confusing. I propose that we don't add any dropck outlives requirements for arrays of length zero. This allows strictly more code to compile, it may however result in unsoundness if people rely on this behavior to guarantee errors.

[lcnr]

cc #103413 for more details about dropck

@rfcbot fcp merge

[rfcbot]

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

Concerns:

• rushed-fcp resolved by stop adding dropck outlives requirements for [T; 0] #110288 (comment)
• we could instead stop specialcasing [T; 0] in needs_drop resolved by stop adding dropck outlives requirements for [T; 0] #110288 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[RalfJung]

How (if at all) does this interact with const generics? What if the length is unknown but could be zero? Seems surprising that behavior depends so heavily on whether the compiler can "figure out" the length.

[lcnr]

This happens after type inference, so we should only fail to evaluate for generic constants. In this case they are always considered to be potentially non-zero.

Seems surprising that behavior depends so heavily on whether the compiler can "figure out" the length.

yes, I considered to propose changing the behavior for arrays to always require drop glue, even for N == 0 because relying on the value of constants in this way is always a bit sketchy. The problem is that this is actually quite useful.

I am a bit confused by why exactly this errors, but the following snippet would fail if we remove the special case for zero length arrays in needs_drop

const fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

error[E0493]: destructor of `[T; 0]` cannot be evaluated at compile-time
 --> src/main.rs:2:6
  |
2 |     &[]
  |      ^^ the destructor for this type cannot be evaluated in constant functions
3 | }
  | - value is dropped here

opened #110322 if you want to take a deeper look at this. Afaict we never actually drop [T; 0] here so this is the const check being too pessimistic (iirc using dataflow for that was too involved so we didn't end up stabilizing precise const drop).

@rfcbot concern we could instead stop specialcasing [T; 0] in needs_drop

[RalfJung]

Afaict we never actually drop [T; 0] here so this is the const check being too pessimistic (iirc using dataflow for that was too involved so we didn't end up stabilizing precise const drop).

What probably happens is that [] cannot get promoted any more because it looks like that would skip some drop glue. Presumably this also fails with your patch, saying the value does not live long enough?

fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

[lcnr]

no, this still works 😅 apparently promotion does work. Had the same idea at first when seeing the test failures

[RalfJung]

Hm, maybe promotion has its own knowledge that [] never needs to run any drop glue...

[steffahn]

Promotion for arrays (unlike any other type) even supports mutable references, i.e. &mut [] promotes, so I wouldn’t be surprised if &[] hits some special case as well.

[lcnr]

@rfcbot resolve we could instead stop specialcasing [T; 0] in needs_drop

doing so results in too many regressions, see #110322

[RalfJung]

@rfcbot resolve we could instead stop specialcasing [T; 0] in needs_drop

doing so results in too many regressions, see #110322

I'm not convinced yet that we want to ditch that option. Clearly there's something going on that we don't understand -- if this still works

fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

then this should also still work

const fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

This looks like an issue specific to promotion, not something related to dropck in general.

[RalfJung]

Note that this also works

fn foo() -> &'static Option<Vec<i32>> { &None }

so "promotion of types that need dropping, if we know that the value doesn't need dropping", is already a thing. &[] would also fit that category.

This works even with const, so it seems worth figuring out why it stops working for arrays in const fn.

[tmiasko]

I am a bit confused by why exactly this errors, but the following snippet would fail if we remove the special case for zero length arrays in needs_drop

const fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

If a local is defined by an assignment from Rvalue::Aggregate, its value needs
to be dropped if a type inherently needs drop, i.e., it implements Drop, or if
any operands needs to be dropped (that in turns depends how those locals or
constants are defined). For example:

pub const fn f() {
    // OK:
    let _a: [Option<String>; 2] = [None, None];
    let _b: &'static [Option<String>; 2] = &[None, None];
}

Additionally, in const checking (but not in promotion which works differently
here), if a place with a type that needs drop is borrowed, and borrows allows
for mutation (mutable borrow or borrowed place is not Freeze), then the value
of local needs dropping. For example:

use std::cell::Cell;
pub const fn g() {
    // OK:
    let _a: Option<Cell<String>> = None;
    // Error:
    let _b: Option<Cell<String>> = None;
    &_b;
}

Note that [T; 0] is not Freeze, and without the special case it needs drop.

[jackh726]

@rfcbot concern rushed-fcp

I personally don't feel comfortable making non-gated changes here right now. As I noted on zulip, I'm not really satisfied with the answer regarding const generics. And, it seems like there is still deeper discussion to be had (glancing at the above comments).

@lcnr I would feel more comfortable canceling FCP here in favor of more discussion and gated experimentation. Ultimately, I might be swayed with a very detailed and well thought out writeup of the problems here and intended solution, though.

[SoniEx2]

rushed-fcp seems fair, how about rust-lang/rfcs#3414 ?

[lcnr]

hmm, I originally assumed to this be fairly straightforward. Will attempt to provide a full summary of the issue and the proposed solution:

the issue

Implicitly dropping variables during borrowck is different from explicitly calling drop in that the variables don't have to be fully live. Dropping a variable uses drop glue. Conceptionally drop glue recursively calls Drop::drop for all owned values. For drop glue to be sound all regions used in a - potentially builtin - Drop impl have to be live.

Computing this is currently done in two steps:

• Check whether there is any drop glue via tcx.needs_drop(ty), i.e. may variables of this type own a value which has a Drop impl.
• If so, compute the regions which have to be live for these Drop impls using tcx.dropck_outlives.

For [T; 0] these two methods disagree: needs_drop returns false while dropck_outlives assumes that we may drop a T in the drop glue of T (which does not exist). This means that whether implicitly dropping [T; 0] requires T to be droppable relies on whether another type owned by this variable requires drop glue. See this playground example for how this impacts stable code. I consider this current behavior to clearly be a bug and something we have to fix¹.

how can we fix this

I believe there to only be two valid options

change needs_drop

This is the alternative proposed by @RalfJung. The idea is to change needs_drop to remove the specialcase for zero length arrays. We would add (noop) drop glue for [T; 0] if T has drop glue.

This removes our reliance on const generics/const evaluation in drop computation. I have implemented that approach in #110322.

change dropck_outlives

This is what I proposed for FCP. We add a check for zero lenth arrays in dropck_outlives. This would mean that we now consistently consider [T; 0] to not have any drop glue.

which approach should we choose

I strongly believe that we should keep the specialcase for [T; 0] and also add it in dropck_outlives.

why remove the [T; 0] specialcase

Afaict the main concern is that specialcasing [T; 0] relies on const generics. Checking implicit drops only happens after type inference. This means array lengths are in one of the following states:

• ty::ConstKind::Param: could be anything seems trivially correct to require T to be droppable.
• (evaluable to) ty::ConstKind::Value: can trivially check for 0.
• a generic constant (with feature(generic_const_exprs)): If the constant is actually still generic and relies on a generic parameter, it also feels fairly straightforward to require T to be droppable.

The actual interaction with const generics is therefore really limited and straightforward imo.

I stated the following in a previous comment:

yes, I considered to propose changing the behavior for arrays to always require drop glue, even for N == 0 because relying on the value of constants in this way is always a bit sketchy. The problem is that this is actually quite useful.

The reason I consider this to be slightly sketchy is that it is hard to emulate by purely using the trait system, especially on stable. I don't know how (and why) we would ever move dropck fully into the trait system so I am not very worried here.

why consistently apply the [T; 0] specialcase

I think that the specialcase is both actually desirably and necessary for backwards compatability.

If [T; 0] acts if it may drop T, one cannot use references to [T; 0] in const contexts. This breaks using generic empty slices: &[].

const fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

See the comment by @tmiasko for why this breaks. I expect it to be difficult to resolve this issue without adding another specialcase depending on the array length for that. I may be wrong and there's a sensible way to change const checking. @tmiasko should have a better understanding of the design space here. Crater found a few root regressions with a total of a few hundred affected projects.

Even if we fix this specific somewhat weird regression this would still be a breaking change. We would for example break size_of_trait which actually drops an array of length zero. I dislike the idea of breaking existing code without a strong reason.

Ignoring the status quo, I still think we should specialcase [T; 0]. It allows strictly more code to soundly compile and I believe it fits the way users - and I - think about Rust. I don't expect anyone to be confused by [T; 0] not requiring T to be live and can imagine bug reports if they encounter a case where we do.

Even if we do end up moving dropck into the trait system the specialcase for zero does not cause any new issue as we have a similar issue with Default and in serde for [T; 0]. I think that going forward we already want to add a way to enable this pattern using stable trait system features.

---

I hope this writeup is useful. The only place that's still unclear to me is whether the current const checking behavior is too strict and we can change this pattern to compile:

const fn zero_sized<T>() -> &'static [T; 0] {
    &[]
}

That won't change my perspective on this FCP though. I don't believe that looking further into this is worth it, unless you first disagree with the rest of my arguments.

Footnotes

1. The same issue exists for PhantomData<T> which I am trying to fix separately via https://github.com/rust-lang/rfcs/pull/3417 as that is more involved. ↩

[SoniEx2]

if we do opt to apply needs_drop to [T; 0], a simple fix for crates that use it for alignment (since, unlike PhantomData, [T; 0] follows the alignment of T) would be to use ManuallyDrop with it. tho not sure if that works with const.

[RalfJung]

@tmiasko oh I see... with interior mutability the value might change before it is dropped so the value-based analysis gives up. That makes sense.

@lcnr thanks for the writeup!

See #110288 (comment) by @tmiasko for why this breaks. I expect it to be difficult to resolve this issue without adding another specialcase depending on the array length for that.

Yes that is probably correct, so this pretty much defeats my argument.

Ignoring the status quo, I still think we should specialcase [T; 0]. It allows strictly more code to soundly compile and I believe it fits the way users - and I - think about Rust. I don't expect anyone to be confused by [T; 0] not requiring T to be live and can imagine bug reports if they encounter a case where we do.

This is fair. My remaining concern here is consistency: the reasoning above applies not just to dropping. To name an example, [Vec<T>; 0] is not Copy even though it could be. So while "array length 0 does not give any special powers" might be surprising at first, at least it is easy to teach and explain and internalize. The alternative is a world where some analyses know about empty arrays being special and others do not. This can be more startling than applying the same principle consistently.

Of course we could set a goal to make 0 special for more cases (Copy, Clone, Default, ...); then the question is -- how realistic is it to achieve that goal?

Even if we do end up moving dropck into the trait system the specialcase for zero does not cause any new issue as we have a similar issue with Default and in serde for [T; 0]. I think that going forward we already want to add a way to enable this pattern using stable trait system features.

Well, it would force us to add such a special case to the trait system before we can do the move. Currently we don't have such a special case, right? And it is unclear whether specialization will be powerful enough for this.

[lcnr]

Of course we could set a goal to make 0 special for more cases (Copy, Clone, Default, ...); then the question is -- how realistic is it to achieve that goal?

We already have that special case for Default, blocking us from replacing the 33 impls with const generics 😁 https://doc.rust-lang.org/nightly/std/primitive.array.html#impl-Default-for-%5BT;+0%5D

The specialcase also exists in the wild, e.g. in serde: https://doc.rust-lang.org/nightly/std/primitive.array.html#impl-Default-for-%5BT;+0%5D

Using some compiler hack and marker traits it is already possible right now, allowing this behavior on stable is probably also a longterm goal.

[RalfJung]

We already have that special case for Default

Ah, fair, I didn't know that.

[lcnr]

will discuss this as part of rust-lang/types-team#92

[jackh726]

@rfcbot resolve rushed-fcp

While I'm not ready to check my box yet, this was obviously discussed, so resolving concern

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[lcnr]

implemented in #128438, we still need to update the documentation however, cc @Bryanskiy