Skip to content

Mark ZST as FFI-safe if all its fields are PhantomData #106675

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2023
Merged

Mark ZST as FFI-safe if all its fields are PhantomData #106675

merged 1 commit into from
Jan 13, 2023

Conversation

krtab
Copy link
Contributor

@krtab krtab commented Jan 10, 2023 •
edited
Loading

This presents one possible solution to issue: #106629.

This is my first (tentative) contribution to the compiler itself.

I'm looking forward for comments and feedback

Closes: #106629

@rustbot
Copy link
Collaborator

rustbot commented Jan 10, 2023

r? @davidtwco

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 10, 2023
@krtab krtab marked this pull request as draft January 10, 2023 10:48
@krtab krtab changed the title Draft: Mark ZST as FFI-safe if all its fields are PhantomData Mark ZST as FFI-safe if all its fields are PhantomData Jan 10, 2023
davidtwco
davidtwco requested changes Jan 10, 2023
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

This is a good start, thanks for the contribution, left some comments below.

@davidtwco
Copy link
Member

@rustbot author (use @rustbot ready when you want me to look again)

@rustbot rustbot 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 Jan 10, 2023
@krtab krtab force-pushed the fix_improper_ctypes branch from 0272d23 to 27c5684 Compare January 10, 2023 13:52
@krtab
Copy link
Contributor Author

krtab commented Jan 10, 2023

I think the new version takes into account your points 2 and 3, and I answered 1 above. LMKWYT
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2023
@krtab krtab force-pushed the fix_improper_ctypes branch from 27c5684 to 50b53ca Compare January 10, 2023 15:11
@krtab
Copy link
Contributor Author

krtab commented Jan 10, 2023

@rustbot ready

@krtab krtab force-pushed the fix_improper_ctypes branch from 50b53ca to f5ab431 Compare January 10, 2023 15:13
@davidtwco
Copy link
Member

There's a test failing, after you fix that then I can approve :)

@krtab
Copy link
Contributor Author

krtab commented Jan 10, 2023

This is a bit above my pay-grade:

The test failing is basically:

#[repr(transparent)]
enum TransparentEnum<T> {
    Variant(T, std::marker::PhantomData<Z>),
}

which fails because it is an enum with a PhantomData, triggering this code path
https://github.com/rust-lang/rust/blob/f5ab431cb7bd2dbf2aa50daa187c8e53ba1419f8/compiler/rustc_lint/src/types.rs#L901-L907

I'm not sure which one is the correct behavior. Should it have been considered not FFI safe since the beginning, or is this PR introducing a new false positive?

@krtab
Copy link
Contributor Author

krtab commented Jan 10, 2023

I've pushed a fix for the failing test but I'll leave it to you to decide which is the correct behavior.

@rustbot ready

davidtwco
davidtwco reviewed Jan 11, 2023
View reviewed changes
@krtab
Copy link
Contributor Author

krtab commented Jan 11, 2023 •
edited
Loading

It this is ok for you, I guess all is left is for me to squash that into a single commit and mark the PR as not draft.

@davidtwco
Copy link
Member

It this is ok for you, I guess all is left is for me to squash that into a single commit and mark the PR as not draft.

Works for me :)

@krtab krtab force-pushed the fix_improper_ctypes branch from 1722028 to 574a31f Compare January 11, 2023 15:46
@krtab krtab marked this pull request as ready for review January 11, 2023 15:46
@krtab
Copy link
Contributor Author

krtab commented Jan 11, 2023

Looks like we're good to go!
Thanks a lot for the reviewing work, and cheers to my first contribution to rustc! 🥳

@davidtwco
Copy link
Member

In fact, one last thing, can you add a test case for #106629?

@krtab krtab force-pushed the fix_improper_ctypes branch from 574a31f to 27a4e17 Compare January 12, 2023 10:44
@krtab
Copy link
Contributor Author

krtab commented Jan 12, 2023

My bad, I also forgot to do it!
Should be ok now.

@davidtwco
Copy link
Member

My bad, I also forgot to do it! Should be ok now.

Tests were moved from src/test to tests/ recently, can you rebase and change it to that? CI will fail otherwise.

@krtab
Mark ZST as FFI-safe if all its fields are PhantomData
797f247 
Modify the linting behavior and add the corresponding
regression test
@krtab krtab force-pushed the fix_improper_ctypes branch from 27a4e17 to 797f247 Compare January 12, 2023 12:43
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 12, 2023

📌 Commit 797f247 has been approved by davidtwco

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2023
@JohnTitor JohnTitor mentioned this pull request Jan 12, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 12, 2023
@bors
Auto merge of rust-lang#106786 - JohnTitor:rollup-8f4vk8m, r=JohnTitor
86ad69d 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#105795 (Stabilize `abi_efiapi` feature)
 - rust-lang#106446 ([LSDA] Take ttype_index into account when taking unwind action)
 - rust-lang#106675 (Mark ZST as FFI-safe if all its fields are PhantomData)
 - rust-lang#106740 (Adding a hint on iterator type errors)
 - rust-lang#106741 (Fix reexport of `doc(hidden)` item)
 - rust-lang#106759 (Revert "Make nested RPITIT inherit the parent opaque's generics.")
 - rust-lang#106772 (Re-add mw to review rotation)
 - rust-lang#106778 (Exclude formatting commit from blame)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 19ba430 into rust-lang:master Jan 13, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 13, 2023
@oxalica oxalica mentioned this pull request Jul 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.

improper_ctypes lint fires on #[repr(transparent)] wrappers around PhantomData (in struct fields).
4 participants
@krtab @rustbot @davidtwco @bors