improper_ctypes lint should check types referenced in C ABI function definitions

[tomjakubowski]

For example, this compiles with no warnings or errors:

#![feature(lang_items)]
#![crate_type="staticlib"]
#![allow(missing_copy_implementations)]
#![deny(improper_ctypes)]
#![no_std]

pub struct Blah {
    pub x: i32
}

#[no_mangle]
pub extern "C" fn yikes(b: Blah) -> i32 {
    b.x
}

#[lang = "copy"] trait Copy {}
#[lang = "sized"] trait Sized {}
#[lang = "stack_exhausted"] extern fn stack_exhausted() {}
#[lang = "eh_personality"] extern fn eh_personality() {}
#[lang = "panic_fmt"] fn panic_fmt() -> ! { loop {} }

Even though Blah lacks the #[repr(C)] attribute.

[tomjakubowski]

To clarify: foreign functions (fn inside an extern block) are checked. Regular functions with a C ABI are not.

[DanielKeep]

This is still an issue. It just came up in a Stack Overflow question, with something roughly equivalent to this:

#[no_mangle]
pub extern fn print(s: String) {
    println!("{}", s);
}

This produces no warnings. For context, the user was trying to bind to this function from C#, which went about as well as you'd expect.

[arielb1]

@DanielKeep

Its even worse here - this is an extern "Rust" function, and these have their own ABI.

[TimNN]

@arielb1 https://github.com/rust-lang/rust/blob/master/src/doc/trpl/ffi.md#calling-rust-code-from-c seems to imply that a pub extern fn something() {} uses the C calling convention by default.

Edit: and the reference seems to agree: https://github.com/rust-lang/rust/blob/master/src/doc/reference.md#extern-functions

[shepmaster]

Continues to be a problem for people new to FFI:

#[no_mangle]
pub extern "C" fn hello(cs: CString) -> CString {
  // ...
}

The OP states:

Well, my reasoning is that if I can pass int32 back and forth + if CString can easily go in and out (commented code), then it should work.

If this generated a warning, there's more of a chance people might not do this.

[shepmaster]

Should we start some kind of RFC for this?

/cc @nagisa from comment

[nagisa]

See also #36464. A comment in that issue explains why extern fn stuff(with: NonCTypes) is completely valid code.

[tomjakubowski]

What's the use case for that example? If it is a niche case, I think the cost of adding a #[allow(improper_ctypes)] for the rare case is worth catching bugs for the more common case: a Rust library which also exposes a C-compatible interface.

[nagisa]

Lints in rustc shouldn’t have any false positives even in niche cases. Clippy has all the false positives you might want.

[retep998]

I'm a firm believer that the lint should fire in this case. That niche usage does not mean we should let this massive footgun go by without any warning at all. In fact, if you so believe in that niche usage, find some examples in real crates that do that using non-repr(C) types.

[hanna-kruppe]

Would it be more acceptable for the lint to only apply to #[no_mangle] extern functions instead of all extern ones? That would cover the most common way to expose Rust functions to C (tough it would miss functions that are called from C through function pointers), while not firing on the hypothetical Rust function that has a C calling convention but is only called from Rust.

[retep998]

@rkruppe In my opinion function pointers passed to C are much more common than Rust code calling other Rust functions that happen to use the C calling convention.

[hanna-kruppe]

I agree. But #[no_mangle] extern functions are common enough that it's worthwhile warning about non-repr(C) types in them, even if the "Rust calling Rust with C ABI" objection rules out warning about all extern fns with non-repr(C) types.

Of course, if that objection is overruled, that would be great as well, but if not, let's try to at least catch some cases.

[shepmaster]

Lints in rustc shouldn’t have any false positives even in niche cases.

I agree with the spirit of this statement, but not the absolute degree. Lints that are built into rustc and enabled by default should have a very low false positive rate, but not zero.

Discussing with @nagisa, I think that our understanding of the role of lints has changed a little bit in the last 2+ years:

nagisa: we lint against extern "C" { fn foo(x: String); }
nagisa: which makes sense because we are about 100% sure this function is being imported across FFI.
nagisa: We should perhaps lint against defining functions like [extern "C" fn foo(_: String) {}], with perhaps a lint that is different from the current one.
[...]
nagisa: Right, because extern "C" { fn ... } is also not quite 100% false-positive free.

I'd like to propose that we take the following course of action:

1. Add a new lint that warns about using non -repr(C) structs in any extern "C" function defined in Rust code.
2. Run it through crater.
3. Based on crater feedback, maybe incorporate the no_mangle restriction.

Then, bikesheddable steps:

4. Rename the current lint and put in a lint group as the current name for both lints.

[shepmaster]

I've never done a lint before, but I might be able to try and do this one, if someone had any mentoring steps to provide. (or if you just do it, that's cool too!)

[varkor]

@shepmaster:
The code for linting for improper_ctypes is found in src/librustc_lint/types.rs. Specifically, the part here is the part that needs changing:
https://github.com/rust-lang/rust/blob/a9b907b5fae0c1cbf3447091746886b41ea374a5/src/librustc_lint/types.rs#L804-L820
The LateLintPass implementation has a check_foreign_item definition, which will end up calling the functions triggering the lint. However, extern "C" fn doesn't count as a foreign item (because it's not in an extern block).

You'll want to implement the check_fn method for ImproperCTypes. You should then be able to do something very similar to check_foreign_item, which should end up triggering the lints. (The check_type_for_ffi_and_report_errors function is the one that triggers the lints, so you can trace back from there if this doesn't work straight off.)

hir::FnHeader's abi field might be helpful:
https://github.com/rust-lang/rust/blob/a9b907b5fae0c1cbf3447091746886b41ea374a5/src/librustc/hir/mod.rs#L2236-L2242

[varkor]

@shepmaster: have you had a chance to try implementing this yet?

[shepmaster]

I have not... it's just been sitting in a tab, waiting, lonely. If someone wants to steal it, please feel free!

[davidtwco]

@varkor @shepmaster I had a go at implementing the instructions in issue-19834-improper-ctypes-in-extern-C-fn, but quickly ran into problems:

• I had to #[allow(improper_ctypes)] on a bunch of places within the libproc_macro, libstd, libpanic_abort and libpanic_unwind.
• typeck currently only prohibits generics on foreign functions, so by allowing improper_ctypes on regular functions (that have extern "C"), you quickly run into something like ICE: src/librustc_lint/types.rs:858: unexpected type in foreign function: T #65035 - I added basic support for generics to the improper_ctypes lint so I could #[allow(improper_ctypes)] away the remaining errors.

The branch doesn't fail any tests at the moment. Do either of you think it's worth continuing with this and opening a PR?

[hanna-kruppe]

Neither of these problems is a showstopper in my opinion, please continue and open a PR. Also, I have comments about both of the issues you identified and how you addressed them but it would probably best to discuss that on the PR.

[davidtwco]

@rkruppe opened as #65134

[davidtwco]

Closing, extern fns are handled in #72700.