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

Should improper_ctypes_definitions lint warn against boxed DST? #85714

Closed
marmeladema opened this issue May 26, 2021 · 2 comments · Fixed by #85853
Closed

Should improper_ctypes_definitions lint warn against boxed DST? #85714

marmeladema opened this issue May 26, 2021 · 2 comments · Fixed by #85853
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@marmeladema
Copy link
Contributor

Given the following code:

#![deny(improper_ctypes_definitions)]

#[no_mangle]
pub extern "C" fn test_boxed_str() -> Box<str> {
    "test_boxed_str".into()
}

#[no_mangle]
pub extern "C" fn test_boxed_slice() -> Box<[u8]> {
    b"test_boxed_slice"[..].into()
}

Playground link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dfca164488b7561826bbb61ee137b6bc

The current output is:

  • It compiles successfully

Ideally the output should look like:

warning: `extern` fn uses type `Box<str>`, which is not FFI-safe
  --> src/lib.rs:24:32
   |
24 | pub extern "C" fn test_boxed_str() -> Box<str> {
   |                                       ^^^^^^^^ not FFI-safe
   |
   = note: `#[warn(improper_ctypes_definitions)]` on by default
   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
   = note: this struct has unspecified layout

From what I understand, fat pointers layout is not stable and should not be relied upon, pretty much in the same way that relying on the layout of a #[repr(rust)] type is an internal decision to the compiler. If they are now considered stable ABI, then the current lint is fine.

But in any case, I believe the documentation at https://doc.rust-lang.org/std/boxed/index.html#memory-layout could probably be enhanced to speak more about DST. Either to say they should be avoided because the memory layout is not stable, or to explicitly document that it is and makes this a commitment from the compiler.

If those arguments are compelling and the compiler team agrees to those changes, I am willing to implement them, providing a little bit of mentoring from someone 👍

@marmeladema marmeladema added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 26, 2021
@elichai
Copy link
Contributor

elichai commented May 28, 2021

Hi, I'm not part of the compiler team but IMHO this does deserve a warning, because DST do have unspecified layout,
If you want to give it a go the code for this is in rustc_lint::types::check_type_for_ffi, otherwise I don't mind fixing this myself.

If you do work on this and have questions free to ping me on Zulip/Discord(elichai2)/IRC(libera #rust channel, elichai2).

@marmeladema
Copy link
Contributor Author

I'll try to come up with something but I'll probably ping you for help 🙂 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants