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

AsyncDestructor: replace fields with impl_did #139348

Merged
merged 1 commit into from
Apr 5, 2025

Conversation

meithecatte
Copy link
Contributor

The future and ctor fields aren't actually used, and the way they are extracted is obviously wrong – swapping the order of the items in the source code will give wrong results.

Instead, store just the LocalDefId of the impl, which is enough for the only use of this data.

The future and ctor fields aren't actually used, and the way they are
extracted is obviously wrong – swapping the order of the items in the
source code will give wrong results.

Instead, store just the LocalDefId of the impl, which is enough for the
only use of this data.
@rustbot
Copy link
Collaborator

rustbot commented Apr 4, 2025

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 4, 2025
@meithecatte
Copy link
Contributor Author

#126482

@rustbot label +F-async_drop

@rustbot rustbot added the F-async_drop Async drop label Apr 4, 2025
@petrochenkov
Copy link
Contributor

cc @zetanumbers

@zetanumbers
Copy link
Contributor

zetanumbers commented Apr 4, 2025

The future and ctor fields aren't actually used, and the way they are extracted is obviously wrong – swapping the order of the items in the source code will give wrong results.

Although I should notice such an error easily, could you please provide failing code sample? Adding a test case would be the best.

@meithecatte
Copy link
Contributor Author

There is no failing sample, because you never use these fields for anything. However:

  1. Add a debug!(?future, ?ctor);.
  2. Write the simplest possible AsyncDrop impl:
#![feature(async_drop)]
use std::future::{AsyncDrop, Ready};
use std::pin::Pin;

struct Foo;

impl AsyncDrop for Foo {
    type Dropper<'a> = Ready<()>;

    fn async_drop(self: Pin<&mut Self>) -> Self::Dropper<'_> {
        todo!()
    }
}
  1. Observe that the output of the debug! statement makes sense.
  2. Swap the two associated items as follows:
impl AsyncDrop for Foo {
    fn async_drop(self: Pin<&mut Self>) -> Self::Dropper<'_> {
        todo!()
    }

    type Dropper<'a> = Ready<()>;
}
  1. Observe that the output of the debug! statement is now wrong.

@zetanumbers
Copy link
Contributor

Holy! Now I remember. Thanks for fixing it. LGTM @petrochenkov

@petrochenkov
Copy link
Contributor

r? @petrochenkov
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 4, 2025

📌 Commit a2618e1 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#139041 (Remove `rustc_middle::ty::util::ExplicitSelf`.)
 - rust-lang#139328 (Fix 2024 edition doctest panic output)
 - rust-lang#139339 (unstable book: document tait)
 - rust-lang#139348 (AsyncDestructor: replace fields with impl_did)
 - rust-lang#139353 (Fix `Debug` impl for `LateParamRegionKind`.)
 - rust-lang#139366 (ToSocketAddrs: fix typo)
 - rust-lang#139374 (Use the span of the whole bound when the diagnostic talks about a bound)
 - rust-lang#139378 (Use target-agnostic LLD flags in bootstrap for `use-lld`)
 - rust-lang#139384 (Add `compiletest` adhoc_group for `r? compiletest`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d61a473 into rust-lang:master Apr 5, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 5, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2025
Rollup merge of rust-lang#139348 - meithecatte:async-destructor-minify, r=petrochenkov

AsyncDestructor: replace fields with impl_did

The future and ctor fields aren't actually used, and the way they are extracted is obviously wrong – swapping the order of the items in the source code will give wrong results.

Instead, store just the LocalDefId of the impl, which is enough for the only use of this data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-async_drop Async drop S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants