Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIR generation for generators considers any borrowed or assigned value as live across yield points #94067

Open
eholk opened this issue Feb 16, 2022 · 2 comments · May be fixed by #128846
Open
Labels
A-borrow-checker Area: The borrow checker A-coroutines Area: Coroutines A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@eholk
Copy link
Contributor

eholk commented Feb 16, 2022

Consider an example such as this:

impl Agent {
    async fn handle(&mut self) {
        let mut info = self.info_result.clone();
        info.node = None;
        let element = parse_info(info);
        let _ = send_element(element).await;
    }
}

fn parse_info(_: Info) -> Element { ... }

fn foo(agent: Agent) {
    assert_send(agent.handle());
    //~^ cannot be sent between threads safely
}

Let's assume info is !Send. The future returned from handle is currently !Send as well, because the MIR generation for the generator considers info to be live across the await point, despite the fact that info is consumed by parse_info. The reason is because the generator step explicitly considers any borrowed local as live across the suspend point.

With the more precise drop tracking in the type checker, it would be nice not to have to capture these at the MIR level. Currently drop tracking has to be more conservative than necessary in order to match the MIR behavior.

eholk added a commit to eholk/rust that referenced this issue Feb 16, 2022
This is needed to match MIR more conservative approximation of any
borrowed value being live across a suspend point (See rust-lang#94067). This
change considers an expression such as `x.y = z` to be a borrow of `x`
and therefore keeps `x` live across suspend points.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 25, 2022
…andry

Consider mutations as borrows in generator drop tracking

This is needed to match MIR more conservative approximation of any borrowed value being live across a suspend point (See rust-lang#94067). This change considers an expression such as `x.y = z` to be a borrow of `x` and therefore keeps `x` live across suspend points.

r? `@nikomatsakis`
@GoldsteinE
Copy link
Contributor

Also encountered this with Mutex guards

use std::sync::Mutex;

#[derive(Default)]
pub struct Foo {
    x: Mutex<usize>,
}

async fn foo() {}

impl Foo {
    pub async fn bar(&self) {
        let mut guard = self.x.lock().unwrap();
        *guard += 1;
        drop(guard);
        foo().await;
    }
}

fn assert_send(_: impl Send) {}

pub fn check(foo: &Foo) {
    assert_send(foo.bar());
}

@GoldsteinE
Copy link
Contributor

Actually, it seems to break even if local is never borrowed (playground)

struct NotSend(*const ());

async fn foo() {}

pub async fn bar() {
    let var = NotSend(&());
    drop(var);
    foo().await;
}

fn assert_send(_: impl Send) {}

pub fn check() {
    assert_send(bar());
}

@jyn514 jyn514 added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-borrow-checker Area: The borrow checker A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-coroutines Area: Coroutines labels Apr 26, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 4, 2023
Stop considering moved-out locals when computing auto traits for generators

Addresses rust-lang#94067 (but does not fix it since drop-tracking-mir is unstable)

This PR, unlike rust-lang#110420 or rust-lang#112050, does not attempt to reduce the number of live locals across suspension points. It however ignores moved-out locals for trait purposes. So this PR solves the non-send issue, but not the size issue.

Suggested by `@RalfJung` in [rust-lang/unsafe-code-guidelines#188](rust-lang/unsafe-code-guidelines#188 (comment))

cc `@b-naber` who's working on this from a different perspective.

r? `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 8, 2024
Stop considering moved-out locals when computing auto traits for generators (rebased)

This PR revives rust-lang#112279. I wanted to reopen this to gauge `@cjgillot's` thoughts about this, since it's been a while since `-Zdrop-tracking-mir` has landed.

If this PR looks OK, I can flesh it out and write up an FCP report -- I think this is a T-types FCP, since this has to do with the types that are considered live for auto traits, and doesn't otherwise affect the layout of coroutines.

Open questions:
* **"Why do we require storage to be live if the locals is not initialized?"** (rust-lang#112279 (comment))
    * I agree that we should eventually fix the storage analysis for coroutines, but this seems like a problem that can be fixed after we fix witnesses for *the purposes of traits* here.
    * As far as I could tell, fixing the problem of moved locals for *storage* would require insertion of additional `StorageDead` statements, or would require further entangling the type system with the operational semantics in ways that T-opsem seemed uncomfortable with [when I asked](https://rust-lang.zulipchat.com/#narrow/stream/189540-t-compiler.2Fwg-mir-opt/topic/Inserting.20.60StorageDrop.60.20after.20unconditional.20moves). cc `@RalfJung`
    * Relevant: rust-lang/unsafe-code-guidelines#188

Fixes rust-lang#128095
Fixes rust-lang#94067

r? `@cjgillot`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-coroutines Area: Coroutines A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
3 participants