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

improper ctypes: normalize return types and transparent structs #72890

Conversation

davidtwco
Copy link
Member

Fixes #66202.

See each commit individually (except the first which adds a test) for more detailed explanations on the changes made.

In summary, this PR ensures that return types are normalized before being checked for FFI-safety, and that transparent newtype wrappers are FFI-safe if the type being wrapped is FFI-safe (often true previously, but not if, after substitution, all types in a transparent newtype were zero sized).

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Jun 1, 2020
This commit adds a test of the improper ctypes lint, checking that
return type are normalized bethat return types are normalized before
being checked for FFI-safety, and that transparent newtype wrappers
are FFI-safe if the type being wrapped is FFI-safe.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-66202-normalize-and-transparent-improper-ctypes branch from f0f13ff to 65149f7 Compare June 9, 2020 09:07
@davidtwco
Copy link
Member Author

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned matthewjasper Jun 9, 2020
if !is_static && self.check_for_array_ty(sp, ty) {
return;
}

// Don't report FFI errors for unit return types - not checking in `check_foreign_fn` so
Copy link
Member

Choose a reason for hiding this comment

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

I can't quite parse this sentence. Is it "we are now checking in"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently this made sense to me when I wrote it - I understand past-me to have meant "we aren't checking in check_foreign_fn (where this check might have been expected) because we want to check after normalization".

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a hopefully clearer comment.

@varkor
Copy link
Member

varkor commented Jun 9, 2020

The changes look good to me. Just want to get clarification on that comment. Also cc @hanna-kruppe, who may have comments.

This commit moves the check that skips unit return types to after
where the return type has been normalized - therefore ensuring that
FFI-safety lints are not emitted for types which normalize to unit.

Signed-off-by: David Wood <david@davidtw.co>
This commit ensures that if a `repr(transparent)` newtype's only
non-zero-sized field is FFI-safe then the newtype is also FFI-safe.

Previously, ZSTs were ignored for the purposes of linting FFI-safety
in transparent structs - thus, only the single non-ZST would be checked
for FFI-safety. However, if the non-zero-sized field is a generic
parameter, and is substituted for a ZST, then the type would be
considered FFI-unsafe (as when every field is thought to be zero-sized,
the type is considered to be "composed only of `PhantomData`" which is
FFI-unsafe).

In this commit, for transparent structs, the non-zero-sized field is
identified (before any substitutions are applied, necessarily) and then
that field's type (now with substitutions) is checked for FFI-safety
(where previously it would have been skipped for being zero-sized in
this case).

To handle the case where the non-zero-sized field is a generic
parameter, which is substituted for `()` (a ZST), and is being used
as a return type - the `FfiUnsafe` result (previously `FfiPhantom`) is
caught and silenced.

Signed-off-by: David Wood <david@davidtw.co>
@davidtwco davidtwco force-pushed the issue-66202-normalize-and-transparent-improper-ctypes branch from 65149f7 to d4d3d7d Compare June 9, 2020 13:37
@varkor
Copy link
Member

varkor commented Jun 9, 2020

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 9, 2020

📌 Commit d4d3d7d has been approved by varkor

@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 Jun 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#72706 (Add windows group to triagebot)
 - rust-lang#72789 (resolve: Do not suggest imports from the same module in which we are resolving)
 - rust-lang#72890 (improper ctypes: normalize return types and transparent structs)
 - rust-lang#72897 (normalize adt fields during structural match checking)
 - rust-lang#73005 (Don't create impl candidates when obligation contains errors)
 - rust-lang#73023 (Remove noisy suggestion of hash_map )
 - rust-lang#73070 (Add regression test for const generic ICE in rust-lang#72819)
 - rust-lang#73157 (Don't lose empty `where` clause when pretty-printing)
 - rust-lang#73184 (Reoder order in which MinGW libs are linked to fix recent breakage)

Failed merges:

r? @ghost
@bors bors merged commit fda594e into rust-lang:master Jun 11, 2020
@davidtwco davidtwco deleted the issue-66202-normalize-and-transparent-improper-ctypes branch June 11, 2020 07:01
davidtwco added a commit to davidtwco/rust that referenced this pull request Jun 19, 2020
This commit applies the changes introduced in rust-lang#72890 to both enum
variants and unions - where the logic prior to rust-lang#72890 was duplicated.

Signed-off-by: David Wood <david@davidtw.co>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Types equal to/wrapping () result in "not FFI-safe" warning
5 participants