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

Change untagged_unions to not allow union fields with drop #62330

Open
wants to merge 11 commits into
base: master
from

Conversation

@SimonSapin
Copy link
Contributor

commented Jul 3, 2019

This is a rebase of #56440, massaged to solve merge conflicts and make the test suite pass.

Change untagged_unions to not allow union fields with drop

Union fields may now never have a type with attached destructor. This for example allows unions to use arbitrary field types only by wrapping them in ManuallyDrop (or similar).

The stable rule remains, that union fields must be Copy. We use the new rule for the untagged_union feature.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

Show resolved Hide resolved src/librustc_typeck/check/mod.rs Outdated
Show resolved Hide resolved src/librustc_typeck/check/mod.rs Outdated
Show resolved Hide resolved src/librustc_typeck/check/mod.rs Outdated
Show resolved Hide resolved src/librustc_typeck/error_codes.rs Outdated
Show resolved Hide resolved src/test/run-pass/drop/dynamic-drop.rs Outdated
Show resolved Hide resolved src/test/run-pass/union/union-nodrop.rs Outdated
Show resolved Hide resolved src/test/run-pass/union/union-nodrop.rs Outdated
@Centril

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

cc tracking issue: #55149

@SimonSapin SimonSapin force-pushed the SimonSapin:no-drop-in-union-fields branch from a51a67a to 81949b0 Jul 4, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

☔️ The latest upstream changes (presumably #62419) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:no-drop-in-union-fields branch from 81949b0 to 0951cca Jul 5, 2019

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

@rust-highfive rust-highfive assigned RalfJung and unassigned eddyb Jul 11, 2019

@@ -73,7 +72,8 @@ pub unsafe fn ptr_rotate<T>(mut left: usize, mid: *mut T, mut right: usize) {
}

let mut rawarray = MaybeUninit::<RawArray<T>>::uninit();
let buf = &mut (*rawarray.as_mut_ptr()).typed as *mut [T; 2] as *mut T;
let buf = &mut (*rawarray.as_mut_ptr()).typed as *mut [MaybeUninit<T>; 2]

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

Might be worth adding a note that this is creating a reference to uninitialized data, cc rust-lang/rfcs#2582 ?

/// When the `#![feature(untagged_unions)]` gate is active,
/// check that the fields of the `union` does not contain fields that need dropping.
fn check_union_fields(tcx: TyCtxt<'_>, _: Span, item_def_id: DefId) -> bool {
// Without the feature we check Copy types only later

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

Where is "later"? A reference would be nice.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

Is there a good reason these two checks are done in totally different places? Doing them in the same place would be better IMO.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 14, 2019

Author Contributor

I have no idea. This code comes from #56440.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

Could you find out?

let f = ptr::read(&mut (*data).f);
ptr::write(&mut (*data).r, f());
let f = ptr::read(&mut *(*data).f);
ptr::write(&mut *(*data).r, f());

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

These look like good candidates for ManuallyDrop::{read,write}?


#![feature(untagged_unions)]

union MaybeItem<T: Iterator> {
union MaybeItem<T: Iterator> where T::Item: Copy {
elem: T::Item,

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

I feel like wrapping this in ManuallyDrop more closely preserves the intent of the original test.


error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0740`.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

Could you add a test for a union with a field that's a union with a manual impl Drop? Just to make sure we do not treat all unions as not having any drop glue.

}
let item_type = tcx.type_of(item_def_id);
if let ty::Adt(def, substs) = item_type.sty {
if def.is_union() {

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 14, 2019

Member

It looks like check_union_fields is only called from check_union, so should we not already know that this is a union?

This check looks a bit funny I have to say because it looks like it checks the type of an item, as opposed to items that are types... does that mean if we have a static FOO: Union, we run this check on the type of the static?

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 15, 2019

Member

No, this is just getting the fully generic type from the DefId of the definition of that type (i.e. the union Foo {...}), which may seem a bit silly.

Maybe it would be easier to just put this code in check_union?

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 16, 2019

Member

So type_of can be called on a union item and then returns that union?

I wouldn't call that the "type of" the item, that's why I am confused. The type of union Foo { ... } should be Type or so.

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 18, 2019

Member

Type is a kind because we don't have type universes.

type_of(X) merely means the Ty<'tcx> associated with that definition, e.g. the type that is used to look up Y in X::Y.

The _of suffix is a convention I came up with to make the query names flow a bit better, so foo_of(X) is the foo for that X, in general.

Maybe we should just drop the _of? But then it might not be obvious that e.g. tcx.ty(def_id) is querying anything.

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 18, 2019

Member

Although nowadays we could do tcx.def_ty(def_id), tcx.def_generics(def_id), etc. and it would probably work well enough.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 19, 2019

Member

Type is a kind because we don't have type universes.

Actually i32 is a "kind", at least when I look at this thing... :/ That Kind type is really more like exists k: kind, thing_of_kind k. It certainly is not representing a "kind" itself, that would be an enum Kind { Lifetime, Type, Const }.

The _of suffix is a convention I came up with to make the query names flow a bit better, so foo_of(X) is the foo for that X, in general.

Maybe type_for? type_at? I also like def_ty though.

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 19, 2019

Member

Actually i32 is a "kind", at least when I look at this thing... :/

Sorry, I was referring to the type theory notion of "kind", not the Kind in rustc... which we should already rename to Term or something (idk if there is a discussion somewhere).

Maybe type_for? type_at? I also like def_ty though.

Out of those three, I still think def_foo is the form that would work best in all cases (and the query system generally can only talk about defs, anything finer-grained than that is nested within HIR/MIR anyway).

This comment has been minimized.

Copy link
@Centril

Centril Jul 21, 2019

Member

that would be an enum Kind { Lifetime, Type, Const }.

(The Const here is surely indexed by a Ty<'tcx> as the kind of const T: A is not the same as const T: B?)

I agree we should rename to Term since Kind as in rustc is just plainly wrong.

This comment has been minimized.

Copy link
@RalfJung

RalfJung Jul 21, 2019

Member

(The Const here is surely indexed by a Ty<'tcx> as the kind of const T: A is not the same as const T: B?)

Well that depends on how precise our kind system is. ;)

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

☔️ The latest upstream changes (presumably #63029) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnTitor

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Ping from triage: @SimonSapin, any updates on this? you should also resolve conflicts.

@SimonSapin SimonSapin force-pushed the SimonSapin:no-drop-in-union-fields branch from 0951cca to d2a4d57 Aug 5, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Rebased, but I’m not the original author of this code so I feel I’m not able to answer some of the review comments. I’d appreciate if someone more familiar with compiler internals is willing to take over.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-05T16:19:33.6312378Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-05T16:19:33.6505965Z ##[command]git config gc.auto 0
2019-08-05T16:19:33.6589564Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-05T16:19:33.6644934Z ##[command]git config --get-all http.proxy
2019-08-05T16:19:33.6787047Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/62330/merge:refs/remotes/pull/62330/merge
---
2019-08-05T16:20:10.3163046Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-05T16:20:10.3163076Z 
2019-08-05T16:20:10.3163283Z   git checkout -b <new-branch-name>
2019-08-05T16:20:10.3163496Z 
2019-08-05T16:20:10.3163563Z HEAD is now at a91151e62 Merge d2a4d5732f55204b21d0fc0b6ca31375e26f74df into 4be067558962c004b638e4c6f162d50f7c0c98b6
2019-08-05T16:20:10.3322060Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-05T16:20:10.3324878Z ==============================================================================
2019-08-05T16:20:10.3324935Z Task         : Bash
2019-08-05T16:20:10.3324981Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-05T17:23:21.8166646Z .................................................................................................... 1400/8828
2019-08-05T17:23:28.0171526Z .................................................................................................... 1500/8828
2019-08-05T17:23:40.9382506Z ....................................................................i...............i............... 1600/8828
2019-08-05T17:23:48.3033728Z .................................................................................................... 1700/8828
2019-08-05T17:24:04.2113395Z ......................................................iiiii......................................... 1800/8828
2019-08-05T17:24:16.0511268Z .................................................................................................... 2000/8828
2019-08-05T17:24:18.6973967Z .................................................................................................... 2100/8828
2019-08-05T17:24:22.1406633Z .................................................................................................... 2200/8828
2019-08-05T17:24:30.3514382Z .................................................................................................... 2300/8828
---
2019-08-05T17:28:25.9335711Z .................................................................................................... 5200/8828
2019-08-05T17:28:34.4877314Z .....................................................................i.............................. 5300/8828
2019-08-05T17:28:42.0481869Z .................................................................................................... 5400/8828
2019-08-05T17:28:49.2727584Z .................................................................................................... 5500/8828
2019-08-05T17:29:00.7763942Z ...............................................................ii...i..ii...........i............... 5600/8828
2019-08-05T17:29:25.6092377Z .................................................................................................... 5800/8828
2019-08-05T17:29:30.7832532Z .................................................................................................... 5900/8828
2019-08-05T17:29:30.7832532Z .................................................................................................... 5900/8828
2019-08-05T17:29:37.1194686Z ................................................................i..ii............................... 6000/8828
2019-08-05T17:30:06.9953633Z .................................................................................................... 6200/8828
2019-08-05T17:30:09.2799401Z .......i............................................................................................ 6300/8828
2019-08-05T17:30:11.5014800Z ...............................................................................i.................... 6400/8828
2019-08-05T17:30:14.2790581Z .................................................................................................... 6500/8828
---
2019-08-05T17:34:25.9845824Z failures:
2019-08-05T17:34:25.9873366Z 
2019-08-05T17:34:25.9874167Z ---- [ui] ui/union/union-with-drop-fields-lint-rpass.rs stdout ----
2019-08-05T17:34:25.9874347Z 
2019-08-05T17:34:25.9874870Z error: test compilation failed although it shouldn't!
2019-08-05T17:34:25.9875048Z status: exit code: 1
2019-08-05T17:34:25.9875973Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/union/union-with-drop-fields-lint-rpass/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/union/union-with-drop-fields-lint-rpass/auxiliary" "-A" "unused"
2019-08-05T17:34:25.9876593Z ------------------------------------------
2019-08-05T17:34:25.9876741Z 
2019-08-05T17:34:25.9877079Z ------------------------------------------
2019-08-05T17:34:25.9877238Z stderr:
2019-08-05T17:34:25.9877238Z stderr:
2019-08-05T17:34:25.9877589Z ------------------------------------------
2019-08-05T17:34:25.9877751Z warning: unknown lint: `unions_with_drop_fields`
2019-08-05T17:34:25.9878339Z    |
2019-08-05T17:34:25.9878339Z    |
2019-08-05T17:34:25.9878470Z LL | #![allow(unions_with_drop_fields)]
2019-08-05T17:34:25.9878751Z    |
2019-08-05T17:34:25.9878873Z    = note: `#[warn(unknown_lints)]` on by default
2019-08-05T17:34:25.9879000Z 
2019-08-05T17:34:25.9879124Z error[E0740]: unions may not contain fields that need dropping
2019-08-05T17:34:25.9879124Z error[E0740]: unions may not contain fields that need dropping
2019-08-05T17:34:25.9879503Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:12:5
2019-08-05T17:34:25.9879687Z    |
2019-08-05T17:34:25.9879808Z LL |     a: String, // OK
2019-08-05T17:34:25.9880064Z    |
2019-08-05T17:34:25.9880186Z note: `std::mem::ManuallyDrop` can be used to wrap the type
2019-08-05T17:34:25.9880554Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:12:5
2019-08-05T17:34:25.9880997Z    |
2019-08-05T17:34:25.9880997Z    |
2019-08-05T17:34:25.9881199Z LL |     a: String, // OK
2019-08-05T17:34:25.9881677Z 
2019-08-05T17:34:25.9881895Z error[E0740]: unions may not contain fields that need dropping
2019-08-05T17:34:25.9882380Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:20:5
2019-08-05T17:34:25.9882578Z    |
2019-08-05T17:34:25.9882578Z    |
2019-08-05T17:34:25.9882698Z LL |     a: S, // OK
2019-08-05T17:34:25.9882953Z    |
2019-08-05T17:34:25.9883077Z note: `std::mem::ManuallyDrop` can be used to wrap the type
2019-08-05T17:34:25.9883466Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:20:5
2019-08-05T17:34:25.9883632Z    |
2019-08-05T17:34:25.9883632Z    |
2019-08-05T17:34:25.9883751Z LL |     a: S, // OK
2019-08-05T17:34:25.9883988Z 
2019-08-05T17:34:25.9884351Z error[E0740]: unions may not contain fields that need dropping
2019-08-05T17:34:25.9884848Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:25:5
2019-08-05T17:34:25.9885030Z    |
2019-08-05T17:34:25.9885030Z    |
2019-08-05T17:34:25.9885150Z LL |     a: T, // OK
2019-08-05T17:34:25.9885412Z    |
2019-08-05T17:34:25.9885533Z note: `std::mem::ManuallyDrop` can be used to wrap the type
2019-08-05T17:34:25.9885922Z   --> /checkout/src/test/ui/union/union-with-drop-fields-lint-rpass.rs:25:5
2019-08-05T17:34:25.9886088Z    |
2019-08-05T17:34:25.9886088Z    |
2019-08-05T17:34:25.9886207Z LL |     a: T, // OK
2019-08-05T17:34:25.9886445Z 
2019-08-05T17:34:25.9886566Z error: aborting due to 3 previous errors
2019-08-05T17:34:25.9886667Z 
2019-08-05T17:34:25.9887046Z For more information about this error, try `rustc --explain E0740`.
---
2019-08-05T17:34:25.9915626Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:536:22
2019-08-05T17:34:25.9915876Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-08-05T17:34:25.9942831Z 
2019-08-05T17:34:26.8257846Z 
2019-08-05T17:34:26.8298815Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-08-05T17:34:26.8299091Z 
2019-08-05T17:34:26.8299136Z 
2019-08-05T17:34:26.8299186Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-08-05T17:34:26.8299239Z Build completed unsuccessfully in 1:07:58
2019-08-05T17:34:26.8299239Z Build completed unsuccessfully in 1:07:58
2019-08-05T17:34:26.8357601Z ##[error]Bash exited with code '1'.
2019-08-05T17:34:26.8411585Z ##[section]Starting: Checkout
2019-08-05T17:34:26.8413222Z ==============================================================================
2019-08-05T17:34:26.8413283Z Task         : Get sources
2019-08-05T17:34:26.8413350Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bluss and others added some commits Dec 19, 2018

Change untagged_unions to not allow union fields with drop
Union fields may now never have a type with attached destructor.
This for example allows unions to use arbitrary field types only by
wrapping
them in ManuallyDrop.

The stable rule remains, that union fields must be Copy. We use the new
rule for the `untagged_union` feature.

See RFC 2514.

Note for ui tests:
We can't test move out through Box's deref-move since we can't
have a Box in a union anymore.
Remove unions_with_drop_fields lint
Cases where it would trigger are now hard errors.
Update src/librustc_typeck/check/mod.rs
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Update src/librustc_typeck/check/mod.rs
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Update src/librustc_typeck/error_codes.rs
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>
Update src/test/run-pass/union/union-nodrop.rs
Co-Authored-By: Mazdak Farrokhzad <twingoow@gmail.com>

@SimonSapin SimonSapin force-pushed the SimonSapin:no-drop-in-union-fields branch from d2a4d57 to 20c917a Aug 5, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

☔️ The latest upstream changes (presumably #61937) made this pull request unmergeable. Please resolve the merge conflicts.

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Aug 16, 2019

Ping from triage. @SimonSapin any updates on this? Thanks.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

This hasn’t changed:

I’m not the original author of this code so I feel I’m not able to answer some of the review comments. I’d appreciate if someone more familiar with compiler internals is willing to take over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.