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

Implement for<> lifetime binder for closures #98705

Merged
merged 12 commits into from
Jul 14, 2022

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Jun 30, 2022

This PR implements RFC 3216 (TI) and allows code like the following:

let _f = for<'a, 'b> |a: &'a A, b: &'b B| -> &'b C { b.c(a) };
//       ^^^^^^^^^^^--- new!

cc @Aaron1011 @cjgillot

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 30, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2022
@rust-log-analyzer

This comment has been minimized.

Comment on lines 7 to 8
LL | for<'a, 'b> |_: &'a ()| {};
| ^^ undeclared lifetime
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @cjgillot even without an impl in ast_lowering, shouldn't this get picked up since you've moved name resolution earlier? Or is that still WIP?

@cjgillot cjgillot self-assigned this Jun 30, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jun 30, 2022

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

Honestly I have no idea why CI fails, the rustc_ast::Expr::presedence clearly exists

Omg, I just noticed

// `Expr` is used a lot. Make sure it doesn't unintentionally get bigger.
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
//rustc_data_structures::static_assert_size!(Expr, 104); // FIXME

impl Expr {

the cfg applies to the impl block now

@rust-log-analyzer

This comment has been minimized.

@ytmimi
Copy link
Contributor

ytmimi commented Jun 30, 2022

@WaffleLapkin when you get a chance could you add a formatting test case for bound closure to rustfmt/tests/target/closure.rs

Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks @WaffleLapkin. I left a few comments.

compiler/rustc_ast/src/visit.rs Outdated Show resolved Hide resolved
@@ -1931,6 +1931,7 @@ pub enum ExprKind<'hir> {
/// This may also be a generator literal or an `async block` as indicated by the
/// `Option<Movability>`.
Closure {
binder: &'hir ClosureBinder<'hir>,
capture_clause: CaptureBy,
bound_generic_params: &'hir [GenericParam<'hir>],
Copy link
Contributor

Choose a reason for hiding this comment

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

This field also holds generic parameters introduced by lifetime resolution. Can this and binder be merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would require to distinguish somewhere the presence of the for<> in code vs just in HIR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I'm not sure. This can probably be binder: ClosureBinder { Present, NotPresent } and all lifetimes stored in bound_generic_params, but this makes it impossible to distinguish lifetimes from binder from other lifetimes... No idea if we really need to distinguish them though

Copy link
Contributor

Choose a reason for hiding this comment

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

bound_generic_params is only filled by anonymous lifetimes, that you explicitly ban. So I don't think you need to worry about separating both.
Furthermore, if we ever allow anonymous lifetimes with a for<>, we will want them to use the for<> semantics and not the default/legacy one, won't we?

Copy link
Member Author

Choose a reason for hiding this comment

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

So I've tried this in 27905df77fd6dd9ad6d73b7820ff3d020ec2e7b3 and... in my humble opinion the code turned out to be worse.

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
}
}

pub(crate) fn suggestion(&self, sugg: &str) -> String {
match self {
Self::BoundEmpty | Self::TypeEmpty => format!("for<{}> ", sugg),
Self::BoundTail | Self::TypeTail => format!(", {}", sugg),
Self::ClosureEmpty => format!("for<{}>", sugg),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you merge this arm with the previous one?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't have a trailing space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to skip the trailing whitespace? I couldn't find a ui test suggesting to introduce a closure for bound.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we only suggest for<'lifetime> when there is already for<>. So we suggest replacing for<> with for<'lifetime>. In cases of BoundEmpty and TypeEmpty this is used when there is no for<> at all, so in these cases it makes sense to add a trailing space.

However at closer inspection... I don't think this is ever used. This variant is added to missing_named_lifetime_spots which is used only in add_missing_lifetime_specifiers_label that is only used in resolve_elided_lifetimes which, IIRC, is not called for closures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll see what I will do when moving this diagnostic to AST resolution.

compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_ast_lowering/src/expr.rs Outdated Show resolved Hide resolved
@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2022
Comment on lines -1346 to -1350
// NOTE(Centril, eddyb): DO NOT REMOVE! Beyond providing parser recovery,
// this is an insurance policy in case we allow qpaths in (tuple-)struct patterns.
// When `for <Foo as Bar>::Proj in $expr $block` is wanted,
// you can disambiguate in favor of a pattern with `(...)`.
self.recover_quantified_closure_expr(attrs)
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @eddyb is that ok to replace recovering with parsing closures?

@oli-obk oli-obk removed their assignment Jul 4, 2022
@bors
Copy link
Contributor

bors commented Jul 5, 2022

☔ The latest upstream changes (presumably #98584) made this pull request unmergeable. Please resolve the merge conflicts.

@WaffleLapkin
Copy link
Member Author

I think this is ready for the next round of review

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 11, 2022
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks @WaffleLapkin. I think this looks very good. AFAICT, there is just your 2 FIXMEs to handle.

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved
@@ -3479,7 +3491,7 @@ impl<'hir> Node<'hir> {
#[cfg(all(target_arch = "x86_64", target_pointer_width = "64"))]
mod size_asserts {
rustc_data_structures::static_assert_size!(super::Block<'static>, 48);
rustc_data_structures::static_assert_size!(super::Expr<'static>, 64);
//rustc_data_structures::static_assert_size!(super::Expr<'static>, 64); // FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, should we box the Closure variant?

Copy link
Member Author

@WaffleLapkin WaffleLapkin Jul 11, 2022

Choose a reason for hiding this comment

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

Like so

    // ExprKind
    Closure(&'hir Closure<'hir>),
struct Closure<'hir> {
    binder: &'hir ClosureBinder,
    capture_clause: CaptureBy,
    bound_generic_params: &'hir [GenericParam<'hir>],
    fn_decl: &'hir FnDecl<'hir>,
    body: BodyId,
    fn_decl_span: Span,
    movability: Option<Movability>,
}

?

Maybe, I'm not sure.

compiler/rustc_hir/src/intravisit.rs Outdated Show resolved Hide resolved
@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2022

Some changes occurred in need_type_info.rs

cc @lcnr

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member Author

@bors r=cjgillot

@bors
Copy link
Contributor

bors commented Jul 12, 2022

@WaffleLapkin: 🔑 Insufficient privileges: Not in reviewers

@cjgillot
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 13, 2022

📌 Commit 9aa142b has been approved by cjgillot

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 Jul 13, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 14, 2022
…llot

Implement `for<>` lifetime binder for closures

This PR implements RFC 3216 ([TI](rust-lang#97362)) and allows code like the following:

```rust
let _f = for<'a, 'b> |a: &'a A, b: &'b B| -> &'b C { b.c(a) };
//       ^^^^^^^^^^^--- new!
```

cc `@Aaron1011` `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 14, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97720 (Always create elided lifetime parameters for functions)
 - rust-lang#98315 (Stabilize `core::ffi:c_*` and rexport in `std::ffi`)
 - rust-lang#98705 (Implement `for<>` lifetime binder for closures)
 - rust-lang#99126 (remove allow(rustc::potential_query_instability) in rustc_span)
 - rust-lang#99139 (Give a better error when `x dist` fails for an optional tool)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e5a86d7 into rust-lang:master Jul 14, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 14, 2022
@WaffleLapkin WaffleLapkin deleted the closure_binder branch July 15, 2022 07:35
@WaffleLapkin
Copy link
Member Author

Can someone update the tracking issue now, tick the implementation and link this pr?

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
…llot

Implement `for<>` lifetime binder for closures

This PR implements RFC 3216 ([TI](rust-lang#97362)) and allows code like the following:

```rust
let _f = for<'a, 'b> |a: &'a A, b: &'b B| -> &'b C { b.c(a) };
//       ^^^^^^^^^^^--- new!
```

cc ``@Aaron1011`` ``@cjgillot``
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 18, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97720 (Always create elided lifetime parameters for functions)
 - rust-lang#98315 (Stabilize `core::ffi:c_*` and rexport in `std::ffi`)
 - rust-lang#98705 (Implement `for<>` lifetime binder for closures)
 - rust-lang#99126 (remove allow(rustc::potential_query_instability) in rustc_span)
 - rust-lang#99139 (Give a better error when `x dist` fails for an optional tool)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants