Lint on tail expr drop order change in Edition 2024 #128662

dingxiangfei2009 · 2024-08-04T19:59:59Z

This lint warns users to consider extra discretion on the effect of a transposed drop order arising from Edition 2024, which involves temporaries in tail expression location with significant drop implementation.

cc @traviscross

Tracking:

Tracking Issue for RFC 3606: Drop temporaries in tail expressions before local variables #123739

rustbot · 2024-08-04T20:00:07Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

compiler/rustc_lint/src/tail_expr_drop_order.rs

traviscross · 2024-08-13T19:46:47Z

r? compiler

This is an edition item.

jieyouxu · 2024-08-14T13:36:46Z

Thanks for the PR. I'm taking a look at this now, but I'm going to read back some of the context first.

jieyouxu

The implementation looks good to me in general. I have some discussions / questions and suggestions (for doc clarity and test coverage).

compiler/rustc_lint/messages.ftl

jieyouxu · 2024-08-14T18:41:02Z

compiler/rustc_lint/src/tail_expr_drop_order.rs

+    /// 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.
+    /// In case there are also local variables of type with significant `Drop` implementation as well,
+    /// this lint warns you of a potential transposition in the drop order.
+    /// Your discretion on the new drop order introduced by Edition 2024 is required.


Discussion: this wording is a bit unclear to me. Maybe something to the effect of:

The tail_expr_drop_order lint looks for potential transposition of the drop order before and after Edition 2024 between:

values whose types have significant Drop implementations, which are generated at tail expression positions, and

local variables whose types have significant Drop implementation as well.

The transposition of drop order introduced in Edition 2024 warrants your attention,
as it may change observable behavior and thus your intention.

(Or some better wording to that effect)

I think we also want to clarify or define what "significant Drop implementation" means here as well, does this mean "Drop implementation with side effects", or is there a more technically accurate description that would align with e.g. the reference?

AFAICT, the "significant" terminology comes from the internal has_significant_drop method on Ty (please correct me if this is well-defined elsewhere like in the reference), but it's not super obvious what this means even looking at the impl in rustc_middle, let alone for regular users.

(This is not a blocking concern, we can always fine-tune the wording later.)

compiler/rustc_lint/src/tail_expr_drop_order.rs

tests/ui/drop/lint-tail-expr-drop-order.rs

jieyouxu · 2024-08-14T19:18:38Z

@rustbot author

compiler/rustc_lint/src/tail_expr_drop_order.rs

tests/ui/drop/lint-tail-expr-drop-order.rs

jieyouxu · 2024-08-20T12:29:53Z

Thanks, the rest looks good to me now, just the edition/feature double-gating that needs to be addressed.

jieyouxu

The rest looks good to me, but I just realized one thing: we should add an ui test with three revisions for negative gating checks //@ revisions: neither no_feature_gate edition_less_than_2024:

neither: //@[neither] edition: 2021 and #![cfg_attr(neither, shorter_tail_lifetimes)]; check lint does not fire
no_feature_gate: //@[no_feature_gate] edition: 2024; check lint does not fire
edition_less_than_2024: //@[edition_less_than_2024] edition: 2021 and #![cfg_attr(edition_less_than_2024, shorter_tail_lifetimes)]; check lint does not fire

tests/ui/drop/lint-tail-expr-drop-order-gated.rs

jieyouxu

Thanks for the updates! Let me double-check if we're missing anything from previous review comments, then we can send this off to the merge queue ^^

jieyouxu · 2024-08-20T17:29:33Z

Ok this LGTM now, r=me once PR CI is green.

jieyouxu · 2024-08-20T18:15:06Z

@bors r+

bors · 2024-08-20T18:15:09Z

📌 Commit ef25fbd has been approved by jieyouxu

It is now in the queue for this repository.

jieyouxu · 2024-08-20T18:15:31Z

(Not tagging relnotes as I expect this to be announced alongside Edition 2024.)

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#128662 (Lint on tail expr drop order change in Edition 2024) - rust-lang#128932 (skip updating when external binding is existed) - rust-lang#129270 (Don't consider locals to shadow inner items' generics) - rust-lang#129277 (Update annotate-snippets to 0.11) - rust-lang#129294 (Stabilize `iter::repeat_n`) - rust-lang#129308 (fix: simple typo in compiler directory) - rust-lang#129309 (ctfe: make CompileTimeInterpCx type alias public) - rust-lang#129314 (fix a broken link in `mir/mod.rs`) - rust-lang#129318 (Remove unneeded conversion to `DefId` for `ExtraInfo`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128662 - dingxiangfei2009:lint-tail-expr-drop-order, r=jieyouxu Lint on tail expr drop order change in Edition 2024 This lint warns users to consider extra discretion on the effect of a transposed drop order arising from Edition 2024, which involves temporaries in tail expression location with significant drop implementation. cc `@traviscross` Tracking: - rust-lang#123739

rustbot assigned TaKO8Ki Aug 4, 2024

rustbot added S-waiting-on-review T-compiler labels Aug 4, 2024

dingxiangfei2009 mentioned this pull request Aug 4, 2024

Lint against possible change in drop order from tail expr rust-lang/rust-clippy#13175

Closed

dingxiangfei2009 force-pushed the lint-tail-expr-drop-order branch from 7bf7921 to 9441d83 Compare August 4, 2024 20:33

traviscross mentioned this pull request Aug 4, 2024

Tracking Issue for RFC 3606: Drop temporaries in tail expressions before local variables #123739

Closed

13 tasks