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.

Sign up for GitHub

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

Jump to bottom

Reintroduce into_future in .await desugaring #90737

Merged
merged 5 commits into from
Dec 3, 2021
Merged

Reintroduce into_future in .await desugaring #90737

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dfa0db5
Reintroduce `into_future` in `.await` desugaring
eholk Nov 9, 2021
ca2057c
Fix comments
eholk Nov 11, 2021
db80c07
Update stderr files
eholk Nov 22, 2021
0cb7693
Code review feedback
eholk Dec 2, 2021
3268bf1
Update .stderr files
eholk Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions compiler/rustc_ast_lowering/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ impl<'hir> LoweringContext<'_, 'hir> {

/// Desugar `<expr>.await` into:
/// ```rust
/// match <expr> {
/// match ::std::future::IntoFuture::into_future(<expr>) {
/// mut pinned => loop {
/// match unsafe { ::std::future::Future::poll(
/// <::std::pin::Pin>::new_unchecked(&mut pinned),
Expand Down Expand Up @@ -629,7 +629,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
await_span,
self.allow_gen_future.clone(),
);
let expr = self.lower_expr(expr);
let expr = self.lower_expr_mut(expr);

let pinned_ident = Ident::with_dummy_span(sym::pinned);
let (pinned_pat, pinned_pat_hid) =
Expand Down Expand Up @@ -746,10 +746,26 @@ impl<'hir> LoweringContext<'_, 'hir> {
// mut pinned => loop { ... }
let pinned_arm = self.arm(pinned_pat, loop_expr);

// match <expr> {
// `match ::std::future::IntoFuture::into_future(<expr>) { ... }`
let into_future_span = self.mark_span_with_reason(
DesugaringKind::Await,
await_span,
self.allow_into_future.clone(),
);
let into_future_expr = self.expr_call_lang_item_fn(
into_future_span,
hir::LangItem::IntoFutureIntoFuture,
arena_vec![self; expr],
);

// match <into_future_expr> {
// mut pinned => loop { .. }
// }
hir::ExprKind::Match(expr, arena_vec![self; pinned_arm], hir::MatchSource::AwaitDesugar)
hir::ExprKind::Match(
into_future_expr,
arena_vec![self; pinned_arm],
hir::MatchSource::AwaitDesugar,
)
}

fn lower_expr_closure(
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_ast_lowering/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ struct LoweringContext<'a, 'hir: 'a> {

allow_try_trait: Option<Lrc<[Symbol]>>,
allow_gen_future: Option<Lrc<[Symbol]>>,
allow_into_future: Option<Lrc<[Symbol]>>,
}

pub trait ResolverAstLowering {
Expand Down Expand Up @@ -320,6 +321,7 @@ pub fn lower_crate<'a, 'hir>(
in_scope_lifetimes: Vec::new(),
allow_try_trait: Some([sym::try_trait_v2][..].into()),
allow_gen_future: Some([sym::gen_future][..].into()),
allow_into_future: Some([sym::into_future][..].into()),
}
.lower_crate(krate)
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/lang_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ language_item_table! {
ControlFlowContinue, sym::Continue, cf_continue_variant, Target::Variant, GenericRequirement::None;
ControlFlowBreak, sym::Break, cf_break_variant, Target::Variant, GenericRequirement::None;

IntoFutureIntoFuture, sym::into_future, into_future_fn, Target::Method(MethodKind::Trait { body: false }), GenericRequirement::None;
IntoIterIntoIter, sym::into_iter, into_iter_fn, Target::Method(MethodKind::Trait { body: false }), GenericRequirement::None;
IteratorNext, sym::next, next_fn, Target::Method(MethodKind::Trait { body: false}), GenericRequirement::None;

Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_span/src/symbol.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ symbols! {
Implied,
Input,
Into,
IntoFuture,
IntoIterator,
IoRead,
IoWrite,
Expand Down Expand Up @@ -734,6 +735,7 @@ symbols! {
inout,
instruction_set,
intel,
into_future,
into_iter,
intra_doc_pointers,
intrinsics,
Expand Down
1 change: 1 addition & 0 deletions library/core/src/future/into_future.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ pub trait IntoFuture {

/// Creates a future from a value.
#[unstable(feature = "into_future", issue = "67644")]
#[cfg_attr(not(bootstrap), lang = "into_future")]
fn into_future(self) -> Self::Future;
}

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/async-await/async-fn-size-moved-locals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ async fn mixed_sizes() {
fn main() {
assert_eq!(1025, std::mem::size_of_val(&single()));
assert_eq!(1026, std::mem::size_of_val(&single_with_noop()));
assert_eq!(3078, std::mem::size_of_val(&joined()));
assert_eq!(3079, std::mem::size_of_val(&joined_with_noop()));
assert_eq!(7181, std::mem::size_of_val(&mixed_sizes()));
assert_eq!(3076, std::mem::size_of_val(&joined()));
assert_eq!(3076, std::mem::size_of_val(&joined_with_noop()));
assert_eq!(6157, std::mem::size_of_val(&mixed_sizes()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unexpected! Do you know why these sizes change?

It looks like an improvement but I'd like to understand what's going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't sure either! The previous PR also shrunk the size of the generated futures, without really commenting on it either. It's particularly surprising because neither the numbers before or after this changed are aligned the way we expected them to.

We reasoned that this change would be alright since it doesn't regress, there is precedence for accepting this in #65244, and it doesn't lead to any other changes as far as we can tell. Even if we don't fully understand why the current results are what they are, or what is causing the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is right, but one guess is it has something to do with the match <future> { ... } becoming match InfoFuture::into_future(<future>) { ... }. Maybe the into_future call moves the future into the scrutinee position of the match, rather than borrowing from the outer scope?

Judging by the comment on this test, it seems like the first two changes aren't a big deal, since they are just a few bytes. The last change is surprising because it improves by exactly 1024. This means we were able to eliminate one of the BigFuts from the closure. By my count we should only need 5, so I'm not sure why we have 6 copies now or why we had 7 copies before. I wouldn't be surprised to see 8 or 5, I guess, but 6 and 7 are both weird. Could we be smarter about laying out the closures? Maybe there's a move from one slot to another that's forced by the way we laid out the closures?

Copy link
Member

@tmandry tmandry Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting theory, if we're turning a borrow into a move that can definitely improve things! See #62321 for instance – if I recall correctly, borrowing a local in a generator causes the analysis to assume the local is always live from that point, preventing it from reclaiming the storage.

}
28 changes: 28 additions & 0 deletions src/test/ui/async-await/await-into-future.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// check-pass
eholk marked this conversation as resolved.
Show resolved Hide resolved

// edition:2021

#![feature(into_future)]

use std::{future::{Future, IntoFuture}, pin::Pin};

struct AwaitMe;

impl IntoFuture for AwaitMe {
type Output = i32;
type Future = Pin<Box<dyn Future<Output = i32>>>;

fn into_future(self) -> Self::Future {
Box::pin(me())
}
}

async fn me() -> i32 {
41
}

async fn run() {
assert_eq!(AwaitMe.await, 41);
}

fn main() {}
1 change: 1 addition & 0 deletions src/test/ui/async-await/issue-70594.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ LL | [1; ().await];
| ^^^^^^^^ `()` is not a future
|
= help: the trait `Future` is not implemented for `()`
= note: required because of the requirements on the impl of `IntoFuture` for `()`
eholk marked this conversation as resolved.
Show resolved Hide resolved

error: aborting due to 4 previous errors

Expand Down
1 change: 1 addition & 0 deletions src/test/ui/async-await/issues/issue-62009-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ LL | (|_| 2333).await;
| ^^^^^^^^^^^^^^^^ `[closure@$DIR/issue-62009-1.rs:12:5: 12:15]` is not a future
|
= help: the trait `Future` is not implemented for `[closure@$DIR/issue-62009-1.rs:12:5: 12:15]`
= note: required because of the requirements on the impl of `IntoFuture` for `[closure@$DIR/issue-62009-1.rs:12:5: 12:15]`

error: aborting due to 4 previous errors

Expand Down
8 changes: 8 additions & 0 deletions src/test/ui/async-await/unresolved_type_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,20 @@ async fn foo() {
//~^ ERROR type inside `async fn` body must be known in this context
//~| ERROR type inside `async fn` body must be known in this context
//~| ERROR type inside `async fn` body must be known in this context
//~| ERROR type inside `async fn` body must be known in this context
//~| ERROR type inside `async fn` body must be known in this context
//~| NOTE cannot infer type for type parameter `T`
//~| NOTE cannot infer type for type parameter `T`
//~| NOTE cannot infer type for type parameter `T`
//~| NOTE cannot infer type for type parameter `T`
//~| NOTE cannot infer type for type parameter `T`
//~| NOTE the type is part of the `async fn` body because of this `await`
//~| NOTE the type is part of the `async fn` body because of this `await`
//~| NOTE the type is part of the `async fn` body because of this `await`
//~| NOTE the type is part of the `async fn` body because of this `await`
//~| NOTE the type is part of the `async fn` body because of this `await`
//~| NOTE in this expansion of desugaring of `await`
//~| NOTE in this expansion of desugaring of `await`
//~| NOTE in this expansion of desugaring of `await`
//~| NOTE in this expansion of desugaring of `await`
//~| NOTE in this expansion of desugaring of `await`
Expand Down
26 changes: 25 additions & 1 deletion src/test/ui/async-await/unresolved_type_param.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,30 @@ note: the type is part of the `async fn` body because of this `await`
LL | bar().await;
| ^^^^^^^^^^^

error: aborting due to 3 previous errors
error[E0698]: type inside `async fn` body must be known in this context
eholk marked this conversation as resolved.
Show resolved Hide resolved
--> $DIR/unresolved_type_param.rs:9:5
|
LL | bar().await;
| ^^^ cannot infer type for type parameter `T` declared on the function `bar`
|
note: the type is part of the `async fn` body because of this `await`
--> $DIR/unresolved_type_param.rs:9:5
|
LL | bar().await;
| ^^^^^^^^^^^

error[E0698]: type inside `async fn` body must be known in this context
--> $DIR/unresolved_type_param.rs:9:5
|
LL | bar().await;
| ^^^ cannot infer type for type parameter `T` declared on the function `bar`
|
note: the type is part of the `async fn` body because of this `await`
--> $DIR/unresolved_type_param.rs:9:5
|
LL | bar().await;
| ^^^^^^^^^^^

error: aborting due to 5 previous errors

For more information about this error, try `rustc --explain E0698`.