[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

wesleywiser · 2021-09-23T22:02:36Z

Currently, with the new 2021 edition, if a closure captures all of the
fields of an upvar, we'll drop those fields in the order they are used
within the closure instead of the normal drop order (the definition
order of the fields in the type).

This changes that so we sort the captured fields by the definition order
which causes them to drop in that same order as well.

Fixes rust-lang/project-rfc-2229#42

r? @nikomatsakis

Currently, with the new 2021 edition, if a closure captures all of the fields of an upvar, we'll drop those fields in the order they are used within the closure instead of the normal drop order (the definition order of the fields in the type). This changes that so we sort the captured fields by the definition order which causes them to drop in that same order as well. Fixes rust-lang/project-rfc-2229#42

compiler/rustc_typeck/src/check/upvar.rs

src/test/ui/closures/2229_closure_analysis/preserve_field_drop_order2.rs

arora-aman · 2021-09-23T22:16:50Z

src/test/ui/closures/2229_closure_analysis/preserve_field_drop_order.rs

+    let a = (HasDrop, HasDrop);
+    let b = (HasDrop, HasDrop);
+
+    let c = #[rustc_capture_analysis]


My concern with this test is that we need to reply on checking the stderr, and I feel like if the fields order does get changed, we won't need to update the annoatations in this file and someone might ignore the stderr changes because the ouput needed is all there.

I think we should go with testing with stdout here because changes to stdout are observable behaviour to the user and might be more cause of concern in case something breaks down the line.

I think it's useful to check both the internal results via #[rustc_capture_analysis] as well as the runtime results which is what these two tests do in combination. However, I totally agree with your point that as it is, it would be pretty easy for someone to change this test without realizing what parts are important and which parts aren't. I believe I can remove the checking for the notes we do not care about which should make it much more clear (this is the convention used in other ui tests as well).

compiler/rustc_typeck/src/check/upvar.rs

nikomatsakis · 2021-09-24T13:51:38Z

@bors r+

bors · 2021-09-24T13:51:40Z

📌 Commit 3893656 has been approved by nikomatsakis

…ikomatsakis [rfc 2229] Drop fully captured upvars in the same order as the regular drop code Currently, with the new 2021 edition, if a closure captures all of the fields of an upvar, we'll drop those fields in the order they are used within the closure instead of the normal drop order (the definition order of the fields in the type). This changes that so we sort the captured fields by the definition order which causes them to drop in that same order as well. Fixes rust-lang/project-rfc-2229#42 r? `@nikomatsakis`

nbdd0121 · 2021-09-24T16:32:59Z

Should this be backported to beta?

wesleywiser · 2021-09-24T18:32:24Z

Added some comments to existing tests, clarified which notes are important in the preserve_field_drop_order test and added some new test cases in preserve_field_drop_order2.

@bors r=nikomatsakis

bors · 2021-09-24T18:32:25Z

📌 Commit dd91804 has been approved by nikomatsakis

wesleywiser · 2021-09-24T18:33:31Z

Should this be backported to beta?

Yes, I believe we want this to ship with the 2021 edition.

…ingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#88893 (Add 1.56.0 release notes) - rust-lang#89001 (Be explicit about using Binder::dummy) - rust-lang#89072 (Avoid a couple of Symbol::as_str calls in cg_llvm ) - rust-lang#89104 (Simplify scoped_thread) - rust-lang#89208 ([rfc 2229] Drop fully captured upvars in the same order as the regular drop code) - rust-lang#89210 (Add missing time complexities to linked_list.rs) - rust-lang#89217 (Enable "generate-link-to-definition" option on rust tools docs as well) - rust-lang#89221 (Give better error for `macro_rules! name!`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

apiraino · 2021-09-30T14:33:52Z

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

…ikomatsakis [rfc 2229] Drop fully captured upvars in the same order as the regular drop code Currently, with the new 2021 edition, if a closure captures all of the fields of an upvar, we'll drop those fields in the order they are used within the closure instead of the normal drop order (the definition order of the fields in the type). This changes that so we sort the captured fields by the definition order which causes them to drop in that same order as well. Fixes rust-lang/project-rfc-2229#42 r? `@nikomatsakis`

[beta] Beta rollup * Fix WinUWP std compilation errors due to I/O safety rust-lang#88587 * Disable RemoveZsts in generators to avoid query cycles rust-lang#88979 * Disable the evaluation cache when in intercrate mode rust-lang#88994 * Fix linting when trailing macro expands to a trailing semi rust-lang#88996 * Don't use projection cache or candidate cache in intercrate mode rust-lang#89125 * 2229: Mark insignificant dtor in stdlib rust-lang#89144 * Temporarily rename int_roundings functions to avoid conflicts rust-lang#89184 * [rfc 2229] Drop fully captured upvars in the same order as the regular drop code rust-lang#89208 * Use the correct edition for syntax highlighting doctests rust-lang#89277 * Don't normalize opaque types with escaping late-bound regions rust-lang#89285 * Update Let's Encrypt ROOT CA certificate in dist-(i686|x86_64)-linux docker images rust-lang#89486 Cargo update: * - [beta] 1.56 backports (rust-lang/cargo#9958) * - [beta] Revert "When a dependency does not have a version, git or path… (rust-lang/cargo#9912) * - [beta] Fix rustc --profile=dev unstable check. (rust-lang/cargo#9901)

wesleywiser added 2 commits September 23, 2021 17:45

Add tests to verify the drop order of fields in fully captured upvars

7a3e450

rust-highfive assigned nikomatsakis Sep 23, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2021

arora-aman reviewed Sep 23, 2021

View reviewed changes

compiler/rustc_typeck/src/check/upvar.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

arora-aman reviewed Sep 23, 2021

View reviewed changes

nikomatsakis reviewed Sep 23, 2021

View reviewed changes

compiler/rustc_typeck/src/check/upvar.rs Show resolved Hide resolved

Fix tidy and respond to some feedback

3893656

wesleywiser changed the title ~~Rfc 2229 droporder~~ [rfc 2229] Drop fully captured upvars in the same order as the regular drop code Sep 24, 2021

arora-aman reviewed Sep 24, 2021

View reviewed changes

compiler/rustc_typeck/src/check/upvar.rs Show resolved Hide resolved

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 Sep 24, 2021

Manishearth mentioned this pull request Sep 24, 2021

Rollup of 8 pull requests #89226

Closed

Update and add more tests

dd91804

wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 24, 2021

workingjubilee mentioned this pull request Sep 24, 2021

Rollup of 8 pull requests #89230

Merged

bors merged commit 7ade6ed into rust-lang:master Sep 25, 2021

rustbot added this to the 1.57.0 milestone Sep 25, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 30, 2021

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 30, 2021

ehuss mentioned this pull request Oct 4, 2021

[beta] Beta rollup #89527

Merged

ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2021

Mark-Simulacrum modified the milestones: 1.57.0, 1.56.0 Oct 6, 2021

nathanwhit mentioned this pull request Oct 6, 2021

Internal compiler error: "entered unreachable code: we captured two identical projections" #89606

Closed

SNCPlay42 mentioned this pull request Nov 2, 2021

Typecheck panics feat. a closure and a struct with a private field and a public method #90483

Closed

This was referenced Dec 3, 2023

ice: ProjectionKinds Deref and Field were mismatched #118144

Closed

Fix ICE ProjectionKinds Deref and Field were mismatched #118584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

wesleywiser commented Sep 23, 2021

This comment has been minimized.

arora-aman Sep 23, 2021

wesleywiser Sep 24, 2021

nikomatsakis commented Sep 24, 2021

bors commented Sep 24, 2021

nbdd0121 commented Sep 24, 2021

wesleywiser commented Sep 24, 2021

bors commented Sep 24, 2021

wesleywiser commented Sep 24, 2021

apiraino commented Sep 30, 2021

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

[rfc 2229] Drop fully captured upvars in the same order as the regular drop code #89208

Conversation

wesleywiser commented Sep 23, 2021

This comment has been minimized.

arora-aman Sep 23, 2021

Choose a reason for hiding this comment

wesleywiser Sep 24, 2021

Choose a reason for hiding this comment

nikomatsakis commented Sep 24, 2021

bors commented Sep 24, 2021

nbdd0121 commented Sep 24, 2021

wesleywiser commented Sep 24, 2021

bors commented Sep 24, 2021

wesleywiser commented Sep 24, 2021

apiraino commented Sep 30, 2021