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.

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

2229: Handle capturing a reference into a repr packed struct #82878

Merged
merged 1 commit into from
Mar 13, 2021

Conversation

arora-aman
Copy link
Member

RFC 1240 states that it is unsafe to capture references into a
packed-struct. This PR ensures that when a closure captures a precise
path, we aren't violating this safety constraint.

To acheive so we restrict the capture precision to the struct itself.

An interesting edge case where we decided to restrict precision:

struct Foo(String);

let foo: Foo;
let c = || {
    println!("{}", foo.0);
    let x = foo.0;
}

Given how closures get desugared today, foo.0 will be moved into the
closure, making the println!, safe. However this can be very subtle
and also will be unsafe if the closure gets inline.

Closes: rust-lang/project-rfc-2229#33

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me

compiler/rustc_typeck/src/check/upvar.rs Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 8, 2021

📌 Commit efe0319f05bbccafe282bd6c6eed5bdff06edf1b has been approved by nikomatsakis

@bors 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 Mar 8, 2021
@nikomatsakis
Copy link
Contributor

@bors r-

I want to think about this layout call for one second

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2021
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

OK, one nit but i'm ready to land.

let pos = place.projections.iter().enumerate().position(|(i, p)| {
let ty = place.ty_before_projection(i);

match p.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of matches and ifs here. It'd be nice to have a comment indicating what you are looking for. Something like:

// Return true for fields of packed structs, unless those fields have alignment 1.

RFC 1240 states that it is unsafe to capture references into a
packed-struct. This PR ensures that when a closure captures a precise
path, we aren't violating this safety constraint.

To acheive so we restrict the capture precision to the struct itself.

An interesting edge case:
```rust
struct Foo(String);

let foo: Foo;
let c = || {
    println!("{}", foo.0);
    let x = foo.0;
}
```

Given how closures get desugared today, foo.0 will be moved into the
closure, making the `println!`, safe. However this can be very subtle
and also will be unsafe if the closure gets inline.

Closes: rust-lang/project-rfc-2229#33
@arora-aman
Copy link
Member Author

@nikomatsakis updated

@nikomatsakis
Copy link
Contributor

@bors r+ delegate+

@bors
Copy link
Contributor

bors commented Mar 12, 2021

📌 Commit 612a9b2 has been approved by nikomatsakis

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2021
@bors
Copy link
Contributor

bors commented Mar 12, 2021

✌️ @arora-aman can now approve this pull request

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 12, 2021
…akis

2229: Handle capturing a reference into a repr packed struct

RFC 1240 states that it is unsafe to capture references into a
packed-struct. This PR ensures that when a closure captures a precise
path, we aren't violating this safety constraint.

To acheive so we restrict the capture precision to the struct itself.

An interesting edge case where we decided to restrict precision:
```rust
struct Foo(String);

let foo: Foo;
let c = || {
    println!("{}", foo.0);
    let x = foo.0;
}
```

Given how closures get desugared today, foo.0 will be moved into the
closure, making the `println!`, safe. However this can be very subtle
and also will be unsafe if the closure gets inline.

Closes: rust-lang/project-rfc-2229#33

r? `@nikomatsakis`
@bors
Copy link
Contributor

bors commented Mar 13, 2021

⌛ Testing commit 612a9b2 with merge 178bd91...

@bors
Copy link
Contributor

bors commented Mar 13, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 178bd91 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2021
@bors bors merged commit 178bd91 into rust-lang:master Mar 13, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle #[repr(packed)] with feature gate
6 participants