lint: change help for pointers to dyn types in FFI by niacdoial · Pull Request #131669 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation84 Commits7 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

niacdoial

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

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

@rustbot

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@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

Oct 13, 2024

@workingjubilee

Hm... "have no C equivalent" means there is no C equivalent.

If void* was considered a valid C interpretation of the type, wouldn't there be a C equivalent?

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.

@workingjubilee

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.

@zachs18

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 T has no C equivalent when someone uses *const T does not actually indicate that *const T is not ffi-safe and is wrong to use1.

I agree that telling users to add another level of indirection without explaining why *const dyn Trait is not ffi-safe is probably not a good idea.

Footnotes

  1. For another example, *const Struct is ffi-safe if Struct: Sized, even if Struct itself is not ffi-safe, because it is represented as a thin pointer. It's perfectly fine to pass through FFI and back, as long as the other side does not try to dereference it. This is not true for *const dyn Trait, since dyn Trait is not Sized, so *const dyn Trait is not itself ffi-safe, which the current diagnostic does not make clear IMO. ↩

@chenyukang

@workingjubilee

Yes, I do agree that part is a strict improvement. The whole of it is why I am asking about the thought process here.

@niacdoial

Yes, that was what confused me originally.

Also I was unaware of the ref<->raw_ptr ABI compatibility guarantee (thank you for that info).
Back then, I sort of assumed something called a "raw pointer" would be guaranteed to be 'the same thing' as a "pointer" in the sense of the warning message used for Box<dyn _>, even if references are allowed to not be 'the same thing' (lifetime info aside). Maybe the raw pointer would contain that added indirection?
I believe I thought that the linter was (effectively) warning me against trying to dereference that pointer from another language.
Again though, I sort of deliberately ignored that message because back when I wrote that example code, my point was trying to understand how closures (and pointers/references to them) worked under the hood, by looking at the generated assembly.

So, for the warning message, instead of (or before) suggesting more indirection, I should maybe add something like the Box<dyn _> warning? Not sure what wording to use to disambiguate "pointer", between the "*_" sense and the sense of that other warning.
maybe "pointer/reference to this type cannot be represented as a single (hardware) pointer" ?

@workingjubilee

Hm? Box<dyn Trait>? That also can be a double-wide pointer? ( I am having trouble finding the specific message you are referring to, somehow. )

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 2 * 2 * 3 * 5 * 7 means.

@niacdoial

Hm? Box? That also can be a double-wide pointer?

given the ABI compatibility rules you linked says it's compatible with a reference or pointer, yes?
In any case, it is linted at compiler/rustc_lint/messages.ftl:L363 and compiler/rustc_lint/src/types.rs:L912

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?

@lcnr

@workingjubilee

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.

hm... "a pointer or reference to a trait object needs to encode information about the object's underlying type."

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

  1. the pointer will be twice as large as *mut u8, or any T: Sized
  2. the bonus data and the pointer has any ordering

@workingjubilee

In the case of trait objects it's a pointer to a virtual function table and the actual pointee.

@workingjubilee

given the ABI compatibility rules you linked says it's compatible with a reference or pointer, yes?

Yes, I know, I was unclear what you were trying to say by referring to "the Box<dyn _> warning." That one doesn't say it's about trait objects specifically, it's trying to lint on any Box<T> where T: !Sized. In this case the linting is correct but the message is... pretty opaque.

@niacdoial

Yes, I know, I was unclear what you were trying to say by referring to "the Box warning."

Yeah, my bad. Shortening "the warning I got when replacing *const dyn _ with Box<dyn _>" into "the Box<dyn _> made the description incorrect.
Yes, I was talking about the Box<T> where T: !Sized warning.

The issue is that any indirection to a dynamically sized type [...] has to also pass this bonus data along.
[...]
In this case the linting is correct but the message is... pretty opaque.

Right, so I guess my changes are too narrow in scope if they only deal with the dyn-based Dynamic Sized Types. (Especially for what I actually implemented so far, which only deals with what is passed by raw pointer.)

Should there be a single block in that match statement, looking for any "indirection to DST" pattern, and constructing a multiline message?
Something starting with "a (pointer|reference|Box|<catch-all for other indirections, if possible>) to a !Sized Type cannot be represented as a single simple pointer, as it has to contain extra metadata.", and continuing with one of these:
"This (pointer|reference|Box|etc) needs to contain metadata about the length of {$slice}"
"This (pointer|reference|Box|etc) needs to contain metadata about the methods of the underlying type of {$dyn_type}"
"This (pointer|reference|Box|etc) needs to contain metadata about the variables closed over by {$closure}"

Would this fit the situation better?

@workingjubilee

Right, so I guess my changes are too narrow in scope if they only deal with the dyn-based Dynamic Sized Types. (Especially for what I actually implemented so far, which only deals with what is passed by raw pointer.)

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.

"This (pointer|reference|Box|etc) needs to contain metadata about the methods of the underlying type of {$dyn_type}"

Probably "the concrete type's implementation of {$dyn_type}"?

@workingjubilee

in any case, to be clear:

Should there be a single block in that match statement, looking for any "indirection to DST" pattern, and constructing a multiline message?

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 *const dyn Trait case.

@niacdoial

not sure how I didn't notice your reply for 20h straight but... here.
Also let me try and remember all the points to consider with my changes...

then, less important:

lint_improper_ctypes_unsized_help_dyn = here, the metadata concerns the concrete implementation of the trait in {$ty}
lint_improper_ctypes_unsized_help_closure = here, the metadata concerns the variables closed over by the closure
lint_improper_ctypes_unsized_help_slice = here, the metadata concerns the length of this slice

...hope this message makes sense, I kinda wrote it progressively between 11pm and 1am, as I was fixing things

@rust-log-analyzer

This comment has been minimized.

@niacdoial

So I've made some changes on my side (will update I updated my previous message to reflect this).
But I'm kinda stuck, because I don't understand how boxes are supposed to behave differently in in extern "ABI" function declarations and function definitions. (And I've tried to find documentation about what this could be caused by.)

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.

@niacdoial

so.
I ended up going ahead and trying to implement what I said in the previous message (the second itemized list).
In the end though, I have to change my mind on that. That comment about precise warnings being good was wrong, because rustc itself relies on *mut TypeWithUnstableLayout not emitting a warning-turned-error.

I'll think a little more on what sort of warning makes sense to have, but I figured I would update you on that.
(...especially since I ended up changing the diagnostic infrastructure for improper_ctypes_definitions and improper_ctypes_declarations specifically, so we now can stack an arbitrary amount of "help" and "note" lines)

@niacdoial

ok I think I have a better understanding of what's at hand now.
I've put this comment in the code dealing with it:

                    // there's a nuance on what this lint should do for function definitions
                    // (touched upon in https://github.com/rust-lang/rust/issues/66220 and https://github.com/rust-lang/rust/pull/72700)
                    //
                    // (`extern "C" fn fn_name(...) {...}`) versus declarations (`extern "C" {fn fn_name(...);}`).
                    // The big question is: what does "ABI safety" mean? if you have something translated to a C pointer
                    // (which has a stable layout) but points to FFI-unsafe type, is it safe?
                    // on one hand, the function's ABI will match that of a similar C-declared function API,
                    // on the other, dereferencing the pointer in not-rust will be painful.
                    // In this code, the opinion is split between function declarations and function definitions.
                    // For declarations, we see this as unsafe, but for definitions, we see this as safe.
                    // This is mostly because, for extern function declarations, the actual definition of the function is written somewhere else,
                    // so the fact that a pointer's pointee should be treated as opaque to one side or the other can be explicitely written out.
                    // For extern function definitions, however, both callee and some callers can be written in rust,
                    // so developers need to keep as much typing information as possible.

There's one thing remaining: some errors vanished in tests/ui/lint/lint-ctypes.rs, about declared extern "C" functions using Box<u32> as parameter or return types.
I'm pretty sure the warnings where the box is a parameter should not exist, but I'll let you double-check that.
For the warnings where the box is the returned type, I'm not sure? There's the nonzero guarantee that might be violated, but at the same time rustc_lint::types::ty_is_known_nonnull thinks a box doesn't qualify as non-null when declaring extern "C" functions.

@rustbot rustbot removed the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Dec 6, 2024

@niacdoial

aaaand CI passes! (and local tests for all commits that required manual intervention in the rebase)

@bors r=@workingjubilee

@bors

📌 Commit 02072fd has been approved by workingjubilee

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

Dec 6, 2024

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Dec 7, 2024

@workingjubilee

…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

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 7, 2024

@bors

…kingjubilee

Rollup of 11 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 7, 2024

@bors

…kingjubilee

Rollup of 11 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 7, 2024

@bors

…kingjubilee

Rollup of 11 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 7, 2024

@bors

…kingjubilee

Rollup of 11 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Dec 8, 2024

@workingjubilee

…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

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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 8, 2024

@bors

…kingjubilee

Rollup of 5 pull requests

Successful merges:

Failed merges:

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 8, 2024

@bors

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Dec 8, 2024

@rust-timer

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

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

jieyouxu

Comment on lines +825 to +830

ty => {
bug!(
"we shouldn't be trying to determine if this is unsized for a reason or another: `{:?}`",
ty
)
}

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.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request

Dec 9, 2024

@jieyouxu

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 9, 2024

@bors

Revert rust-lang#131669 due to ICEs

Revert lint: change help for pointers to dyn types in FFI rust-lang#131669 due to ICE reports:

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)

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 9, 2024

@bors

Revert rust-lang#131669 due to ICEs

Revert lint: change help for pointers to dyn types in FFI rust-lang#131669 due to ICE reports:

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)

bors added a commit to rust-lang-ci/rust that referenced this pull request

Dec 9, 2024

@bors

Revert rust-lang#131669 due to ICEs

Revert lint: change help for pointers to dyn types in FFI rust-lang#131669 due to ICE reports:

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)