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

Stabilize `#[repr(transparent)]` on `enum`s in Rust 1.42.0 #68122

Merged
merged 2 commits into from Jan 27, 2020

Conversation

@Centril
Copy link
Member

Centril commented Jan 11, 2020

Stabilization report

The following is the stabilization report for #![feature(transparent_enums)].

Tracking issue: #60405
Version target: 1.42 (2020-01-30 => beta, 2020-03-12 => stable).

User guide

A struct with only a single non-ZST field (let's call it foo) can be marked as #[repr(transparent)]. Such a struct has the same layout and ABI as foo. Here, we also extend this ability to enums with only one variant, subject to the same restrictions as for the equivalent struct. That is, you can now write:

#[repr(transparent)]
enum Foo { Bar(u8) }

which, in terms of layout and ABI, is equivalent to:

#[repr(transparent)]
struct Foo(u8);

Motivation

This is not a major feature that will unlock new and important use-cases. The utility of repr(transparent) enums is indeed limited. However, there is still some value in it:

  1. It provides conceptual simplification of the language in terms of treating univariant enums and structs the same, as both are product types. Indeed, languages like Haskell only have data as the only way to construct user-defined ADTs in the language.

  2. In rare occasions, it might be that the user started out with a univariant enum for whatever reason (e.g. they thought they might extend it later). Now they want to make this enum transparent without breaking users by turning it into a struct. By lifting the restriction here, now they can.

Technical specification

The reference specifies repr(transparent) on a struct as:

The transparent Representation

The transparent representation can only be used on structs that have:

  • a single field with non-zero size, and
  • any number of fields with size 0 and alignment 1 (e.g. PhantomData<T>).

Structs with this representation have the same layout and ABI as the single non-zero sized field.

This is different than the C representation because a struct with the C representation will always have the ABI of a C struct while, for example, a struct with the transparent representation with a primitive field will have the ABI of the primitive field.

Because this representation delegates type layout to another type, it cannot be used with any other representation.

Here, we amend this to include univariant enums as well with the same static restrictions and the same effects on dynamic semantics.

Tests

All the relevant tests are adjusted in the PR diff but are recounted here:

  • src/test/ui/repr/repr-transparent.rs checks that repr(transparent) on an enum must be univariant, rather than having zero or more than one variant. Restrictions on the fields inside the only variants, like for those on structs, are also checked here.

  • A number of codegen tests are provided as well:

    • src/test/codegen/repr-transparent.rs (the canonical test)
    • src/test/codegen/repr-transparent-aggregates-1.rs
    • src/test/codegen/repr-transparent-aggregates-2.rs
    • src/test/codegen/repr-transparent-aggregates-3.rs
  • src/test/ui/lint/lint-ctypes-enum.rs tests the interactions with the improper_ctypes lint.

History

  • 2019-04-30, RFC rust-lang/rfcs#2645
    Author: @mjbshaw
    Reviewers: The Language Team

    This is the RFC that proposes allowing #[repr(transparent)] on enums and union.

  • 2019-06-11, PR #60463
    Author: @mjbshaw
    Reviewers: @varkor and @rkruppe

    The PR implements the RFC aforementioned in full.

  • 2019, PR #67323
    Author: @Centril
    Reviewers: @davidtwco

    The PR reorganizes the static checks taking advantage of the fact that structs and unions are internally represented as ADTs with a single variant.

  • This PR stabilizes transparent_enums.

Related / possible future work

The remaining work here is to figure out the semantics of #[repr(transparent)] on unions and stabilize those. This work continues to be tracked in #60405.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 11, 2020

r? @petrochenkov

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

@Centril

This comment has been minimized.

Copy link
Member Author

Centril commented Jan 11, 2020

Stabilization proposal

Dear community & language team,
I propose that we stabilize #![feature(transparent_enums)].

@rfcbot merge

Tracking issue: #60405
Version target: 1.42 (2020-01-30 => beta, 2020-03-12 => stable).

Please see the attached report above for details.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 11, 2020

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 11, 2020

r=me on the implementation, waiting for FCP

@hanna-kruppe

This comment has been minimized.

Copy link
Member

hanna-kruppe commented Jan 11, 2020

I'm noticing this very late, but it appears we've all forgotten that, just as with transparent structs, there also needs to be an alignment requirement on the ZST fields. It's not even mentioned in the RFC as far as I can tell, and not in the unstable book chapter added in the implementation PR. The only reason it's implemented is code sharing with transparent structs. I assume this doesn't actually influence the decision to stabilize, but:

  • Whoever ends up writing the documentation for this feature, please don't forget about it (cf. rust-lang/reference#737).
  • There don't seem to be tests for this restriction, please add some. There are tests for the same restriction on transparent structs (in ui/repr/repr-transparent.rs), so I'm not overly worried about the check getting lost in a refactoring, but it's easy to test and apparently people keep forgetting about it, so...
@Centril

This comment has been minimized.

Copy link
Member Author

Centril commented Jan 12, 2020

  • There don't seem to be tests for this restriction, please add some.

Thanks for noticing; I've extended the test.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 13, 2020

@rfcbot reviewed

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Jan 16, 2020

We should document the behavior of combining repr(u32) and similar with repr(transparent). Right now that's an error, which is reasonable. In the future we might want to define that explicitly.

But the current behavior seems reasonable, so:
@rfcbot reviewed

@Centril

This comment has been minimized.

Copy link
Member Author

Centril commented Jan 16, 2020

Wake up bot :)

@rfcbot reviewed

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 16, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril Centril removed the I-nominated label Jan 16, 2020
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Jan 18, 2020

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

@Centril Centril force-pushed the Centril:stabilize-transparent-enums branch from 10a576f to 25460eb Jan 20, 2020
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 26, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Centril

This comment has been minimized.

Copy link
Member Author

Centril commented Jan 26, 2020

@bors r=petrochenkov p=3

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 26, 2020

📌 Commit 25460eb has been approved by petrochenkov

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

⌛️ Testing commit 25460eb with merge c3681d6...

bors added a commit that referenced this pull request Jan 27, 2020
…enkov

Stabilize `#[repr(transparent)]` on `enum`s in Rust 1.42.0

# Stabilization report

The following is the stabilization report for `#![feature(transparent_enums)]`.

Tracking issue: #60405
[Version target](https://forge.rust-lang.org/#current-release-versions): 1.42 (2020-01-30 => beta, 2020-03-12 => stable).

## User guide

A `struct` with only a single non-ZST field (let's call it `foo`) can be marked as `#[repr(transparent)]`. Such a `struct` has the same layout and ABI as `foo`. Here, we also extend this ability to `enum`s with only one variant, subject to the same restrictions as for the equivalent `struct`. That is, you can now write:

```rust
#[repr(transparent)]
enum Foo { Bar(u8) }
```

which, in terms of layout and ABI, is equivalent to:

```rust
#[repr(transparent)]
struct Foo(u8);
```

## Motivation

This is not a major feature that will unlock new and important use-cases. The utility of `repr(transparent)` `enum`s is indeed limited. However, there is still some value in it:

1. It provides conceptual simplification of the language in terms of treating univariant `enum`s and `struct`s the same, as both are product types. Indeed, languages like Haskell only have `data` as the only way to construct user-defined ADTs in the language.

2. In rare occasions, it might be that the user started out with a univariant `enum` for whatever reason (e.g. they thought they might extend it later). Now they want to make this `enum` `transparent` without breaking users by turning it into a `struct`. By lifting the restriction here, now they can.

## Technical specification

The reference specifies [`repr(transparent)` on a `struct`](https://doc.rust-lang.org/nightly/reference/type-layout.html#the-transparent-representation) as:

> ### The transparent Representation
>
>  The `transparent` representation can only be used on `struct`s that have:
>  - a single field with non-zero size, and
>  - any number of fields with size 0 and alignment 1 (e.g. `PhantomData<T>`).
>
> Structs with this representation have the same layout and ABI as the single non-zero sized field.
>
> This is different than the `C` representation because a struct with the `C` representation will always have the ABI of a `C` `struct` while, for example, a struct with the `transparent` representation with a primitive field will have the ABI of the primitive field.
>
> Because this representation delegates type layout to another type, it cannot be used with any other representation.

Here, we amend this to include univariant `enum`s as well with the same static restrictions and the same effects on dynamic semantics.

## Tests

All the relevant tests are adjusted in the PR diff but are recounted here:

- `src/test/ui/repr/repr-transparent.rs` checks that `repr(transparent)` on an `enum` must be univariant, rather than having zero or more than one variant. Restrictions on the fields inside the only variants, like for those on `struct`s, are also checked here.

- A number of codegen tests are provided as well:
    - `src/test/codegen/repr-transparent.rs` (the canonical test)
    - `src/test/codegen/repr-transparent-aggregates-1.rs`
    - `src/test/codegen/repr-transparent-aggregates-2.rs`
    - `src/test/codegen/repr-transparent-aggregates-3.rs`

- `src/test/ui/lint/lint-ctypes-enum.rs` tests the interactions with the `improper_ctypes` lint.

## History

- 2019-04-30, RFC rust-lang/rfcs#2645
  Author: @mjbshaw
  Reviewers: The Language Team

  This is the RFC that proposes allowing `#[repr(transparent)]` on `enum`s and `union`.

- 2019-06-11, PR #60463
  Author: @mjbshaw
  Reviewers: @varkor and @rkruppe

  The PR implements the RFC aforementioned in full.

- 2019, PR #67323
  Author: @Centril
  Reviewers: @davidtwco

  The PR reorganizes the static checks taking advantage of the fact that `struct`s and `union`s are internally represented as ADTs with a single variant.

- This PR stabilizes `transparent_enums`.

## Related / possible future work

The remaining work here is to figure out the semantics of `#[repr(transparent)]` on `union`s and stabilize those. This work continues to be tracked in #60405.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 27, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing c3681d6 to master...

@bors bors added the merged-by-bors label Jan 27, 2020
@bors bors merged commit 25460eb into rust-lang:master Jan 27, 2020
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20200120.22 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@Centril Centril deleted the Centril:stabilize-transparent-enums branch Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.