Skip to content

Mismatch between sync and async drop gen? #140600

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

Open
lovely-error opened this issue May 2, 2025 · 7 comments
Open

Mismatch between sync and async drop gen? #140600

lovely-error opened this issue May 2, 2025 · 7 comments
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-monomorphization Area: Monomorphization C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lovely-error
Copy link

I expect that the async version would panic at compile time the same way sync version does

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=4b0f956255c35cca27761291affcda87

@lovely-error lovely-error added the C-bug Category: This is a bug. label May 2, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 2, 2025
@lovely-error lovely-error changed the title Mismatch between between sync and async drop gen? Mismatch between sync and async drop gen? May 2, 2025
@workingjubilee
Copy link
Member

headscratch

...cc @azhogin? Sorry if this isn't in your bailiwick.

@moxian
Copy link
Contributor

moxian commented May 3, 2025

This seems to have little to do with async drop gen, and is more about const eval, and how it handles async (and monomorphization, and visibility.. I think..). Consider:

// assuming --crate-type=lib , i.e. this is src/lib.rs
struct CantMake<T>{inner: T}
impl<T> CantMake<T>{
    fn new(x: T) -> Self{
        const { panic!("wanted to make unmakable") }
    }
}
struct CantDrop<T>{inner: T}
impl<T> CantDrop<T>{
    fn new(x: T) -> Self{
        Self{ inner: x }
    }
}
impl<T> std::ops::Drop for CantDrop<T>{
    fn drop(&mut self){
        const { panic!("wanted to drop undroppable") }
    }
}

// compiles
fn sync_private() {
    let _a_sync_private = CantMake::new(0_u8);
    let _a_sync_private = CantDrop::new(1_u8);
}
// fails to compile
pub fn sync_public() {
    let _a_sync_public = CantMake::new(2_u8);
    //~^ ERROR: evaluation of `CantMake::<u8>::new::{constant#0}` failed
    let _b_sync_public = CantDrop::new(3_u8);
    //~^ ERROR: evaluation of `<CantDrop<u8> as std::ops::Drop>::drop::{constant#0}` failed
}
// compiles
pub async fn async_public() {
    let _a_async_public = CantMake::new(4_u8);
    let _b_async_public = CantDrop::new(4_u8);
}

Note that trying to actually invoke async_public like so:

#[tokio::main]
async fn main(){
    async_public().await;
}

does result in the const eval errors, for both constructor and drop.

(I don't know enough about const eval to speculate whether the status quo is desirable)

@rustbot label: +A-const-eval +A-async-await +T-compiler +A-monomorphization

@rustbot rustbot added A-async-await Area: Async & Await A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-monomorphization Area: Monomorphization labels May 3, 2025
@moxian
Copy link
Contributor

moxian commented May 3, 2025

See also: #111202 and (tangentially) #122814

@saethlin saethlin removed A-async-await Area: Async & Await needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 4, 2025
@saethlin
Copy link
Member

saethlin commented May 4, 2025

Talking about private/public is probably a distraction from the point of this issue. Same with optimizations.

The "I expected this to panic at compile time" program can be reduced to this:

#![crate_type = "lib"]

fn impossible<T>() {
    const { panic!() }
}
pub fn public() -> impl Fn() {
    || impossible::<u8>()
}

(remember that async fn doesn't execute the body when called, it returns a state machine that must be polled to execute the body)

MentionedItems only looks into terminators. The closure (or coroutine in the case of async) only appears in an assignment from an Rvalue::Use, so we never recurse into the body of the closure inside public to find const { panic!() }.

I don't know if the current behavior is intentional or desirable. Just clearing up that this has nothing to do with async.

@moxian
Copy link
Contributor

moxian commented May 5, 2025

I disagree though, this has a whole lot to do with async.

However, looking closer there are two different problems in a trenchcoat here. One about monomorphization (but not async), the second about async (but not monomorphization). I've split the latter into #140655 to keep the issue focused.
Thanks for the minimization of the mono part, this is great!

@saethlin
Copy link
Member

saethlin commented May 5, 2025

What bug do you think this is about that #140655 isn't? Add far as I can tell you have derailed this issue into a discussion about how visibility affects MonoItem collection, which was clearly not OP's intention.

@moxian
Copy link
Contributor

moxian commented May 5, 2025

What bug do you think this is about that #140655 isn't?

Const eval not being performed in polymorphic functions called inside closures, like in your example #140600 (comment). Which might have the same root cause #140655 with the current implementation, but is very different from a user perspective.
I can see a world where pub async fn foo() { const{ panic!() } } errors out, but your example in #140600 (comment) compiles.

Since you insist that this issue is not about async, I thought that splitting async parts out was the right choice

discussion about how visibility affects MonoItem collection, which was clearly not OP's intention.

The OP playground example compiles on playground with no errors if we rename fn main() to fn thing(), so difference in visibility was part of the original confusion, which is why I mentioned it in #140600 (comment)
But I did not intend to stress it overmuch and claim that this is the only thing going on here.

Apologies if this is being nonproductive...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-monomorphization Area: Monomorphization C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants