-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Stabilize shorter-tail-lifetimes #131983
Stabilize shorter-tail-lifetimes #131983
Conversation
@rustbot labels +A-edition-2024 |
@@ -21,8 +21,7 @@ declare_lint! { | |||
/// Your discretion on the new drop order introduced by Edition 2024 is required. | |||
/// | |||
/// ### Example | |||
/// ```rust,edition2024 | |||
/// #![feature(shorter_tail_lifetimes)] | |||
/// ```rust,edition2021 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing, but "The tail_expr_drop_order
lint looks for those values generated at the tail expression location, that of type with a significant Drop
implementation, such as locks." seems wrong to me.
/// The `tail_expr_drop_order` lint looks for those values generated at the tail expression location,
/// whose type has a significant `Drop` implementation, such as locks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tweaked the wording for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're also flipping the editions for which the lint fires from only linting in edition 2024+ to instead only lint in older editions.
Linting in editions <2024 seems correct to me, as the lint is supposed to warn of changes to drop order when switching to edition 2024. Do I understand this correctly?
@@ -185,7 +184,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LintVisitor<'a, 'tcx> { | |||
|
|||
impl<'a, 'tcx> LintVisitor<'a, 'tcx> { | |||
fn check_block_inner(&mut self, block: &Block<'tcx>) { | |||
if !block.span.at_least_rust_2024() { | |||
if block.span.at_least_rust_2024() { | |||
// We only lint for Edition 2024 onwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated
@@ -1,15 +1,8 @@ | |||
// This test ensures that `tail_expr_drop_order` does not activate in case Edition 2024 is not used | |||
// or the feature gate `shorter_tail_lifetimes` is disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that you're removing the reasoning behind this test, shouldn't the whole test be removed entirely? alternatively keep this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nits, then r=me cc @nikomatsakis
27b60a8
to
d51b702
Compare
Yes, it was not corrected even after learning that we should fire the lint for code targeting Edition 2021 and before. It fell through the crack somehow. Let me check the comments again to see if there is any outdated wording. |
@@ -361,6 +361,8 @@ declare_features! ( | |||
(accepted, self_in_typedefs, "1.32.0", Some(49303)), | |||
/// Allows `Self` struct constructor (RFC 2302). | |||
(accepted, self_struct_ctor, "1.32.0", Some(51994)), | |||
/// Shortern the tail expression lifetime | |||
(accepted, shorter_tail_lifetimes, "1.79.0", Some(123739)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be CURRENT_RUSTC_VERSION
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
@bors r+ rollup |
…-tail-lifetimes, r=lcnr Stabilize shorter-tail-lifetimes Close rust-lang#131445 Tracked by rust-lang#123739 We found a test case `tests/ui/drop/drop_order.rs` that had not been covered by the change. The test fixture is fixed now with the correct expectation.
☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts. |
43803df
to
0689b21
Compare
@bors r+ |
…iaskrgr Rollup of 4 pull requests Successful merges: - rust-lang#131790 (Document textual format of SocketAddrV{4,6}) - rust-lang#131983 (Stabilize shorter-tail-lifetimes) - rust-lang#132097 (sanitizer.md: LeakSanitizer is not supported on aarch64 macOS) - rust-lang#132107 (Remove visit_expr_post from ast Visitor) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131983 - dingxiangfei2009:stabilize-shorter-tail-lifetimes, r=lcnr Stabilize shorter-tail-lifetimes Close rust-lang#131445 Tracked by rust-lang#123739 We found a test case `tests/ui/drop/drop_order.rs` that had not been covered by the change. The test fixture is fixed now with the correct expectation.
Close #131445
Tracked by #123739
We found a test case
tests/ui/drop/drop_order.rs
that had not been covered by the change. The test fixture is fixed now with the correct expectation.