rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation56 Commits12 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Agrailag, Virgiel, nanoqsh, nicklauri, zohnannor, purpleposeidon, andreytkachenko, GrayJack, MartinKavik, zotho, and 2 more reacted with thumbs up emoji ivan770, GoldsteinE, Aaron1011, yoshuawuyts, wwylele, zohnannor, Noratrieb, Alexendoo, jyn514, danielhenrymantilla, and 20 more reacted with heart emoji Kobzol, slanterns, marmeladema, flip1995, taiki-e, yoshuawuyts, zohnannor, Noratrieb, jyn514, danielhenrymantilla, and 11 more reacted with rocket emoji

rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Jun 30, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

r? @oli-obk

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

This comment has been minimized.

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

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?

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

This comment has been minimized.

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

This comment has been minimized.

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

Choose a reason for hiding this comment

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

@@ -1931,6 +1931,7 @@ pub enum ExprKind<'hir> {
/// This may also be a generator literal or an `async block` as indicated by the
/// `Option`.
Closure {
binder: &'hir ClosureBinder<'hir>,
capture_clause: CaptureBy,
bound_generic_params: &'hir [GenericParam<'hir>],

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?

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.

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

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?

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.

}
}

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

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?

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.

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.

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.

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.

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 ::Proj in exprexpr exprblock` is wanted,
// you can disambiguate in favor of a pattern with `(...)`.
self.recover_quantified_closure_expr(attrs)

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?

I think this is ready for the next round of review

@rustbot ready

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

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.

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

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?

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

Maybe, I'm not sure.

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

Some changes occurred in need_type_info.rs

cc @lcnr

This comment has been minimized.

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

📌 Commit 9aa142b has been approved by cjgillot

It is now in the queue for this repository.

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

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

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

lqd mentioned this pull request

Jul 15, 2022

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

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

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.

Implement for<> lifetime binder for closures by WaffleLapkin · Pull Request #98705 · rust-lang/rust (original) (raw)

Conversation

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Implement `for<>` lifetime binder for closures by WaffleLapkin · Pull Request #98705 · rust-lang/rust (original) (raw)