Make assignments to `Copy` union fields safe #42083

petrochenkov · 2017-05-18T11:59:48Z

This is an accompanying PR to PR #42068 stabilizing FFI unions.

This was first proposed in #32836 (comment), see subsequent comments as well.
Assignments to Copy union fields do not read any data from the union and are equivalent to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular.

It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by unused_unsafe lint in stable code.

cc #32836
r? @nikomatsakis

petrochenkov · 2017-05-18T12:01:12Z

src/librustc/middle/effect.rs

+                            let field_ty = self.tables.expr_ty_adjusted(lhs);
+                            let owner_def_id = self.tcx.hir.body_owner_def_id(self.body_id);
+                            let param_env = self.tcx.parameter_environment(owner_def_id);
+                            if field_ty.moves_by_default(self.tcx, &param_env, field.span) {


Is there a way to query "current parameter environment" or "current body id" without keeping them in the visitor, like I did here?

Not at present.

(I think what I would rather do is to have a loop over all the bodies and instantiate a distinct visitor per body, with just the appropriate tables etc. This visitor would have to walk up to find out contextual information though (since closures, at least, inherit their "unsafety" from their parents at present.))

alexcrichton · 2017-05-18T20:10:38Z

I think @nikomatsakis is out today, but we'll make sure he gets around to this in the near future!

carols10cents · 2017-05-22T16:52:27Z

ping @nikomatsakis, are you back from PTO? have some more reviews!

nikomatsakis · 2017-05-23T23:19:12Z

@bors r+

bors · 2017-05-23T23:19:13Z

📌 Commit 59b65b3 has been approved by nikomatsakis

@nikomatsakis

…akis Make assignments to `Copy` union fields safe This is an accompanying PR to PR rust-lang#42068 stabilizing FFI unions. This was first proposed in rust-lang#32836 (comment), see subsequent comments as well. Assignments to `Copy` union fields do not read any data from the union and are [equivalent](rust-lang#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular. It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code. cc rust-lang#32836 r? @nikomatsakis

@nikomatsakis

…akis Make assignments to `Copy` union fields safe This is an accompanying PR to PR rust-lang#42068 stabilizing FFI unions. This was first proposed in rust-lang#32836 (comment), see subsequent comments as well. Assignments to `Copy` union fields do not read any data from the union and are [equivalent](rust-lang#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular. It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code. cc rust-lang#32836 r? @nikomatsakis

Mark-Simulacrum · 2017-05-24T20:39:36Z

I suspect that this will fail at some point, the previous rollup failed with, which looks suspiciously like it comes from this PR; possibly a conflict with another PR that landed -- I seem to recall @nikomatsakis landing something that renamed parameter_environment into param_env.

[00:06:22]    Compiling rustc v0.0.0 (file:///checkout/src/librustc)
[00:06:35] error: no method named `parameter_environment` found for type `ty::context::TyCtxt<'a, 'tcx, 'tcx>` in the current scope
[00:06:35]    --> /checkout/src/librustc/middle/effect.rs:231:54
[00:06:35]     |
[00:06:35] 231 |                             let param_env = self.tcx.parameter_environment(owner_def_id);
[00:06:35]     |                                                      ^^^^^^^^^^^^^^^^^^^^^
[00:06:35] 
[00:06:35] error[E0308]: mismatched types
[00:06:35]    --> /checkout/src/librustc/middle/effect.rs:232:68
[00:06:35]     |
[00:06:35] 232 |                             if field_ty.moves_by_default(self.tcx, &param_env, field.span) {
[00:06:35]     |                                                                    ^^^^^^^^^^ expected struct `ty::ParamEnv`, found reference
[00:06:35]     |
[00:06:35]     = note: expected type `ty::ParamEnv<'_>`
[00:06:35]                found type `&_`
[00:06:35] 
[00:06:42] error: aborting due to 2 previous errors
[00:06:42] 
[00:06:42] error: Could not compile `rustc`.

…tion

petrochenkov · 2017-05-25T07:49:02Z

@bors r-

petrochenkov · 2017-05-25T07:49:41Z

Rebased.
@bors r=nikomatsakis

bors · 2017-05-25T07:49:41Z

📌 Commit fa13cd3 has been approved by nikomatsakis

bors · 2017-05-25T07:50:05Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Stabilize rfc 1506 - Clarified ADT Kinds #41145

bors · 2017-05-25T07:50:05Z

📌 Commit fa13cd3 has been approved by nikomatsakis

bors · 2017-05-26T10:17:57Z

⌛ Testing commit fa13cd3 with merge 2f278c5...

@nikomatsakis

Make assignments to `Copy` union fields safe This is an accompanying PR to PR #42068 stabilizing FFI unions. This was first proposed in #32836 (comment), see subsequent comments as well. Assignments to `Copy` union fields do not read any data from the union and are [equivalent](#32836 (comment)) to whole union assignments, which are safe, so they should be safe as well. This removes a significant number of "false positive" unsafe blocks, in code dealing with FFI unions in particular. It desirable to make this change now, together with stabilization of FFI unions, because now it affecfts only unstable code, but later it will cause warnings/errors caused by `unused_unsafe` lint in stable code. cc #32836 r? @nikomatsakis

bors · 2017-05-26T12:43:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 2f278c5 to master...

@nikomatsakis

Stabilize unions with `Copy` fields and no destructor What else is needed: - [x] Documentation (rust-lang/reference#57). - [x] Making assignments to `Copy` union fields safe (#42083). - [ ] Backport? (The "stabilization decision" is from [Apr 13](#32836 (comment)), it's just this PR is late.) cc #32836 r? @nikomatsakis

rust-highfive assigned nikomatsakis May 18, 2017

petrochenkov commented May 18, 2017

View reviewed changes

petrochenkov mentioned this pull request May 18, 2017

Stabilize unions with Copy fields and no destructor #42068

Merged

3 tasks

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2017

nikomatsakis 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 May 23, 2017

Mark-Simulacrum mentioned this pull request May 24, 2017

Rollup of 14 pull requests #42193

Closed

Mark-Simulacrum mentioned this pull request May 24, 2017

Rollup of 17 pull requests #42199

Closed

petrochenkov added 2 commits May 25, 2017 09:36

Make assignments to Copy union fields safe

4386f97

Use parameter environment associated with field use, not field defini…

fa13cd3

…tion

petrochenkov force-pushed the safeassign branch from 59b65b3 to fa13cd3 Compare May 25, 2017 07:47

bors merged commit fa13cd3 into rust-lang:master May 26, 2017

This was referenced May 31, 2017

[beta] Stabilize unions with Copy fields and no destructor #42321

Closed

[beta] Backport union stabilization PRs #42328

Closed

petrochenkov mentioned this pull request Aug 3, 2017

Rust mutable union allows write to field w/o needing unsafe block in stable ( Documentation issue? ) rust-lang/rfcs#2095

Closed

petrochenkov deleted the safeassign branch August 26, 2017 00:15

niting mentioned this pull request May 24, 2020

Update README to use a Docker image matching the one used in CI. rust-vmm/kvm-ioctls#95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make assignments to `Copy` union fields safe #42083

Make assignments to `Copy` union fields safe #42083

petrochenkov commented May 18, 2017

petrochenkov May 18, 2017

nikomatsakis May 22, 2017

nikomatsakis May 22, 2017

alexcrichton commented May 18, 2017

carols10cents commented May 22, 2017

nikomatsakis commented May 23, 2017

bors commented May 23, 2017

Mark-Simulacrum commented May 24, 2017

petrochenkov commented May 25, 2017

petrochenkov commented May 25, 2017 •

edited

Loading

bors commented May 25, 2017

bors commented May 25, 2017

bors commented May 25, 2017

bors commented May 26, 2017

bors commented May 26, 2017

Make assignments to Copy union fields safe #42083

Make assignments to Copy union fields safe #42083

Conversation

petrochenkov commented May 18, 2017

petrochenkov May 18, 2017

Choose a reason for hiding this comment

nikomatsakis May 22, 2017

Choose a reason for hiding this comment

nikomatsakis May 22, 2017

Choose a reason for hiding this comment

alexcrichton commented May 18, 2017

carols10cents commented May 22, 2017

nikomatsakis commented May 23, 2017

bors commented May 23, 2017

Mark-Simulacrum commented May 24, 2017

petrochenkov commented May 25, 2017

petrochenkov commented May 25, 2017 • edited Loading

bors commented May 25, 2017

bors commented May 25, 2017

bors commented May 25, 2017

bors commented May 26, 2017

bors commented May 26, 2017

Make assignments to `Copy` union fields safe #42083

Make assignments to `Copy` union fields safe #42083

petrochenkov commented May 25, 2017 •

edited

Loading