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

lint: change help for pointers to dyn types in FFI #131669

Merged
merged 7 commits into from
Dec 8, 2024

Conversation

niacdoial
Copy link
Contributor

@niacdoial niacdoial commented Oct 13, 2024

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
Copy link
Collaborator

rustbot commented Oct 13, 2024

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 author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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
Copy link
Member

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
Copy link
Member

workingjubilee commented Oct 14, 2024

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
Copy link
Contributor

zachs18 commented Oct 14, 2024

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
Copy link
Member

r? compiler

@rustbot rustbot assigned lcnr and unassigned chenyukang Oct 14, 2024
@workingjubilee
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

workingjubilee commented Oct 14, 2024

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
Copy link
Contributor Author

niacdoial commented Oct 14, 2024

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
Copy link
Contributor

lcnr commented Oct 15, 2024

r? @workingjubilee as you seem to already be reviewing this

@rustbot rustbot assigned workingjubilee and unassigned lcnr Oct 15, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 15, 2024

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
Copy link
Member

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

@workingjubilee
Copy link
Member

workingjubilee commented Oct 15, 2024

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
Copy link
Contributor Author

niacdoial commented Oct 15, 2024

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
Copy link
Member

workingjubilee commented Oct 16, 2024

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
Copy link
Member

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
Copy link
Contributor Author

niacdoial commented Oct 17, 2024

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...

  • The most important part here is that I don't understand what the difference between function declarations and definitions is supposed to do in this (since it apparently was already one of the criteria to determine if Box<_> is FFI-safe).
    (there's actually a test that I didn't fix because I'm not sure if the test or the code is wrong on this one)
  • I still have to re-add the help messages that no longer show up (another test file will fail until I deal with this properly) [edit: done in unpushed changes]
  • I might have caused a regression on issue #⁠96621?? (a third test file failed) [edit: wrong, that test also fails before my changes]

then, less important:

  • I can't make a 2-line message like I planned, not without restructuring that file's diagnostic tooling (FfiResult and the like) to allow extra "note" entries.
    In my current draft, they would look like this, because the current diagnostic architecture doesn't let me refer to inner_ty in these strings.
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
  • I tried to follow what is the linter already considered safe/unsafe, but there are bound to have changes, if for some reason a !Sized pointee type used to be considered safe
  • Also, I remembered RFC 1861, added ty::Foreign(..) as a safe pointee, even if it's unsized (I hope I understood what ty::Foreign(..) means correctly)

...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
Copy link
Contributor Author

niacdoial commented Oct 21, 2024

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.)

  • Box seems to be the only place where this distinction is made (at least before my changes), both for FFI safety and for nonnull optimisation
  • OK, there's one exception, but that's type parameters being allowed in definitions but unexpected in definitions.
  • For function definitions (before my changes), any Box<T> where T: Sized would be considered FFI-safe even if the pointee has an unknown layout.
  • For function declarations, there is no special-casing of boxes, and all boxes (even the ones that seem safe) hit the "this struct has unspecified layout" lint.
  • This has implications for one of the test files that for now unfixed (tests/ui/lint/lint-ctypes-cstr.rs), because &CStrings to be considered safe in function definitions. (Somehow, for &CString in particular, not warning against its use looks like a footgun, given what they are made of.)

Anyway, here is what I think I will do, if you're explicitly ok with this:

  • first, remove the definition/declaration distinction for boxes, refs and pointers, at least when checking for this safety.
  • then, remove the ability to say that a box is FFI-safe no matter what the pointee is (if the warning is precise enough to say that the pointee type is the one that's FFI-unsafe, then the user will likely understand problems will only appear when dereferencing that pointer)

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@bors r=@workingjubilee

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 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
…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
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2024
…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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 8, 2024
…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
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2024
…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
@bors bors merged commit 703bb98 into rust-lang:master Dec 8, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2024
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
```
@niacdoial niacdoial deleted the linting-ptrdyn-ffi branch December 8, 2024 17:27
@jieyouxu jieyouxu added the L-improper_ctypes_definitions Lint: improper_ctypes_definitions label Dec 9, 2024
@jieyouxu jieyouxu mentioned this pull request Dec 9, 2024
4 tasks
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
)
}
Copy link
Member

@jieyouxu jieyouxu Dec 9, 2024

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
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
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)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L-improper_ctypes_definitions Lint: improper_ctypes_definitions 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
Development

Successfully merging this pull request may close these issues.

9 participants