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

Add new temporary lifetime feature gate and super let keyword #119043

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add new temporary lifetime feature gate and super let keyword #119043

wants to merge 3 commits into from
+829 −42

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Dec 17, 2023 •
edited

cc @m-ou-se @nikomatsakis

This is a test drive with the new temporary lifetime rule. With the #![feature(new_temp_lifetime)] gate enabled, the new rules kick in to override the old scope assignments; and you will gain access to super let.

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@cjgillot
Copy link
Contributor

I won't have time to review this PR.
r? compiler

@rustbot rustbot assigned wesleywiser and unassigned cjgillot Dec 17, 2023
flip1995
flip1995 reviewed Dec 20, 2023
View reviewed changes
src/tools/clippy/clippy_lints/src/loops/needless_range_loop.rs Outdated
Comment on lines 60 to 65
let var_scope = if cx.tcx.sess.at_least_rust_2024() && cx.tcx.features().new_temp_lifetime {
let scope_map = cx.tcx.body_scope_map(parent_def_id);
scope_map.var_scope_new(pat.hir_id.local_id)
} else {
region_scope_tree.var_scope(pat.hir_id.local_id)
};
Copy link
Member

Choose a reason for hiding this comment

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

This pattern is now appearing everywhere. Why not include this in the var_scope method itself?

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 have given this a thought. The idea is that I would like to keep the body_scope_map behind the feature gate and leave the original scope tree implementation untouched so that both can evolve independently.

@bors
Copy link
Contributor

bors commented Dec 26, 2023

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

@dingxiangfei2009
Copy link
Contributor Author

This has taken #119554 into account. I have decided to enclose the match guard in an intermediate scope, because the bindings will never escape into the arm body by the current match design.

flip1995
flip1995 reviewed Jan 11, 2024
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

I don't have any objections to the Clippy changes, as long as the reviewer for the compiler changes think that this is not a concern.

I'm not the right person to review the compiler changes though.

@oli-obk oli-obk self-assigned this Jan 18, 2024
oli-obk
oli-obk reviewed Jan 18, 2024
View reviewed changes
Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Please add some more tests. e.g. usage of super let in consts/statics. Usage at the top level (fn main() { super let x = 5; }, ...

compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated
Comment on lines 238 to 242
let temp_lifetime = if let Some(scope_map) = self.scope_map {
scope_map.temporary_scope_new(expr.hir_id.local_id)
} else {
self.rvalue_scopes.temporary_scope(self.region_scope_tree, expr.hir_id.local_id)
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a recurring pattern. Please add a helper for it.

compiler/rustc_parse/src/parser/stmt.rs
@@ -73,7 +73,14 @@ impl<'a> Parser<'a> {
}

Ok(Some(if self.token.is_keyword(kw::Let) {
self.parse_local_mk(lo, attrs, capture_semi, force_collect)?
self.parse_local_mk(lo, attrs, capture_semi, force_collect, false)?
} else if self.sess.edition.at_least_rust_2024()
Copy link
Contributor

Choose a reason for hiding this comment

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

why does this need edition gating?

@oli-obk oli-obk 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 Jan 29, 2024
@dingxiangfei2009
first version of super let syntax support
73259dc 
smoke tests of the new syntax

move new_temp_lifetime into query

restore classic scoping rules
@dingxiangfei2009
put up a facade so that features are tested earlier
a37cf51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

@flip1995 flip1995 flip1995 left review comments

Assignees

@oli-obk oli-obk

@wesleywiser wesleywiser

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@dingxiangfei2009 @rustbot @cjgillot @bors @rust-log-analyzer @oli-obk @flip1995 @wesleywiser