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

Implement RFC 2645 (transparent enums and unions) #60463

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

mjbshaw
Copy link
Contributor

@mjbshaw mjbshaw commented May 2, 2019

Tracking issue: #60405

@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2019
@Centril
Copy link
Contributor

Centril commented May 2, 2019

cc @pnkfelix

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

This looks mostly good. Just a few comments.

src/librustc/hir/check_attr.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs 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_lint/types.rs Outdated Show resolved Hide resolved
@varkor varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 6, 2019
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@mjbshaw
Copy link
Contributor Author

mjbshaw commented May 8, 2019

@varkor Thanks for reviewing this! I think I've addressed all the issues you raised. Let me know if there is more.

@varkor
Copy link
Member

varkor commented May 10, 2019

It's looking pretty good now! I've just got a couple more small comments about the diagnostics, making sure that it reads well even in the edge cases.

Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

Sorry for jumping in late, I agree with @varkor that this looks pretty good, but I have a few nits.

src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Show resolved Hide resolved
src/librustc_typeck/error_codes.rs Show resolved Hide resolved
src/librustc/ty/mod.rs Show resolved Hide resolved
src/test/codegen/repr-transparent.rs Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@mjbshaw mjbshaw force-pushed the transparent branch 2 times, most recently from afe263a to b0f8f1b Compare May 22, 2019 15:14
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@mjbshaw
Copy link
Contributor Author

mjbshaw commented Jun 11, 2019

@rkruppe Thanks for the review! I think I've addressed all your comments. Fingers crossed all tests pass now...

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Jun 11, 2019

LGTM, thanks!

@bors r=varkor,rkruppe rollup=never

(no rollup because IME thes platform-dependent codegen tests for ABIs always take several tries to get right)

@bors
Copy link
Contributor

bors commented Jun 11, 2019

📌 Commit dac1c6a has been approved by varkor,rkruppe

@bors bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 11, 2019
@bors

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 11, 2019

📌 Commit dac1c6a has been approved by varkor,rkruppe

@Centril
Copy link
Contributor

Centril commented Jun 11, 2019

r? @rkruppe

@rust-highfive rust-highfive assigned hanna-kruppe and unassigned varkor Jun 11, 2019
@bors
Copy link
Contributor

bors commented Jun 11, 2019

⌛ Testing commit dac1c6a with merge 8e948df...

bors added a commit that referenced this pull request Jun 11, 2019
Implement RFC 2645 (transparent enums and unions)

Tracking issue: #60405
@bors
Copy link
Contributor

bors commented Jun 11, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: varkor,rkruppe
Pushing 8e948df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2019
@bors bors merged commit dac1c6a into rust-lang:master Jun 11, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants