Add by-value arrays to improper_ctypes lint
#66305
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
elichai
force-pushed
the
2019-11-array_ffi
branch
from
c5cab5e
to
c9298b4
Compare
|
Don't really have the time to review this but from a quick glance: AFAICT this won't interact as desired with transparent newtypes (e.g., passing |
|
Right I forgot I wanted to ask about transparent structs. although technically with a single value struct I'm pretty sure |
elichai
force-pushed
the
2019-11-array_ffi
branch
from
c9298b4
to
848b3ec
Compare
|
C structs with a single field have the same memory layout as their field, but they are not always treated the same by calling conventions. The whole point of repr(transparent) is to ensure that, and calling convention is a key consideration for this lint, so it seems pretty clear-cut to me that transparent newtypes should get the exact same treatment as the type they wrap. |
|
@rkruppe anyhow. I don't mind adding the lint for transparent too, but if there's an inherent problem with the linting of |
I should note that this is entirely coincidental, and depends on the architecture. This is just like |
Thanks! Good to know. |
src/librustc_lint/types.rs
Outdated
| reason: "passing raw arrays by value is not FFI-safe", | ||
| help: Some("consider passing a pointer to the array"), | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's really easy to poison the cache, so maybe add a method that wraps check_type_for_ffi (say, check_top_level_type_for_ffi?) and checks for ty::Array before calling check_type_for_ffi?
There was a problem hiding this comment.
Hmm, the point of the cache is to prevent infinite recursion.
I'm not sure I'm seeing how is this any different than other checks.
There was a problem hiding this comment.
top_level isn't in the hash key, so whichever comes first will stay in the cache and affect any other occurrences of the same type.
Also, overall the diff would be simpler if the top-level check was separate, and check_type_for_ffi didn't have an extra parameter.
There was a problem hiding this comment.
Updated the name of the inner one, otherwise calling check_top_level_type_for_ffi would've been misleading, because it checks both top level and lower levels.
|
☔ The latest upstream changes (presumably #66378) made this pull request unmergeable. Please resolve the merge conflicts. |
elichai
force-pushed
the
2019-11-array_ffi
branch
2 times, most recently
from
61b1517
to
159aa11
Compare
JohnCSimon
added
S-waiting-on-author
elichai
force-pushed
the
2019-11-array_ffi
branch
from
159aa11
to
9f97525
Compare
elichai
force-pushed
the
2019-11-array_ffi
branch
from
9f97525
to
de1532f
Compare
elichai
force-pushed
the
2019-11-array_ffi
branch
from
de1532f
to
b3666b6
Compare
|
📌 Commit b3666b6 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit b3666b6 with merge 4fc8f2ced225b73fc28307b7c96ed0880dd707e3... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
I don't think this error is related to the PR here |
|
@bors retry (spurious network error?) |
bors
added
S-waiting-on-bors
tmandry
added
A-spurious
Add by-value arrays to `improper_ctypes` lint Hi, C doesn't have a notion of passing arrays by value, only by reference/pointer. Rust currently will pass it correctly by reference by it looks very misleading, and can confuse the borrow checker to think a move had occurred. Fixes rust-lang#58905 and fixes rust-lang#24578. We could also improve the borrow checker here but I think it's kinda a waste of work if we instead just tell the user it's an invalid FFI call. (My first PR to `rustc` so if I missed some test or formatting guideline please tell me :) )
Rollup of 17 pull requests Successful merges: - #64325 (Stabilize nested self receivers in 1.41.0) - #66222 (Use `eq_opaque_type_and_type` when type-checking closure signatures) - #66305 (Add by-value arrays to `improper_ctypes` lint) - #66399 (rustc_metadata: simplify the interactions between Lazy and Table.) - #66534 (Allow global references via ForeignItem and Item for the same symbol name during LLVM codegen) - #66700 (Fix pointing at arg for fulfillment errors in function calls) - #66704 (Intra doc enum variant field) - #66718 (Refactor `parse_enum_item` to use `parse_delim_comma_seq`) - #66722 (Handle non_exhaustive in borrow checking) - #66744 (Fix shrink_to panic documentation) - #66761 (Use LLVMDisposePassManager instead of raw delete in rustllvm) - #66769 (Add core::{f32,f64}::consts::TAU.) - #66774 (Clean up error codes) - #66777 (Put back tidy check on error codes) - #66797 (Fixes small typo in array docs r? @steveklabnik) - #66798 (Fix spelling typos) - #66800 (Combine similar tests for const match) Failed merges: r? @ghost
|
I'm guilty of assuming this would "just work" and just got hit by this warning. How dangerous would it be to ignore this warning? Is it likely that array layout is at some point going to be defined and officially usable in FFI? |
|
@jplatte no. |
|
@jplatte If you (want to) have a If we are ever going to define
That's more of an ABI detail, rather than an optimization, and it's most likely to happen for types where e.g. in C |
Sorry for being unclear about what it is I'm currently doing.
This is in fact what I'm doing currently. I've got Thanks for the very quick reply! |
|
AFAIU |
|
With const generics this should work: #[repr(C)]
struct CppArray<T, const N: usize> {
data: [T; N],
} |
Hi,
C doesn't have a notion of passing arrays by value, only by reference/pointer.
Rust currently will pass it correctly by reference by it looks very misleading, and can confuse the borrow checker to think a move had occurred.
Fixes #58905 and fixes #24578.
We could also improve the borrow checker here but I think it's kinda a waste of work if we instead just tell the user it's an invalid FFI call.
(My first PR to
rustcso if I missed some test or formatting guideline please tell me :) )