Multiple incompatible clashing extern fn's should be a hard error (potentially phased in via future-incompat)

[pnkfelix]

(Many thanks to @celinval for alerting me to the existence of this issue.)

I tried this code (playpen):

pub mod a {
    use std::ffi::c_int;
    extern "C" { fn isalpha(_: c_int) -> c_int; }
    pub fn wrap(x: c_int) -> c_int { unsafe { isalpha(x) } }
}

pub mod b {
    use std::ffi::c_int;
    extern "C" { fn isalpha(_: c_int, _: c_int) -> c_int; }
    pub fn wrap(x: c_int, y: c_int) -> c_int { unsafe { isalpha(x, y) } }
}

fn main() {
    dbg!(a::wrap(10));
    dbg!(b::wrap(10, 20));
}

I expected to see this happen: code should be rejected as obviously wrong, since within the same codegen unit, it is using two incompatible declarations for the same external foreign function.

Instead, this happened: the code is accepted by the compiler with just a warning:

warning: `isalpha` redeclared with a different signature
 --> src/main.rs:9:18
  |
3 |     extern "C" { fn isalpha(_: c_int) -> c_int; }
  |                  ------------------------------ `isalpha` previously declared here
...
9 |     extern "C" { fn isalpha(_: c_int, _: c_int) -> c_int; }
  |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this signature doesn't match the previous declaration
  |
  = note: `#[warn(clashing_extern_declarations)]` on by default
  = note: expected `unsafe extern "C" fn(i32) -> i32`
             found `unsafe extern "C" fn(i32, i32) -> i32`

This lint was added in PR #70946; there was an attempt, in PR #75192, to make the lint deny-by-default, but that attempt was abandoned (see associated comment explaining why that was abandoned).

Its important to note in particular that in the case of these clashing declarations, you can see in the generated LLVM-IR that there is truly only one declaration chosen, and the order in which one feeds the mod a and mod b above will dictate which extern fn declaration is selected by LLVM.

(Its possible that we may need a more precise variant of the existing clashing_extern_declarations lint that more precisely identifies the problematic cases.)

I personally suspect that this error represents enough of a red flag that we really should be aiming to turn the existing lint into a hard-error, even if only over an edition boundary for the language.

Meta

rustc --version --verbose:

Stable channel

Build using the Stable version: 1.65.0

Nightly channel

Build using the Nightly version: 1.67.0-nightly (2022-11-30 c97b539e408ea353f4fd)

[pnkfelix]

(To be clear: I don't expect this to be a high priority issue to fix. I think there is an implicit requirement that if you are writing any extern blocks, you have a responsibility to review all such blocks to ensure they make sense and are compatible with the foreign libraries that are linked in. I am mainly objecting to categorizing this as a mere lint, when it seems like it should always, or nearly always, correspond to a huge red flag that we should signal much more strongly.)

[traviscross]

@rustbot labels +I-lang-nominated

Let's discuss whether we want to disallow this in Rust 2024.

[scottmcm]

Instead, this happened: the code is accepted by the compiler with just a warning:

My inclination is that deny-by-default is a good option here -- on old editions too. (Perhaps with a slightly different lint than the one today, if there are concerns about the current implementation.)

If we have to define what's going to happen for multiple incompatible clashing externs across sibling crates (even if that's just "it's UB to use them"), then making it a hard error in some ways makes the spec harder to write, since we have to give the details of what exactly is or isn't detected as a hard error.

Whereas a set of "hey, we noticed you're doing something that's definitely wrong, so it's deny" things that we can update over time -- since it's a lint -- sounds great to me. And would mean that cap-lints would apply, so it doesn't keep people from using dependencies that haven't fixed it.

[madsmtm]

I think this may be a duplicate of https://github.com/rust-lang/rust/issues/12707? At least heavily related to that.