Skip to content

Implement repr(transparent) #47158

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

Merged
merged 1 commit into from
Jan 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 176 additions & 0 deletions src/doc/unstable-book/src/language-features/repr-transparent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
# `repr_transparent`

The tracking issue for this feature is: [#43036]

[#43036]: https://github.com/rust-lang/rust/issues/43036

------------------------

This feature enables the `repr(transparent)` attribute on structs, which enables
the use of newtypes without the usual ABI implications of wrapping the value in
a struct.

## Background

It's sometimes useful to add additional type safety by introducing *newtypes*.
For example, code that handles numeric quantities in different units such as
millimeters, centimeters, grams, kilograms, etc. may want to use the type system
to rule out mistakes such as adding millimeters to grams:

```rust
use std::ops::Add;

struct Millimeters(f64);
struct Grams(f64);

impl Add<Millimeters> for Millimeters {
type Output = Millimeters;

fn add(self, other: Millimeters) -> Millimeters {
Millimeters(self.0 + other.0)
}
}

// Likewise: impl Add<Grams> for Grams {}
```

Other uses of newtypes include using `PhantomData` to add lifetimes to raw
pointers or to implement the "phantom types" pattern. See the [PhantomData]
documentation and [the Nomicon][nomicon-phantom] for more details.

The added type safety is especially useful when interacting with C or other
languages. However, in those cases we need to ensure the newtypes we add do not
introduce incompatibilities with the C ABI.

## Newtypes in FFI

Luckily, `repr(C)` newtypes are laid out just like the type they wrap on all
platforms which Rust currently supports, and likely on many more. For example,
consider this C declaration:

```C
struct Object {
double weight; //< in grams
double height; //< in millimeters
// ...
}

void frobnicate(struct Object *);
```

While using this C code from Rust, we could add `repr(C)` to the `Grams` and
`Millimeters` newtypes introduced above and use them to add some type safety
while staying compatible with the memory layout of `Object`:

```rust,no_run
#[repr(C)]
struct Grams(f64);

#[repr(C)]
struct Millimeters(f64);

#[repr(C)]
struct Object {
weight: Grams,
height: Millimeters,
// ...
}

extern {
fn frobnicate(_: *mut Object);
}
```

This works even when adding some `PhantomData` fields, because they are
zero-sized and therefore don't have to affect the memory layout.

However, there's more to the ABI than just memory layout: there's also the
question of how function call arguments and return values are passed. Many
common ABI treat a struct containing a single field differently from that field
itself, at least when the field is a scalar (e.g., integer or float or pointer).

To continue the above example, suppose the C library also exposes a function
like this:

```C
double calculate_weight(double height);
```

Using our newtypes on the Rust side like this will cause an ABI mismatch on many
platforms:

```rust,ignore
extern {
fn calculate_weight(height: Millimeters) -> Grams;
}
```

For example, on x86_64 Linux, Rust will pass the argument in an integer
register, while the C function expects the argument to be in a floating-point
register. Likewise, the C function will return the result in a floating-point
register while Rust will expect it in an integer register.

Note that this problem is not specific to floats: To give another example,
32-bit x86 linux will pass and return `struct Foo(i32);` on the stack while
`i32` is placed in registers.

## Enter `repr(transparent)`

So while `repr(C)` happens to do the right thing with respect to memory layout,
it's not quite the right tool for newtypes in FFI. Instead of declaring a C
struct, we need to communicate to the Rust compiler that our newtype is just for
type safety on the Rust side. This is what `repr(transparent)` does.

The attribute can be applied to a newtype-like structs that contains a single
field. It indicates that the newtype should be represented exactly like that
field's type, i.e., the newtype should be ignored for ABI purpopses: not only is
it laid out the same in memory, it is also passed identically in function calls.

In the above example, the ABI mismatches can be prevented by making the newtypes
`Grams` and `Millimeters` transparent like this:

```rust
#![feature(repr_transparent)]

#[repr(transparent)]
struct Grams(f64);

#[repr(transparent)]
struct Millimeters(f64);
```

In addition to that single field, any number of zero-sized fields are permitted,
including but not limited to `PhantomData`:

```rust
#![feature(repr_transparent)]

use std::marker::PhantomData;

struct Foo { /* ... */ }

#[repr(transparent)]
struct FooPtrWithLifetime<'a>(*const Foo, PhantomData<&'a Foo>);

#[repr(transparent)]
struct NumberWithUnit<T, U>(T, PhantomData<U>);

struct CustomZst;

#[repr(transparent)]
struct PtrWithCustomZst<'a> {
ptr: FooPtrWithLifetime<'a>,
some_marker: CustomZst,
}
```

Transparent structs can be nested: `PtrWithCustomZst` is also represented
exactly like `*const Foo`.

Because `repr(transparent)` delegates all representation concerns to another
type, it is incompatible with all other `repr(..)` attributes. It also cannot be
applied to enums, unions, empty structs, structs whose fields are all
zero-sized, or structs with *multiple* non-zero-sized fields.

[PhantomData]: https://doc.rust-lang.org/std/marker/struct.PhantomData.html
[nomicon-phantom]: https://doc.rust-lang.org/nomicon/phantom-data.html
64 changes: 63 additions & 1 deletion src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,69 @@ that refers to itself. That is permitting, since the closure would be
invoking itself via a virtual call, and hence does not directly
reference its own *type*.

"##, }
"##,

E0692: r##"
A `repr(transparent)` type was also annotated with other, incompatible
representation hints.

Erroneous code example:

```compile_fail,E0692
#![feature(repr_transparent)]

#[repr(transparent, C)] // error: incompatible representation hints
struct Grams(f32);
```

A type annotated as `repr(transparent)` delegates all representation concerns to
another type, so adding more representation hints is contradictory. Remove
either the `transparent` hint or the other hints, like this:

```
#![feature(repr_transparent)]

#[repr(transparent)]
struct Grams(f32);
```

Alternatively, move the other attributes to the contained type:

```
#![feature(repr_transparent)]

#[repr(C)]
struct Foo {
x: i32,
// ...
}

#[repr(transparent)]
struct FooWrapper(Foo);
```

Note that introducing another `struct` just to have a place for the other
attributes may have unintended side effects on the representation:

```
#![feature(repr_transparent)]

#[repr(transparent)]
struct Grams(f32);

#[repr(C)]
struct Float(f32);

#[repr(transparent)]
struct Grams2(Float); // this is not equivalent to `Grams` above
```

Here, `Grams2` is a not equivalent to `Grams` -- the former transparently wraps
a (non-transparent) struct containing a single float, while `Grams` is a
transparent wrapper around a float. This can make a difference for the ABI.
"##,

}


register_diagnostics! {
Expand Down
25 changes: 21 additions & 4 deletions src/librustc/hir/check_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
let mut int_reprs = 0;
let mut is_c = false;
let mut is_simd = false;
let mut is_transparent = false;

for hint in &hints {
let name = if let Some(name) = hint.name() {
Expand Down Expand Up @@ -137,6 +138,14 @@ impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
continue
}
}
"transparent" => {
is_transparent = true;
if target != Target::Struct {
("a", "struct")
} else {
continue
}
}
"i8" | "u8" | "i16" | "u16" |
"i32" | "u32" | "i64" | "u64" |
"isize" | "usize" => {
Expand All @@ -155,14 +164,22 @@ impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
.emit();
}

// Just point at all repr hints if there are any incompatibilities.
// This is not ideal, but tracking precisely which ones are at fault is a huge hassle.
let hint_spans = hints.iter().map(|hint| hint.span);

// Error on repr(transparent, <anything else>).
if is_transparent && hints.len() > 1 {
let hint_spans: Vec<_> = hint_spans.clone().collect();
span_err!(self.tcx.sess, hint_spans, E0692,
"transparent struct cannot have other repr hints");
}
// Warn on repr(u8, u16), repr(C, simd), and c-like-enum-repr(C, u8)
if (int_reprs > 1)
|| (is_simd && is_c)
|| (int_reprs == 1 && is_c && is_c_like_enum(item)) {
// Just point at all repr hints. This is not ideal, but tracking
// precisely which ones are at fault is a huge hassle.
let spans: Vec<_> = hints.iter().map(|hint| hint.span).collect();
span_warn!(self.tcx.sess, spans, E0566,
let hint_spans: Vec<_> = hint_spans.collect();
span_warn!(self.tcx.sess, hint_spans, E0566,
"conflicting representation hints");
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#![feature(box_syntax)]
#![feature(conservative_impl_trait)]
#![feature(const_fn)]
#![feature(copy_closures, clone_closures)]
#![feature(core_intrinsics)]
#![feature(drain_filter)]
#![feature(dyn_trait)]
Expand Down
6 changes: 5 additions & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1499,8 +1499,9 @@ bitflags! {
const IS_C = 1 << 0;
const IS_PACKED = 1 << 1;
const IS_SIMD = 1 << 2;
const IS_TRANSPARENT = 1 << 3;
// Internal only for now. If true, don't reorder fields.
const IS_LINEAR = 1 << 3;
const IS_LINEAR = 1 << 4;

// Any of these flags being set prevent field reordering optimisation.
const IS_UNOPTIMISABLE = ReprFlags::IS_C.bits |
Expand Down Expand Up @@ -1540,6 +1541,7 @@ impl ReprOptions {
flags.insert(match r {
attr::ReprC => ReprFlags::IS_C,
attr::ReprPacked => ReprFlags::IS_PACKED,
attr::ReprTransparent => ReprFlags::IS_TRANSPARENT,
attr::ReprSimd => ReprFlags::IS_SIMD,
attr::ReprInt(i) => {
size = Some(i);
Expand Down Expand Up @@ -1567,6 +1569,8 @@ impl ReprOptions {
#[inline]
pub fn packed(&self) -> bool { self.flags.contains(ReprFlags::IS_PACKED) }
#[inline]
pub fn transparent(&self) -> bool { self.flags.contains(ReprFlags::IS_TRANSPARENT) }
#[inline]
pub fn linear(&self) -> bool { self.flags.contains(ReprFlags::IS_LINEAR) }

pub fn discr_type(&self) -> attr::IntType {
Expand Down
17 changes: 14 additions & 3 deletions src/librustc_lint/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
match def.adt_kind() {
AdtKind::Struct => {
if !def.repr.c() {
if !def.repr.c() && !def.repr.transparent() {
return FfiUnsafe("found struct without foreign-function-safe \
representation annotation in foreign module, \
consider adding a #[repr(C)] attribute to the type");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change this message to refer to #[repr(transparent)]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. The message is already too long IMHO. Maybe this lint should use a help diagnostic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll do this in a follow-up PR.

Expand All @@ -427,13 +427,24 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
adding a member to this struct");
}

// We can't completely trust repr(C) markings; make sure the
// fields are actually safe.
// We can't completely trust repr(C) and repr(transparent) markings;
// make sure the fields are actually safe.
let mut all_phantom = true;
for field in &def.non_enum_variant().fields {
let field_ty = cx.fully_normalize_associated_types_in(
&field.ty(cx, substs)
);
// repr(transparent) types are allowed to have arbitrary ZSTs, not just
// PhantomData -- skip checking all ZST fields
Copy link
Member

Choose a reason for hiding this comment

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

We could restrict it to PhantomData, didn't know the lint was special-casing that already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn. Supporting only PhantomData would be the more conservative option and I don't have a use case for other ZSTs at hand. OTOH repr(transparent) is explicitly ignoring all ZSTs, so it makes sense to allow them for C interop too even though repr(C) warns about other ZSTs.

Copy link
Member

Choose a reason for hiding this comment

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

No, I meant also changing rustc_typeck, not just this check.

Copy link
Contributor Author

@hanna-kruppe hanna-kruppe Jan 3, 2018

Choose a reason for hiding this comment

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

Ah. The RFC says all ZSTs are fine, and that seems nicer to me than special casing PhantomData, but being conservative and following the precedent of the improper-ctypes lint also makes sense. If anyone has a preference for starting out accepting only PhantomData, I'll implement that. Otherwise, well, it's not exactly a burden to support other ZSTs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, if the RFC says all ZSTs, I'm fine with that.

Copy link
Member

@kennytm kennytm Jan 3, 2018

Choose a reason for hiding this comment

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

Wouldn't allowing all ZSTs could change the size and alignment, making it not really transparent?

#[repr(transparent)]
struct U8 {
    a: u8,
    not_really: [u32; 0],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennytm A good point, but this was already brought up in the RFC discussion, with the proposed solution (that I implemented) being to prohibit ZSTs with alignment > 1.

Copy link
Member

Choose a reason for hiding this comment

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

@rkuppe thanks, found the check in check_transparent

if def.repr.transparent() {
let is_zst = (cx, cx.param_env(field.did))
.layout_of(field_ty)
.map(|layout| layout.is_zst())
.unwrap_or(false);
Copy link
Member

Choose a reason for hiding this comment

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

You can write this with map_or fwiw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

layout_of returns Result, map_or only exists on Option.

Copy link
Member

Choose a reason for hiding this comment

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

Huh, didn't know that.

Copy link
Member

Choose a reason for hiding this comment

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

But you can call ok() first before calling map_or()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess, but it doesn't look any nicer than this IMO.

if is_zst {
continue;
}
}
let r = self.check_type_for_ffi(cache, field_ty);
match r {
FfiSafe => {
Expand Down
Loading