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 repr(transparent) #47158

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Conversation

hanna-kruppe
Copy link
Contributor

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
Copy link
Member

eddyb commented Jan 3, 2018

cc @steveklabnik @nikomatsakis

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

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

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.

@kennytm 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
@eddyb
Copy link
Member

eddyb commented Jan 3, 2018

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

@frewsxcv
Copy link
Member

frewsxcv commented Jan 3, 2018

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

@hanna-kruppe
Copy link
Contributor Author

Doc tests should be fixed.

@hanna-kruppe
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jan 4, 2018

📌 Commit 697c390 has been approved by eddyb

@kennytm 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
@bors
Copy link
Contributor

bors commented Jan 7, 2018

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

@hanna-kruppe
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jan 7, 2018

📌 Commit 4e1d212 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jan 7, 2018

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

@bors
Copy link
Contributor

bors commented Jan 7, 2018

💔 Test failed - status-travis

@kennytm 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
@kennytm
Copy link
Member

kennytm commented Jan 7, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@bors r=eddyb

@hanna-kruppe
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jan 15, 2018

📌 Commit 46dad65 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jan 16, 2018

⌛ Testing commit 46dad65f90ce9cff0ae84c1337f7f7718bc05e4b with merge b08302f63c3dca543fb9b5180f4cf2b697990cb4...

@bors
Copy link
Contributor

bors commented Jan 16, 2018

💔 Test failed - status-travis

@hanna-kruppe
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jan 16, 2018

📌 Commit 2be697b has been approved by eddyb

@kennytm 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
Copy link
Contributor

bors commented Jan 16, 2018

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

@bors
Copy link
Contributor

bors commented Jan 16, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 16, 2018

@bors retry #46903

@shepmaster
Copy link
Member

@bors retry

@shepmaster
Copy link
Member

@bors clean retry

@bors
Copy link
Contributor

bors commented Jan 22, 2018

⌛ Testing commit 2be697b with merge b887317...

bors added a commit that referenced this pull request Jan 22, 2018
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
Copy link
Contributor

bors commented Jan 22, 2018

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

@bors bors merged commit 2be697b into rust-lang:master Jan 22, 2018
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jul 29, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants