Implement repr(transparent) #47158

hanna-kruppe · 2018-01-03T16:52:35Z

r? @eddyb for the functional changes. The bulk of the PR is error messages and docs, might be good to have a doc person look over those.

cc #43036
cc @nox

eddyb · 2018-01-03T17:14:56Z

cc @steveklabnik @nikomatsakis

eddyb · 2018-01-03T17:16:21Z

src/librustc_lint/types.rs

                        let mut all_phantom = true;
                        for field in &def.struct_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


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

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.

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

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.

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

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], }

@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.

@rkuppe thanks, found the check in check_transparent

eddyb · 2018-01-03T17:16:52Z

src/librustc_lint/types.rs

@@ -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");


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

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

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

eddyb · 2018-01-03T17:17:23Z

src/librustc_lint/types.rs

+                                let is_zst = (cx, cx.param_env(field.did))
+                                    .layout_of(field_ty)
+                                    .map(|layout| layout.is_zst())
+                                    .unwrap_or(false);


You can write this with map_or fwiw.

layout_of returns Result, map_or only exists on Option.

Huh, didn't know that.

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

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

eddyb · 2018-01-03T22:42:21Z

@rust-lang/docs r=me on the implementation side.

frewsxcv · 2018-01-03T23:10:32Z

docs look good to me! though travis found some doctest errors that should be addressed before approving

hanna-kruppe · 2018-01-04T01:22:31Z

Doc tests should be fixed.

hanna-kruppe · 2018-01-04T12:56:37Z

@bors r=eddyb

bors · 2018-01-04T12:56:38Z

📌 Commit 697c390 has been approved by eddyb

bors · 2018-01-07T03:42:04Z

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

hanna-kruppe · 2018-01-07T10:21:35Z

@bors r=eddyb

bors · 2018-01-07T10:21:36Z

📌 Commit 4e1d212 has been approved by eddyb

bors · 2018-01-07T12:07:23Z

⌛ Testing commit 4e1d2122bc7d09bb5fa8b05c558d4e8bf481e0c2 with merge 78e7563b1b9088a9295d3325670d7aee376d3100...

bors · 2018-01-07T13:12:11Z

💔 Test failed - status-travis

kennytm · 2018-01-07T13:32:40Z

Test failed on i686-musl, the type is not transparent 🤔

[01:00:07] ---- [codegen] codegen/repr-transparent.rs stdout ----
[01:00:07] 	
[01:00:07] error: verification with 'FileCheck' failed
[01:00:07] status: exit code: 1
[01:00:07] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll" "/checkout/src/test/codegen/repr-transparent.rs"
[01:00:07] stdout:
[01:00:07] ------------------------------------------
[01:00:07] 
[01:00:07] ------------------------------------------
[01:00:07] stderr:
[01:00:07] ------------------------------------------
[01:00:07] /checkout/src/test/codegen/repr-transparent.rs:160:11: error: expected string not found in input
[01:00:07] // CHECK: define i32 @test_Rgb8Wrap(i32 %arg0)
[01:00:07]           ^
[01:00:07] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll:142:135: note: scanning from here
[01:00:07] define void @test_BigW(%BigW* noalias nocapture sret dereferenceable(128), %BigW* byval noalias nocapture dereferenceable(128) %arg0) unnamed_addr #0 {
[01:00:07]                                                                                                                                       ^
[01:00:07] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/repr-transparent.ll:151:1: note: possible intended match here
[01:00:07] define void @test_Rgb8Wrap(%Rgb8Wrap* noalias nocapture sret dereferenceable(3), %Rgb8Wrap* byval noalias nocapture dereferenceable(3) %arg0) unnamed_addr #0 {
[01:00:07] ^
[01:00:07] 
[01:00:07] ------------------------------------------
[01:00:07] 
[01:00:07] thread '[codegen] codegen/repr-transparent.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2783:9
[01:00:07] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:00:07] 
[01:00:07] 
[01:00:07] failures:
[01:00:07]     [codegen] codegen/repr-transparent.rs
[01:00:07] 
[01:00:07] test result: �[31mFAILED�(B�[m. 54 passed; 1 failed; 6 ignored; 0 measured; 0 filtered out

hanna-kruppe · 2018-01-07T13:38:28Z

Nah, it works just fine, that's just how newtypes around aggregates are currently handled. I just had a brainfart in the cfgs: This part of the test should apply to x86_64 Linux but I wrote target_arch="x86". I'll push a fix in a second.

hanna-kruppe · 2018-01-07T14:24:45Z

@bors r=eddyb

hanna-kruppe · 2018-01-15T13:43:42Z

@bors r=eddyb

bors · 2018-01-15T13:43:43Z

📌 Commit 46dad65 has been approved by eddyb

bors · 2018-01-16T05:07:28Z

⌛ Testing commit 46dad65f90ce9cff0ae84c1337f7f7718bc05e4b with merge b08302f63c3dca543fb9b5180f4cf2b697990cb4...

bors · 2018-01-16T05:52:16Z

💔 Test failed - status-travis

hanna-kruppe · 2018-01-16T08:31:54Z

@bors r=eddyb

bors · 2018-01-16T08:31:54Z

📌 Commit 2be697b has been approved by eddyb

bors · 2018-01-16T14:34:20Z

⌛ Testing commit 2be697b with merge 3f82c9163c4728c7f540ca89c4cedb35ce13804b...

bors · 2018-01-16T17:36:51Z

💔 Test failed - status-appveyor

kennytm · 2018-01-16T17:44:44Z

@bors retry #46903

shepmaster · 2018-01-20T18:59:19Z

@bors retry

shepmaster · 2018-01-20T19:14:40Z

@bors clean retry

bors · 2018-01-22T08:10:48Z

⌛ Testing commit 2be697b with merge b887317...

@eddyb

Implement repr(transparent) r? @eddyb for the functional changes. The bulk of the PR is error messages and docs, might be good to have a doc person look over those. cc #43036 cc @nox

bors · 2018-01-22T11:11:38Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing b887317 to master...

Slightly more complicated: also give them appropriate names that somewhat describe the cases they are trying to cover, using information from PR chatter in rust-lang#47158

rust-highfive assigned eddyb Jan 3, 2018

eddyb reviewed Jan 3, 2018

View reviewed changes

hanna-kruppe force-pushed the repr-transparent branch from 7bbb3d6 to c2ec5d1 Compare January 3, 2018 20:14

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 3, 2018

hanna-kruppe force-pushed the repr-transparent branch from c2ec5d1 to d62764d Compare January 4, 2018 01:22

hanna-kruppe force-pushed the repr-transparent branch from d62764d to 697c390 Compare January 4, 2018 11:10

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2018

hanna-kruppe force-pushed the repr-transparent branch from 697c390 to 4e1d212 Compare January 7, 2018 10:21

kennytm 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 7, 2018

hanna-kruppe force-pushed the repr-transparent branch from 4e1d212 to 6a005eb Compare January 7, 2018 14:24

hanna-kruppe force-pushed the repr-transparent branch from 7456fe1 to 46dad65 Compare January 15, 2018 13:43

Implement repr(transparent)

2be697b

hanna-kruppe force-pushed the repr-transparent branch from 46dad65 to 2be697b Compare January 16, 2018 08:31

kennytm 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 Jan 16, 2018

bors merged commit 2be697b into rust-lang:master Jan 22, 2018

This was referenced Jan 22, 2018

On missing method do not suggest private traits #47534

Merged

Stabilized #[repr(align(x))] attribute (RFC 1358) #47006

Merged

hanna-kruppe mentioned this pull request Jan 25, 2018

Amend RFC 1758 (repr(transparent)) w.r.t. repr(align) rust-lang/rfcs#2271

Merged

aturon mentioned this pull request Jan 26, 2018

Tracking issue for RFC 1758: Specify repr(transparent) #43036

Closed

3 tasks

tannergooding mentioned this pull request Jan 31, 2020

Expose CLong, CULong, and NFloat interchange types dotnet/runtime#13788

Closed

Implement repr(transparent) #47158

Implement repr(transparent) #47158

Conversation

hanna-kruppe commented Jan 3, 2018

eddyb commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanna-kruppe Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennytm Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jan 3, 2018

frewsxcv commented Jan 3, 2018

hanna-kruppe commented Jan 4, 2018

hanna-kruppe commented Jan 4, 2018

bors commented Jan 4, 2018

bors commented Jan 7, 2018

hanna-kruppe commented Jan 7, 2018

bors commented Jan 7, 2018

bors commented Jan 7, 2018

bors commented Jan 7, 2018

kennytm commented Jan 7, 2018

hanna-kruppe commented Jan 7, 2018

hanna-kruppe commented Jan 7, 2018

hanna-kruppe commented Jan 15, 2018

bors commented Jan 15, 2018

bors commented Jan 16, 2018

bors commented Jan 16, 2018

hanna-kruppe commented Jan 16, 2018

bors commented Jan 16, 2018

bors commented Jan 16, 2018

bors commented Jan 16, 2018

kennytm commented Jan 16, 2018

shepmaster commented Jan 20, 2018

shepmaster commented Jan 20, 2018

bors commented Jan 22, 2018

bors commented Jan 22, 2018

hanna-kruppe Jan 3, 2018 •

edited

Loading

kennytm Jan 3, 2018 •

edited

Loading