Update rust reference info about closures #1059

roxelo · 2021-07-06T15:26:33Z

This PR updates the closure types page so it includes up to date information on the new behavior introduced by RFC2229/Edition 2021

cc: @arora-aman
r? @nikomatsakis

Closes #1066

ehuss

Thanks! Just some minor editorial comments. I'll let Niko review the actual content. 😉

src/types/closure.md

nikomatsakis

The content is good. It'd be nice to also add in the "formal algorithm" that we came up with in our last meeting.

src/types/closure.md

arora-aman · 2021-07-20T08:50:16Z

src/types/closure.md

@@ -316,65 +316,69 @@ If a closure captures a field of a composite types such as structs, tuples, and
 ## Overall Capture analysis algorithm

 * Input:
-    * Analyzing the closure C yields a set of `(Mode, Place)` pairs that are accessed
+    * Analyzing the closure C yields a mapping of `Place -> Mode` that are accessed


Add details for how things are currently stored in the initial analysis phase

arora-aman · 2021-07-20T08:50:32Z

src/types/closure.md

 * Helper functions:
-    * `copy_type(Mode, Place) -> (Mode, Place)`


This is not a concern of the capture analysis anymore

src/types/closure.md

arora-aman · 2021-07-20T08:51:36Z

src/types/closure.md


-struct Point { x: i32, y: i32 }


These examples were us discussiong what analysis should do in moving copy types and working through some examles

src/types/closure.md

nikomatsakis

This is very nice. I left some comments.

nikomatsakis · 2021-07-30T12:25:18Z

src/types/closure.md

+* Input:
+    * Analyzing the closure C yields a mapping of `Place -> Mode` that are accessed
+    * Access mode is `ref`, `ref uniq`, `ref mut`, or `by-value` (ordered least to max)
+        * For a `Place` that is used in two different acess modes within the same closure, the mode reported from closure analysis is the maximum access mode.


Suggested change

* For a `Place` that is used in two different acess modes within the same closure, the mode reported from closure analysis is the maximum access mode.

* For a `Place` that is used in two different access modes within the same closure, the mode reported from closure analysis is the maximum access mode.

nikomatsakis · 2021-07-30T12:26:09Z

src/types/closure.md

+
+* Input:
+    * Analyzing the closure C yields a mapping of `Place -> Mode` that are accessed
+    * Access mode is `ref`, `ref uniq`, `ref mut`, or `by-value` (ordered least to max)


I'm still a bit unclear on the best way to incorporate ref uniq here -- my sense is that the results that come in initially should never have ref uniq, but it gets determined later, whenever we do a truncation through a * of an &mut (as we talked about).

nikomatsakis · 2021-07-30T12:27:47Z

src/types/closure.md

+                * Let Place1 = (Base, Projections[0..=l])
+                * Return (Place1, Ref)
+
+## Key examples


These are pretty inscrutable if you didn't attend our meeting, I think

src/types/closure.md

…FC2229

ehuss · 2021-11-19T20:01:35Z

@roxelo / @arora-aman / @nikomatsakis Is there an update on how this is going? It looks like niko left some comments that may not have been addressed?

Co-authored-by: Josh Triplett <josh@joshtriplett.org>

arora-aman · 2021-11-19T22:07:27Z

@roxelo / @arora-aman / @nikomatsakis Is there an update on how this is going? It looks like niko left some comments that may not have been addressed?

@ehuss I think they have been addressed, github didn't auto resolve them because lines were added instead of them being changed.

There was one style change that josh left that I have now committed. Hopefully tests pass given Edition 2021 has been stabilized

arora-aman · 2021-11-19T22:16:55Z

I should have time tomorrow to fix the style errors.

roxelo · 2021-12-03T15:57:53Z

Just saw this now, can help if needed @arora-aman

shepmaster · 2022-04-06T19:06:27Z

src/types/closure.md

-borrowed to iterate over, the code would not compile.
+### Capturing references in move contexts
+
+Moving fields out of references is not allowed. As a result, in the case of move closures, when values accessed through a shared references are moved into the closure body, the compiler will truncate right before a dereference.


I would love to understand the logic behind

Moving fields out of references is not allowed

Where could I learn more? My broader context is Why does moving a disjoint field capture into a closure differ when the type is a value vs a reference?

Veykril · 2023-05-07T07:14:33Z

src/types/closure.md

+### Wild Card Patterns
+Closures only capture data that needs to be read, which means the following closures will not capture `x`
+
+```rust
+let x = 10;
+let c = || {
+    let _ = x;
+};
+
+let c = || match x {
+    _ => println!("Hello World!")
+};


~~This seems to be wrong. Both of these examples are captured by value today~~ -> https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4044e24fa1cffb795c2dd03239eb5fb2

Nevermind, coercion logic does not have access to semantic captures

HKalbasi · 2023-05-07T10:43:18Z

src/types/closure.md

+let c_box = || {
+    println!("{}", (*b).0); // closure captures `b`
+};
+```

 ## Unique immutable borrows in captures


The example in this section needs to be updated, as it now captures *x by mutable borrow, not x by unique immutable borrow.

ehuss · 2023-05-07T16:42:41Z

@Veykril or @HKalbasi Would either of you be interested in helping to get this PR over the line? Are either of you familiar with how they are implemented? I am not familiar with how closures are implemented in rustc. I could start reviewing all of the RFC 2229 PR's and reviewing all of the current implementation, but I have been dragging because that sounds like weeks of work. It would help to have someone else to ask questions to and to help with reviewing.

HKalbasi · 2023-05-07T17:38:13Z

I guess I can help since I reimplemented this in rust-analyzer and got more or less familiar with the rustc implementation. What should I do?

ehuss · 2023-05-07T17:56:23Z

Thanks! I'm not sure exactly how to make progress, but perhaps some questions might help to get started:

Have you carefully reviewed this PR? If not, would you be willing to give it a detailed examination? Are there any details missing?
Can you point me to a rough outline of the areas in rustc that I would need to look at to understand the text of this PR?
How difficult do you think it would be for me to figure it out? I'm moderately familiar with rustc, but not this area (nor type inference or type checking, which I'm not sure is relevant here). I'm not sure if I'm vastly over- or under-estimating how hard this is to review.

I can try to review the comment you left and push changes where I feel confident in them, but if I end up having questions it would be helpful to have someone to ask.

Hopefully there aren't too many gray areas where it isn't clear if some behavior was intended or not.

HKalbasi · 2023-05-07T18:37:32Z

Have you carefully reviewed this PR? If not, would you be willing to give it a detailed examination? Are there any details missing?

Not yet, but will do it in the following days when I found time.

Can you point me to a rough outline of the areas in rustc that I would need to look at to understand the text of this PR?

The whole thing happens in expr_use_visitor.rs and upvars.rs. The entry point is the analyze_closure in upvars.rs, which calls ExprUseVisitor::consume_body to collect all of the places, and then aggregates them, applies some changes and optimizations, trims the places in old editions and computes the closure kind.

How difficult do you think it would be for me to figure it out? I'm moderately familiar with rustc, but not this area (nor type inference or type checking, which I'm not sure is relevant here). I'm not sure if I'm vastly over- or under-estimating how hard this is to review.

It's pretty straightforward, specially if you are familiar with the Place concept. I think you won't have difficulties in finding the corresponding code for any part of text, but checking that if every small detail is covered might becomes a little difficult.

ehuss · 2023-05-07T21:40:00Z

src/types/closure.md

+### Reference into unaligned `struct`s
+
+Because it is `unsafe` to hold references to unaligned fields in a structure, closures will only capture the prefix of the path that runs up to, but not including, the first field access into an unaligned structure.
+
+```rust
+#[repr(packed)]
+struct T(String, String);
+
+let t = T(String::from("foo"), String::from("bar"));
+let c = || unsafe {
+    println!("{}", t.0); // closure captures t
+};
+```


There seems to be an issue with this entire section. My understanding is that it is not unsafe to take a reference to an unaligned field; it is undefined behavior, and the compiler won't even let you naively do that (indeed this example doesn't compile).

I'm thinking about rewriting this, and changing the example to something like:

#[repr(packed)] struct Packed { f1: u8, f2: u8, } let mut packed = Packed { f1: 1, f2: 2 }; let c = || { // closure captures `packed` in its entirety as an immutable borrow let raw_f2 = std::ptr::addr_of!(packed.f2); assert_eq!(unsafe { raw_f2.read_unaligned() }, 2); }; c();

Yes the example is wrong and doesn't compile, but I'm not sure if your example is relevant here. I guess the addr_of! macro will always capture the whole item, since it is expanded into something like packed as usize + some_offset.

I think this example would work:

#[repr(packed)] struct T(i32, i32); fn main() { let t = T(2, 5); let c = || { t.0; // closure captures t.0 if not packed, but t if packed t.1; }; println!("{}", core::mem::size_of_val(&c)) }

Normally, t.0 should be captured by immutable borrow (since i32 is copy) but now that t is packed and taking &t.0 is UB, the whole t will be captured and this code will print 8 instead of 16 when T is not packed.

ehuss · 2023-05-07T21:44:12Z

src/types/closure.md

+### Packed-field-ref-and-move
+
+When you have a closure that both references a packed field (which is unsafe) and moves from it (which is safe) we capture the entire struct, rather than just moving the field. This is to aid in predictability, so that removing the move doesn't make the closure become unsafe:


Similar to Reference into unaligned structs, this doesn't compile and needs to be rewritten.

ehuss · 2024-07-08T19:35:45Z

I have posted #1521 with some updates to this PR, since I am unable to push directly to this one.

roxelo force-pushed the closure-doc branch from 79513ac to d238a8c Compare July 6, 2021 15:35

ehuss added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Jul 6, 2021

nikomatsakis self-assigned this Jul 6, 2021

roxelo force-pushed the closure-doc branch from d238a8c to 881f305 Compare July 6, 2021 16:41

ehuss reviewed Jul 8, 2021

View reviewed changes

src/types/closure.md Outdated Show resolved Hide resolved

src/types/closure.md Outdated Show resolved Hide resolved

src/types/closure.md Outdated Show resolved Hide resolved

src/types/closure.md Outdated Show resolved Hide resolved

roxelo force-pushed the closure-doc branch 2 times, most recently from 37ce76c to bc0e50c Compare July 9, 2021 00:16

ehuss added the A-edition-2021 Area: Edition 2021 label Jul 9, 2021

ehuss mentioned this pull request Jul 9, 2021

2021: Update for disjoint captures (RFC 2229) #1066

Open

nikomatsakis requested changes Jul 19, 2021

View reviewed changes

src/types/closure.md Show resolved Hide resolved

arora-aman force-pushed the closure-doc branch from 0c5ae17 to 72fd033 Compare July 20, 2021 08:49

arora-aman reviewed Jul 20, 2021

View reviewed changes

arora-aman force-pushed the closure-doc branch 2 times, most recently from 58a0132 to 880405c Compare July 20, 2021 09:00

arora-aman reviewed Jul 20, 2021

View reviewed changes

src/types/closure.md Show resolved Hide resolved

arora-aman force-pushed the closure-doc branch from 880405c to 0a9323a Compare July 20, 2021 09:06

arora-aman reviewed Jul 20, 2021

View reviewed changes

src/types/closure.md Show resolved Hide resolved

lqd reviewed Jul 20, 2021

View reviewed changes

src/types/closure.md Outdated Show resolved Hide resolved

nikomatsakis requested changes Jul 30, 2021

View reviewed changes

arora-aman force-pushed the closure-doc branch 2 times, most recently from 92336ed to 6b88e48 Compare August 2, 2021 09:14

flip1995 mentioned this pull request Aug 4, 2021

Add blog post to introduce disjoint capture in closures rust-lang/blog.rust-lang.org#884

Closed

joshtriplett reviewed Aug 17, 2021

View reviewed changes

src/types/closure.md Outdated Show resolved Hide resolved

nikomatsakis mentioned this pull request Aug 17, 2021

stabilize disjoint capture in closures (RFC 2229) rust-lang/rust#88126

Closed

2 tasks

roxelo and others added 6 commits October 5, 2021 00:36

Update closure types documentation so it includes information about R…

7c776c8

…FC2229

Address comments

124ece6

Copy capture analyis alogrithm from hackmd

fa56c01

Update copy type, optimization, make algorithm (place, mode)

52a9af9

fix stray merge conflict

7944512

Minor edits from Feedback

55c0829

arora-aman added 2 commits October 5, 2021 00:36

Add details on truncation and ref-uniq

fe03349

Update closure algorithm per #88467 #88477

702a71f

arora-aman force-pushed the closure-doc branch from 6b88e48 to 702a71f Compare October 5, 2021 07:56

ehuss removed the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Nov 1, 2021

Update src/types/closure.md

44f6e1e

Co-authored-by: Josh Triplett <josh@joshtriplett.org>

shepmaster reviewed Apr 6, 2022

View reviewed changes

ehuss mentioned this pull request May 27, 2022

Reference not updated to describe disjoint capture in closures rust-lang/rust#97451

Closed

Veykril mentioned this pull request May 7, 2023

feat: Closure capture inlay hints rust-lang/rust-analyzer#14742

Merged

Veykril reviewed May 7, 2023

View reviewed changes

HKalbasi reviewed May 7, 2023

View reviewed changes

ehuss reviewed May 9, 2023

View reviewed changes

ehuss mentioned this pull request Oct 2, 2023

types/closure is not accurate. We don't need to use a new variable when capturing a field from a struct #1412

Closed

ehuss mentioned this pull request Mar 29, 2024

Is the documentation description outdated? #1487

Closed

ehuss mentioned this pull request Jul 8, 2024

Update closures for edition 2021 disjoint closure capturing #1521

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rust reference info about closures #1059

Update rust reference info about closures #1059

roxelo commented Jul 6, 2021 •

edited by ehuss

Loading

ehuss left a comment

nikomatsakis left a comment

arora-aman Jul 20, 2021

arora-aman Jul 20, 2021

arora-aman Jul 20, 2021 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Jul 30, 2021

nikomatsakis Jul 30, 2021

nikomatsakis Jul 30, 2021

ehuss commented Nov 19, 2021

arora-aman commented Nov 19, 2021

arora-aman commented Nov 19, 2021 •

edited

Loading

roxelo commented Dec 3, 2021

shepmaster Apr 6, 2022 •

edited

Loading

Veykril May 7, 2023 •

edited

Loading

HKalbasi May 7, 2023

ehuss commented May 7, 2023

HKalbasi commented May 7, 2023

ehuss commented May 7, 2023

HKalbasi commented May 7, 2023

ehuss May 7, 2023 •

edited

Loading

HKalbasi May 11, 2023

ehuss May 7, 2023

ehuss commented Jul 8, 2024

		* Helper functions:
		* `copy_type(Mode, Place) -> (Mode, Place)`

	* For a `Place` that is used in two different acess modes within the same closure, the mode reported from closure analysis is the maximum access mode.
	* For a `Place` that is used in two different access modes within the same closure, the mode reported from closure analysis is the maximum access mode.

		### Packed-field-ref-and-move

		When you have a closure that both references a packed field (which is unsafe) and moves from it (which is safe) we capture the entire struct, rather than just moving the field. This is to aid in predictability, so that removing the move doesn't make the closure become unsafe:

Update rust reference info about closures #1059

Are you sure you want to change the base?

Update rust reference info about closures #1059

Conversation

roxelo commented Jul 6, 2021 • edited by ehuss Loading

ehuss left a comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arora-aman Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

nikomatsakis left a 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

ehuss commented Nov 19, 2021

arora-aman commented Nov 19, 2021

arora-aman commented Nov 19, 2021 • edited Loading

roxelo commented Dec 3, 2021

shepmaster Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Veykril May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented May 7, 2023

HKalbasi commented May 7, 2023

ehuss commented May 7, 2023

HKalbasi commented May 7, 2023

ehuss May 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehuss commented Jul 8, 2024

roxelo commented Jul 6, 2021 •

edited by ehuss

Loading

arora-aman Jul 20, 2021 •

edited

Loading

arora-aman commented Nov 19, 2021 •

edited

Loading

shepmaster Apr 6, 2022 •

edited

Loading

Veykril May 7, 2023 •

edited

Loading

ehuss May 7, 2023 •

edited

Loading