Holding a non-Send Copy type across a yield should be allowed

[jyn514]

For non-Send types with trivial drop, we still consider them held across the generator, even with -Zdrop-tracking:

async fn trivial_drop() {
    // uncomment to make this program compile
    // {
    let x: *const usize = &0; //~ ERROR has type `*const usize` which is not `Send`
    // }
    async {}.await;
}

fn assert_send<T: Send>(_: T) {}

fn main() {
    assert_send(trivial_drop());
}

There's no reason to do this; *const T has no drop impl and it's not possible to read or write it if it's not explicitly used after the yield.

Originally posted by @jyn514 in #98754 (comment)

[jyn514]

cc @tmandry, not sure if people have explicitly thought about this or it just happens to be forbidden by the current implementation.

[eholk]

We talked about this issue in triage last time and this time (because I forgot to mark it as triaged the first time).

Thanks for the PR! I'll take a closer look at it soon. It looks simpler than I was expecting, so that's a pleasant surprise. I was afraid fixing this issue would need a liveness pass, but maybe we already have the information we need?

The first time we talked about this there was some question about whether we want this behavior. They main concerns were around backwards compatibility and how complex we want the algorithm here to be. Although if it turns out we already have the analysis we need, I think that makes the case stronger for accepting this. At any rate, it's probably good to loop in T-Lang.

@rustbot label +AsyncAwait-Triaged

[joshtriplett]

Seems fine to me from a lang perspective. I do think this needs a lang FCP for the behavior change, though.

[joshtriplett]

@rfcbot merge

[rfcbot]

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

• @cramertj
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm

Concerns:

• early-storage-dead-of-stack-value (Holding a non-Send Copy type across a yield should be allowed #99104 (comment))
• needs-exact-proposed-rule (Holding a non-Send Copy type across a yield should be allowed #99104 (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.

[scottmcm]

What's the definition of "holding" here? Since there's no use after the await, I'm unsure what's complaining.

EDIT: Oh, I guess this is Copy as a marker for "promises not to need any Dropping later"?

[joshtriplett]

Confirming something: this is using Copy as a proxy for "definitely not Drop", right? This is not actually proposing to copy the value?

(If we were copying the value, we should have the lint about large copies. But if we're not copying the value, that's not an issue.)

[joshtriplett]

@jyn514 Could you retitle this issue and update the summary to make it clearer that this is only for not actually holding the type, and doesn't work if you actually use it after the await?

[jyn514]

EDIT: Oh, I guess this is Copy as a marker for "promises not to need any Dropping later"?

Yes, exactly. Theoretically we could do this for types like

enum TrivialDrop {
 A, B, C
}

impl !Sync for TrivialDrop {}

too, but haven't thought that far ahead.

[scottmcm]

I think the explicit promise is important for semver. Especially if you have a type with private fields, async code shouldn't stop compiling because you decided to add a String field where there wasn't one before.

[scottmcm]

@rfcbot concern needs-exact-proposed-rule

I think this needs a precise statement of when something is "held" across things, how that's detected, why it'll be reliably consistent across changes in rustc, etc.

[tmandry]

Especially if you have a type with private fields, async code shouldn't stop compiling because you decided to add a String field where there wasn't one before.

I agree, but will also note that we already do this with auto traits.

Side note: In the future it would be good to have a mechanism for promising to uphold auto traits, so semver breakage is more easily caught. We could then add a lint for public types that don't explicitly say whether they implement Send.

[scottmcm]

that we already do this with auto traits.

That's true, but also a very special exception. Copy could have been an auto trait, but it intentionally isn't. And we also don't treat Copy specially in, for example -> impl Trait -- you have to opt-in for -> impl Trait + Copy for the caller to see that.

[cramertj]

+1 to the previous comments-- I think the rule here winds up resembling NLL in that it's important whether or not the value is actually used after the yield.

[cramertj]

@rfcbot concern early-storage-dead-of-stack-value

After some further thought, I think that allowing this is not any easier than solving the more general problem of early stack re-use / StorageDead marking. In fact, it is somewhat more challenging in that the resulting rule would be directly observable from safe Rust code. Consider this example:

async fn foo() {
    let x = CreateMyNonSendCopyType();
    let y = &x as *const _;
    leak_ptr_to_somewhere(y);
    some_future.await();
    ... // some other code that doesn't reference `x` or `y`
}

In this case, I believe we have to continue storing x after the await, since its location on the stack may be referenced by other (unsafe, but plausibly sound) code outside the body of foo. In cases where x never has its address observed or leaked, this may not be necessary, but we I don't believe we yet have a working definition for when this kind of early-release of a value is possible.

[tmandry]

This builds on the generator drop tracking work, which makes it so you don't have to declare bindings in a nested scope anymore to satisfy the type checker. It does this by approximating some MIR analysis in HIR, but doesn't work at all for Copy types.

For generator layouts we check if a reference to a local has been taken before an await point. If it was we never reuse its storage. One approach is to recreate that (pretty simplistic) check in HIR. In addition to affecting the size of a generator struct, it would now be observable in the trait system.

The main downside I see is that the "was ever borrowed" check is conservative in a way that isn't intuitive – see #62321 for an example. Creating a temporary borrow and then dropping it, or to passing it to a function that doesn't capture the address, means that your local is now captured by your generator and impacts its auto traits. We could perhaps fix the first case easily, the second one not so much.¹

async fn foo(x: *const usize) {
    let _ = self.hash_map.get(&x);  // Won't work
    bar().await;
}

Your only workaround will be to copy the value to a helper function and do the borrow there:

async fn foo(x: *const usize) {
    let _ = self.get_from_hash_map(x);
    bar().await;
}

We're going to need compiler diagnostics to help you out in any case.

The question, for me, is whether making it less likely that you hit these issues (without eliminating them entirely) is worth the complexity of extra rules like this.

Footnotes

1. Along more speculative lines, we could try to declare that dereferencing such pointers is UB, but I think generators are already enough of a headache for UCG :-). I also think we could prove that your function doesn't capture a usable pointer if it's const fn (doesn't access globals) and some kind of provenance tracking. ↩

[nikomatsakis]

@rfcbot fcp cancel

Given that this RFC has been pending since August and there is still a need for more details, I'm going to cancel it for now, we can re-start when there's a concrete proposal we want to move forward.

[rfcbot]

@nikomatsakis proposal cancelled.