Implement `for<>` lifetime binder for closures #98705

WaffleLapkin · 2022-06-30T09:36:27Z

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 · 2022-06-30T09:36:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rust-highfive · 2022-06-30T09:36:32Z

r? @oli-obk

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

oli-obk · 2022-06-30T09:41:19Z

src/test/ui/feature-gates/feature-gate-closure_lifetime_binder.stderr

+LL |     for<'a, 'b> |_: &'a ()| {};
+   |                      ^^ undeclared lifetime


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?

rustbot · 2022-06-30T13:43:06Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

WaffleLapkin · 2022-06-30T16:01:02Z

~~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

ytmimi · 2022-06-30T17:34:30Z

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

cjgillot

Thanks @WaffleLapkin. I left a few comments.

compiler/rustc_ast/src/visit.rs

cjgillot · 2022-06-30T17:22:16Z

compiler/rustc_hir/src/hir.rs

@@ -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>],


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

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

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

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?

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

cjgillot · 2022-06-30T17:25:20Z

compiler/rustc_resolve/src/late/diagnostics.rs

        }
    }

    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),


Could you merge this arm with the previous one?

No, it doesn't have a trailing space.

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

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.

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

compiler/rustc_hir/src/hir.rs

compiler/rustc_ast/src/ast.rs

compiler/rustc_ast_lowering/src/expr.rs

WaffleLapkin · 2022-07-03T16:00:21Z

compiler/rustc_parse/src/parser/expr.rs

-                // 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)


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

bors · 2022-07-05T18:29:36Z

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

WaffleLapkin · 2022-07-11T08:32:54Z

I think this is ready for the next round of review

@rustbot ready

cjgillot

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

compiler/rustc_ast/src/ast.rs

cjgillot · 2022-07-11T09:23:47Z

compiler/rustc_hir/src/hir.rs

@@ -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


Likewise, should we box the Closure variant?

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

rustbot · 2022-07-11T19:44:38Z

Some changes occurred in need_type_info.rs

cc @lcnr

This helps bring `hir::Expr` size down, `Closure` was the biggest variant, especially after `for<>` additions.

WaffleLapkin · 2022-07-12T17:42:42Z

@bors r=cjgillot

bors · 2022-07-12T17:42:43Z

@WaffleLapkin: 🔑 Insufficient privileges: Not in reviewers

cjgillot · 2022-07-13T09:10:57Z

@bors r+

bors · 2022-07-13T09:10:59Z

📌 Commit 9aa142b has been approved by cjgillot

It is now in the queue for this repository.

…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`

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

WaffleLapkin · 2022-07-15T07:37:14Z

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

…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``

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

rust-highfive assigned oli-obk Jun 30, 2022

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

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

This comment has been minimized.

Sign in to view

oli-obk reviewed Jun 30, 2022

View reviewed changes

cjgillot self-assigned this Jun 30, 2022

This comment has been minimized.

Sign in to view

WaffleLapkin force-pushed the closure_binder branch from fd56fff to c68cd2f Compare June 30, 2022 15:42

This comment has been minimized.

Sign in to view

cjgillot reviewed Jun 30, 2022

View reviewed changes

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

WaffleLapkin commented Jul 3, 2022

View reviewed changes

oli-obk removed their assignment Jul 4, 2022

WaffleLapkin force-pushed the closure_binder branch from b8b1efd to bc608ac Compare July 8, 2022 18:21

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

cjgillot reviewed Jul 11, 2022

View reviewed changes

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

This comment has been minimized.

Sign in to view

WaffleLapkin added 8 commits July 12, 2022 21:00

make for<> in closures a possible place to suggest adding named lifetime

0c28484

add test for implicit stuff in signatures of closures with for<>

577f3c6

Add LifetimeBinderKind::Closure

3ebb852

Add an indirection for closures in hir::ExprKind

df4fee9

This helps bring `hir::Expr` size down, `Closure` was the biggest variant, especially after `for<>` additions.

Add back expr size checks

d2923b4

implement rustfmt formatting for for<> closure binders

b504a18

Add rustfmt test for formatting for<> before closures

30a3673

Fix clippy build

9aa142b

WaffleLapkin force-pushed the closure_binder branch from c7b33a3 to 9aa142b Compare July 12, 2022 17:06

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 mentioned this pull request Jul 14, 2022

Rollup of 5 pull requests #99229

Closed

Dylan-DPC mentioned this pull request Jul 14, 2022

Rollup of 5 pull requests #99231

Merged

bors merged commit e5a86d7 into rust-lang:master Jul 14, 2022

rustbot added this to the 1.64.0 milestone Jul 14, 2022

WaffleLapkin deleted the closure_binder branch July 15, 2022 07:35

lqd mentioned this pull request Jul 15, 2022

Regression when compiling with 07-14 nightly #99276

Closed

fmease mentioned this pull request Jul 21, 2022

Tracking Issue for RFC 3216: "Allow using for<'a> syntax when declaring closures" #97362

Open

3 tasks

jruderman mentioned this pull request Oct 30, 2022

ICE normalizing binder for closure #103771

Closed

jinohkang-theori mentioned this pull request Apr 13, 2023

Annotating higher-ranked lifetimes on closures is arduous #58052

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `for<>` lifetime binder for closures #98705

Implement `for<>` lifetime binder for closures #98705

WaffleLapkin commented Jun 30, 2022 •

edited

Loading

rustbot commented Jun 30, 2022

rust-highfive commented Jun 30, 2022

This comment has been minimized.

oli-obk Jun 30, 2022

rustbot commented Jun 30, 2022

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented Jun 30, 2022

This comment has been minimized.

ytmimi commented Jun 30, 2022 •

edited

Loading

cjgillot left a comment

cjgillot Jun 30, 2022

cjgillot Jun 30, 2022

WaffleLapkin Jun 30, 2022

cjgillot Jul 1, 2022

WaffleLapkin Jul 3, 2022

cjgillot Jun 30, 2022

WaffleLapkin Jun 30, 2022

cjgillot Jul 1, 2022

WaffleLapkin Jul 3, 2022

cjgillot Jul 11, 2022

WaffleLapkin Jul 3, 2022

bors commented Jul 5, 2022

WaffleLapkin commented Jul 11, 2022

cjgillot left a comment

cjgillot Jul 11, 2022

WaffleLapkin Jul 11, 2022 •

edited

Loading

rustbot commented Jul 11, 2022

This comment has been minimized.

WaffleLapkin commented Jul 12, 2022

bors commented Jul 12, 2022

cjgillot commented Jul 13, 2022

bors commented Jul 13, 2022

WaffleLapkin commented Jul 15, 2022

Implement for<> lifetime binder for closures #98705

Implement for<> lifetime binder for closures #98705

Conversation

WaffleLapkin commented Jun 30, 2022 • edited Loading

rustbot commented Jun 30, 2022

rust-highfive commented Jun 30, 2022

This comment has been minimized.

Choose a reason for hiding this comment

rustbot commented Jun 30, 2022

This comment has been minimized.

This comment has been minimized.

WaffleLapkin commented Jun 30, 2022

This comment has been minimized.

ytmimi commented Jun 30, 2022 • edited Loading

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 5, 2022

WaffleLapkin commented Jul 11, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaffleLapkin Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

rustbot commented Jul 11, 2022

This comment has been minimized.

WaffleLapkin commented Jul 12, 2022

bors commented Jul 12, 2022

cjgillot commented Jul 13, 2022

bors commented Jul 13, 2022

WaffleLapkin commented Jul 15, 2022

Implement `for<>` lifetime binder for closures #98705

Implement `for<>` lifetime binder for closures #98705

WaffleLapkin commented Jun 30, 2022 •

edited

Loading

ytmimi commented Jun 30, 2022 •

edited

Loading

WaffleLapkin Jul 11, 2022 •

edited

Loading