-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
lint: change help for pointers to dyn types in FFI #131669
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @chenyukang (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Hm... "have no C equivalent" means there is no C equivalent. If Can you explain why you expected one of Rust's raw pointers to a Rust type would have a C equivalent? Because I suspect you are offering the wrong suggestion, if, for instance, you expected the Rust compiler would automatically transform pointers to arbitrary representations in a lossy way that violates our documented rules for compatibility. |
I think suggesting adding another layer of indirection doesn't really help if the real problem is that they want to e.g. interact with the "fields" of the dynamically-sized type's pointer (which are in an unspecified order, even if you put them into memory), so we are best off first making sure we're explaining the problem in a way people can understand. Then maybe suggestions. |
I think changing the note from "trait objects have no C equivalent" to "pointers to trait objects have no C equivalent" (empasis mine) is an improvement here, since that is the type actually being used. This was also the OP's original confusion IIUC; saying I agree that telling users to add another level of indirection without explaining why Footnotes
|
r? compiler |
Yes, I do agree that part is a strict improvement. The whole of it is why I am asking about the thought process here. |
Yes, that was what confused me originally. Also I was unaware of the ref<->raw_ptr ABI compatibility guarantee (thank you for that info). So, for the warning message, instead of (or before) suggesting more indirection, I should maybe add something like the |
Hm? The difficulty here is that what we want the lint to say is basically: "it's a pointer but it's also, like... two pointers, y'know what I mean?" Except uh... more articulately, less like a college student that has just discovered what |
given the ABI compatibility rules you linked says it's compatible with a reference or pointer, yes? hm... "a pointer or reference to a trait object needs to encode information about the object's underlying type. Because of this, it is made of more than a single 'simple' pointer", except with more precise language than "simple", if it exists? Maybe "thin" or "usize-sized" would do the trick? |
r? @workingjubilee as you seem to already be reviewing this |
Unfortunately almost all of the code in the existing ctypes lint should not be cited as a source for "what we should lint on", as it is actively wrong at several points in ways that are difficult to explain without a comprehensive walk-through that will make us be here forever. Partly because there are branches of it we don't actually hit at runtime but should, and branches of it we should never hit but do.
The issue is that any indirection to a dynamically sized type... that's what slices and trait objects have in common... has to also pass this bonus data along. The exact form of this bonus data varies, in fact for slices and trait objects it's different, but it means that
|
In the case of trait objects it's a pointer to a virtual function table and the actual pointee. |
Yes, I know, I was unclear what you were trying to say by referring to "the |
Yeah, my bad. Shortening "the warning I got when replacing
Right, so I guess my changes are too narrow in scope if they only deal with the Should there be a single block in that match statement, looking for any "indirection to DST" pattern, and constructing a multiline message? Would this fit the situation better? |
That's not necessarily the case... I don't want to scope creep your PR unnecessarily... but since the main thing here is choosing a better message that conveys the correct info, we may be best off going ahead and saying something like "this pointer to a Rust unsized type has metadata that makes it incompatible with C pointers", yes.
Probably "the concrete type's implementation of |
in any case, to be clear:
Yes, if you would like to, and I'm happy to accept this with only an improvement that only emits such a message narrowly, for the |
1748f15
to
78fb000
Compare
not sure how I didn't notice your reply for 20h straight but... here.
then, less important:
...hope this message makes sense, I kinda wrote it progressively between 11pm and 1am, as I was fixing things |
This comment has been minimized.
This comment has been minimized.
So I've made some changes on my side (
Anyway, here is what I think I will do, if you're explicitly ok with this:
Something else I've found but don't plan on fixing (in this PR at least) is that, in the big block to look at the FFI-safety of enums, empty enums are considered safe, which (as I understand it) contradicts the nomicon. |
so. I'll think a little more on what sort of warning makes sense to have, but I figured I would update you on that. |
78fb000
to
2f931ef
Compare
ok I think I have a better understanding of what's at hand now.
There's one thing remaining: some errors vanished in |
aaaand CI passes! (and local tests for all commits that required manual intervention in the rebase) |
…rkingjubilee lint: change help for pointers to dyn types in FFI ### Context while playing around, I encountered the warning for dyn types in `extern "C"` functions, but even after that I assumed that a (rust) raw pointer could be interpreted in C ('s ABI) as a `void *`... to be fair part of why I ignored the warning is because I wanted to poke at the generated assembly, not make useful code. ### Example ```rust extern "C" fn caller(callee: *const dyn Fn(i32)->i32){ // -- snip -- } ``` old warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) { | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: trait objects have no C equivalent = note: `#[warn(improper_ctypes_definitions)]` on by default ``` new warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) -> (){ | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: this pointer to an unsized type contains metadata, which makes it incompatible with a C pointer = note: `#[warn(improper_ctypes_definitions)]` on by default ```
…kingjubilee Rollup of 11 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) - rust-lang#133993 (Fix: typo in E0751 error explanation) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 11 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) - rust-lang#133993 (Fix: typo in E0751 error explanation) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 11 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) - rust-lang#133993 (Fix: typo in E0751 error explanation) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 11 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133733 ( compiletest: show the difference between the normalized output and the actual output for lines which didn't match) - rust-lang#133955 (Pass the arch rather than full target name to windows_registry::find_tool) - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) - rust-lang#133993 (Fix: typo in E0751 error explanation) - rust-lang#133996 (Move most tests for `-l` and `#[link(..)]` into `tests/ui/link-native-libs`) r? `@ghost` `@rustbot` modify labels: rollup
…rkingjubilee lint: change help for pointers to dyn types in FFI ### Context while playing around, I encountered the warning for dyn types in `extern "C"` functions, but even after that I assumed that a (rust) raw pointer could be interpreted in C ('s ABI) as a `void *`... to be fair part of why I ignored the warning is because I wanted to poke at the generated assembly, not make useful code. ### Example ```rust extern "C" fn caller(callee: *const dyn Fn(i32)->i32){ // -- snip -- } ``` old warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) { | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: trait objects have no C equivalent = note: `#[warn(improper_ctypes_definitions)]` on by default ``` new warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) -> (){ | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: this pointer to an unsized type contains metadata, which makes it incompatible with a C pointer = note: `#[warn(improper_ctypes_definitions)]` on by default ```
…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133184 (wasi/fs: Improve stopping condition for <ReadDir as Iterator>::next) - rust-lang#133265 (Add a range argument to vec.extract_if) - rust-lang#133456 (Add licenses + Run `cargo update`) - rust-lang#133767 (Add more info on type/trait mismatches for different crate versions) Failed merges: - rust-lang#133522 (Don't suggest restricting bound with unstable traits on stable and mention it's unstable on nightly) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#131669 (lint: change help for pointers to dyn types in FFI) - rust-lang#133104 (crashes: add test for rust-lang#131451) - rust-lang#133767 (Add more info on type/trait mismatches for different crate versions) - rust-lang#133861 (Add allocate_bytes and refactor allocate_str in InterpCx for raw byte…) - rust-lang#133976 (Removed Unnecessary Spaces From RELEASES.md) - rust-lang#133987 (Define acronym for thread local storage) - rust-lang#133992 (Actually walk into lifetimes and attrs in `EarlyContextAndPass`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131669 - niacdoial:linting-ptrdyn-ffi, r=workingjubilee lint: change help for pointers to dyn types in FFI ### Context while playing around, I encountered the warning for dyn types in `extern "C"` functions, but even after that I assumed that a (rust) raw pointer could be interpreted in C ('s ABI) as a `void *`... to be fair part of why I ignored the warning is because I wanted to poke at the generated assembly, not make useful code. ### Example ```rust extern "C" fn caller(callee: *const dyn Fn(i32)->i32){ // -- snip -- } ``` old warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) { | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: trait objects have no C equivalent = note: `#[warn(improper_ctypes_definitions)]` on by default ``` new warning: ``` warning: `extern` fn uses type `dyn Fn(i32) -> i32`, which is not FFI-safe --> file/name.rs:42:19 | 42 | fn caller(callee: *const dyn Fn(i32)->i32) -> (){ | ^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe | = note: this pointer to an unsized type contains metadata, which makes it incompatible with a C pointer = note: `#[warn(improper_ctypes_definitions)]` on by default ```
ty => { | ||
bug!( | ||
"we shouldn't be trying to determine if this is unsized for a reason or another: `{:?}`", | ||
ty | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should not be using a wildcard match, and we really ought to be exhaustively matching here. I looked at the tcx is_sized
logic above, and it's not quite trivial.
Revert <rust-lang#131669> due to ICE reports: - <rust-lang#134059> (real-world) - <rust-lang#134060> (fuzzing) The changes can be re-landed with those cases addressed. This reverts commit 703bb98, reversing changes made to f415c07.
Revert rust-lang#131669 due to ICEs Revert [lint: change help for pointers to dyn types in FFI rust-lang#131669](rust-lang#131669) due to ICE reports: - <rust-lang#134059> (real-world) - <rust-lang#134060> (fuzzing) Closes rust-lang#134060. The revert criteria I used to assess whether to post this revert was: 1. It's not trivial to fix-forward. (1) The implementation itself is tricky due to `tcx.is_sized` query not being very trivial. (2) It will need more extensive test coverage for different ty kinds. 2. It is impacting real-world crates, i.e. rust-lang#134059. 3. `improper_ctypes_definitions` is a warn-by-default lint. This revert is without prejudice to relanding the changes. The changes can be re-landed with those cases addressed and stronger test coverage. A rough regression test corresponding to the fuzzed example reported in rust-lang#134060 is added to check that the revert worked, it is not sufficient for the lint test coverage when the lint improvements are to be relanded. Please feel free to improve the test in the reland. r? `@workingjubilee` (or compiler) cc `@niacdoial` (PR author)
Revert rust-lang#131669 due to ICEs Revert [lint: change help for pointers to dyn types in FFI rust-lang#131669](rust-lang#131669) due to ICE reports: - <rust-lang#134059> (real-world) - <rust-lang#134060> (fuzzing) Closes rust-lang#134060. The revert criteria I used to assess whether to post this revert was: 1. It's not trivial to fix-forward. (1) The implementation itself is tricky due to `tcx.is_sized` query not being very trivial. (2) It will need more extensive test coverage for different ty kinds. 2. It is impacting real-world crates, i.e. rust-lang#134059. 3. `improper_ctypes_definitions` is a warn-by-default lint. This revert is without prejudice to relanding the changes. The changes can be re-landed with those cases addressed and stronger test coverage. A rough regression test corresponding to the fuzzed example reported in rust-lang#134060 is added to check that the revert worked, it is not sufficient for the lint test coverage when the lint improvements are to be relanded. Please feel free to improve the test in the reland. r? `@workingjubilee` (or compiler) cc `@niacdoial` (PR author)
Revert rust-lang#131669 due to ICEs Revert [lint: change help for pointers to dyn types in FFI rust-lang#131669](rust-lang#131669) due to ICE reports: - <rust-lang#134059> (real-world) - <rust-lang#134060> (fuzzing) Closes rust-lang#134060. The revert criteria I used to assess whether to post this revert was: 1. It's not trivial to fix-forward. (1) The implementation itself is tricky due to `tcx.is_sized` query not being very trivial. (2) It will need more extensive test coverage for different ty kinds. 2. It is impacting real-world crates, i.e. rust-lang#134059. 3. `improper_ctypes_definitions` is a warn-by-default lint. This revert is without prejudice to relanding the changes. The changes can be re-landed with those cases addressed and stronger test coverage. A rough regression test corresponding to the fuzzed example reported in rust-lang#134060 is added to check that the revert worked, it is not sufficient for the lint test coverage when the lint improvements are to be relanded. Please feel free to improve the test in the reland. r? `@workingjubilee` (or compiler) cc `@niacdoial` (PR author)
Context
while playing around, I encountered the warning for dyn types in
extern "C"
functions, but even after that I assumed that a (rust) raw pointer could be interpreted in C ('s ABI) as avoid *
... to be fair part of why I ignored the warning is because I wanted to poke at the generated assembly, not make useful code.Example
old warning:
new warning: