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

Allow unions to contain fields that don't need dropping #56440

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@bluss
Contributor

bluss commented Dec 2, 2018

The current rule was that a union field must be Copy, unless you use the
untagged_unions feature gate. Loosen this rule, so that it is "must not
need dropping".

This for example allows unions to use arbitrary field types by wrapping
them in ManuallyDrop.

See RFC 2514, tracking issue #55149

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 2, 2018

r? @estebank

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

@bluss

This comment has been minimized.

Contributor

bluss commented Dec 2, 2018

cc @eddyb @RalfJung

As written, this is insta-stable, but we might want to change that. Nightly with the untagged_unions feature gate doesn't have the requirement we're loosening here, so for users of that feature, nothing changes.

Show resolved Hide resolved src/librustc/middle/stability.rs Outdated
Show resolved Hide resolved src/librustc/middle/stability.rs Outdated
});
if field_needs_drop {
self.tcx.sess.span_err(item.span, "unions may not contain fields \
that need dropping");

This comment has been minimized.

@eddyb

eddyb Dec 2, 2018

Member

Note that this is still under !self.tcx.features().untagged_unions, so the error won't get triggered with #![feature(untagged_unions)], which is likely not what we want.

Also, it should probably not be in this file if it's not tied to stability checks.

cc @nikomatsakis Do you have any suggestions for where to place this code?

This comment has been minimized.

@RalfJung

RalfJung Dec 2, 2018

Member

This is some form of well-formedness check of a type declaration. Where do we usually do these? Things like "recursive occurrences must be below a pointer indirection", "only the last field of a struct may be unsized", ...

@eddyb

This comment has been minimized.

Member

eddyb commented Dec 2, 2018

@rust-highfive

This comment was marked as resolved.

Collaborator

rust-highfive commented Dec 2, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (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.
travis_time:end:00038e3c:start=1543768431821801159,finish=1543768491192500693,duration=59370699534
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:06:41]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:07:59] error: unused variable: `ty`
[00:07:59]    --> src/librustc/middle/stability.rs:761:21
[00:07:59]     |
[00:07:59] 761 |                 let ty = self.tcx.type_of(def_id);
[00:07:59]     |                     ^^ help: consider using `_ty` instead
[00:07:59]     = note: `-D unused-variables` implied by `-D warnings`
[00:07:59] 
[00:07:59] error: aborting due to previous error
[00:07:59] 
[00:07:59] 
[00:07:59] error: Could not compile `rustc`.
[00:07:59] 
[00:07:59] To learn more, run the command again with --verbose.
[00:07:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:07:59] expected success, got: exit code: 101
[00:07:59] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:07:59] Build completed unsuccessfully in 0:04:45
[00:07:59] make: *** [all] Error 1
[00:07:59] Makefile:28: recipe for target 'all' failed
47684 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release
46352 ./src/llvm-emscripten/test/MC
45752 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/x86_64-unknown-linux-gnu/release/deps
44272 ./src/libcompiler_builtins
---
travis_time:end:1e2aac1c:start=1543768980348625398,finish=1543768980354680610,duration=6055212
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0136f2c9
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:089fb5ca
travis_time:start:089fb5ca
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:027e490b
$ dmesg | grep -i kill

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)

Allow unions to contain fields that don't need dropping
The current rule was that a union field must be Copy, unless you use the
untagged_union feature gate. Loosen this rule, so that it is "must not
need dropping".

This for example allows unions to use arbitrary field types by wrapping
them in ManuallyDrop.

See RFC 2514.

@bluss bluss force-pushed the bluss:no-drop-in-union-fields branch from 4f96685 to f54b599 Dec 2, 2018

Show resolved Hide resolved src/test/run-pass/union/union-manuallydrop.rs Outdated
Show resolved Hide resolved src/test/run-pass/union/union-manuallydrop.rs Outdated
}
#[allow(dead_code)]
union UnionOk3<T: Copy> {

This comment has been minimized.

@Centril

Centril Dec 2, 2018

Contributor

It would also be good to add a test that checks how the compiler handles:

trait ImpliesCopy: Copy {}

union UnionOk4<T: ImpliesCopy> {
    empty: (),
    value: T,
}

A test file checking that T :/ Copy will lead to an error would also be good.

This comment has been minimized.

@bluss

bluss Dec 2, 2018

Contributor

The ui tests touched in the PR cover the non-Copy case, but do we need another test?

This comment has been minimized.

@Centril

Centril Dec 2, 2018

Contributor

I think it would be good to for example add:

union U5<T: Clone> { empty: (), value: T, }

your union U4<T> is the only one that covers type variables in that file so a bit of redundancy would be good imo. You can extend the feature gate test file; but we'll need to rememeber to not just throw the file away once we stabilize the gate.

This comment has been minimized.

@bluss

bluss Dec 2, 2018

Contributor

Ok, can't have a failing case in run-pass. It looks like this is normally in ui tests now, so I can add it there (I can't see a single compile-fail test for unions).

This comment has been minimized.

@Centril

Centril Dec 2, 2018

Contributor

UI tests are afaik compile-fail by default; but yeah, U5 needs to be added to a different file which has // compile-fail.

Show resolved Hide resolved src/test/run-pass/union/union-manuallydrop.rs Outdated
@RalfJung

This comment has been minimized.

Member

RalfJung commented Dec 2, 2018

As written, this is insta-stable, but we might want to change that. Nightly with the untagged_unions feature gate doesn't have the requirement we're loosening here, so for users of that feature, nothing changes.

I'd suggest we change the behavior of the feature gate only. So behavior of stable does not change, but we can test on nightly how these unions behave (e.g. the behavior of drop on assignment and the initialization checker might have to be tweaked to match the RFC).

My expectation would be that nightly with the untagged_union feature gate has this requirement.

@bluss

This comment has been minimized.

Contributor

bluss commented Dec 2, 2018

@RalfJung That makes sense, so we'd make a breaking change for that feature gate. In that case there should definitely be a suggestion to use ManuallyDrop as a diagnostic too.

@bluss

This comment has been minimized.

Contributor

bluss commented Dec 7, 2018

@nikomatsakis Any thoughts on what the correct place in the code is for this, a check union fields don't need dropping?

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Dec 11, 2018

@bluss I think this check would make sense to do in this function

fn check_union<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
id: ast::NodeId,
span: Span) {
let def_id = tcx.hir().local_def_id(id);
let def = tcx.adt_def(def_id);
def.destructor(tcx); // force the destructor to be evaluated
check_representable(tcx, span, def_id);
check_packed(tcx, span, def_id);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment