Skip to content

Comments

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

Merged
bors merged 1 commit intorust-lang:masterfrom
krtab:fix_improper_ctypes
Jan 13, 2023
Merged

Mark ZST as FFI-safe if all its fields are PhantomData#106675
bors merged 1 commit intorust-lang:masterfrom
krtab:fix_improper_ctypes

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

davidtwco commented Jan 10, 2023

@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

davidtwco commented Jan 10, 2023

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

davidtwco commented Jan 11, 2023

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

davidtwco commented Jan 12, 2023

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

davidtwco commented Jan 12, 2023

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

davidtwco commented Jan 12, 2023

@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