Incorrect clashing_extern_decl warning in nightly

[nbdd0121]

PR #70946 introduces new clashing_extern_decl lint which causes incorrect warning even when two extern functions are ABI compatible:

#![allow(dead_code)]

#[repr(transparent)]
struct T(usize);

mod a {
    use super::T;
    extern "C" {
        fn test() -> T;
    }
}

mod b {
    extern "C" {
        fn test() -> usize;
    }
}

Some other example pairs that are ABI compatible that are incorrectly warned

• fn test() -> usize and fn test()
• fn test() -> usize and fn test() -> NonZeroUsize
• fn test() -> usize and fn test() -> Option<NonZeroUsize>

This issue has been assigned to @jumbatm via this comment.

[nbdd0121]

cc @jumbatm @nagisa

[nagisa]

@jumbatm let me know if you are not able to look into this in near future.

[jumbatm]

Thanks for the report. I'll start looking into this tonight.

@rustbot claim

[jumbatm]

Good pickup on the #[repr(transparent)] case -- I'm working on fixing that now.

For the additional pairs you mention, it's not clear to me why they shouldn't warn:

• fn test() -> usize and fn test()

This should be compatible because if the real signature for test is the first signature, and the second is the one supplied to Rust code, it's sound because it simply means the return type won't be read, right?

However, the other way around

// On the C side
void test() { /* ... */ }

and

// In the calling Rust code
extern "C" { fn test() -> usize; }

would let you read uninitialised memory, because the Rust code would look for a return value that the C side never set. It's not enough to assume that the first encountered declaration of the function is the correct one and only warn for a specific order of the clashing declaration, because at any point in the code, both versions of the function may be callable, in different modules-- we only know that at least one of the functions is incorrect. That being said, I definitely see how it's not clear in the current diagnostic why this is linted against, though.

• fn test() -> usize and fn test() -> NonZeroUsize

I'm not sure about these two, because although this would not be unsound from a memory safety perspective, there's still a change in invariance between the two signatures. If the former is correct, then the latter's signature (and the corresponding invariant that the return value is non-zero) is incorrect and needs to be corrected anyway; if the latter's is correct, then there is an additional invariant that correcting the former adds -- a win-win situation for both cases.

• fn test() -> usize and fn test() -> Option<NonZeroUsize>

This one's because the size of both return types is the same, right? This relies on the memory layout optimisation of None being 0, which I can't find any mention of being guaranteed (though, to be fair, I'd imagine is a pretty safe bet). It also needs#[allow(improper_ctypes)], which makes doing this seem a bit smelly. I'd be hesitant to not warn on this case unless None-is-0 is guaranteed (in which case, perhaps improper_ctypes would also need to except this case).

Ah... it seems like what these cases have in common is a differing view on whether clashing_extern_decl should be guarding solely against uninitialised memory reads (which implies to me that this lint would boil down to a simple size + alignment check), or whether the lint should be also guarding against "transmuting" memory reads (which makes these additional cases worth linting against). I'm of the opinion that this lint should also be guarding against transmuting reads, but I'd be keen to hear from any other perspectives.

[nbdd0121]

Good pickup on the #[repr(transparent)] case -- I'm working on fixing that now.

For the additional pairs you mention, it's not clear to me why they shouldn't warn:

• fn test() -> usize and fn test()

This should be compatible because if the real signature for test is the first signature, and the second is the one supplied to Rust code, it's sound because it simply means the return type won't be read, right?

However, the other way around

// On the C side
void test() { /* ... */ }

and

// In the calling Rust code
extern "C" { fn test() -> usize; }

would let you read uninitialised memory, because the Rust code would look for a return value that the C side never set. It's not enough to assume that the first encountered declaration of the function is the correct one and only warn for a specific order of the clashing declaration, because at any point in the code, both versions of the function may be callable, in different modules-- we only know that at least one of the functions is incorrect. That being said, I definitely see how it's not clear in the current diagnostic why this is linted against, though.

Well, we don't know about the actual declaration on the C-side. Even all FFI on the Rust side is compatible, there is no guarantee that it matches the C-side declaration. We are and always will depend on programmer to get that correct.

Because I believe there is an intention for this lint to become a hard error in the future, we will have to conservative when generating it, or we will need to different warnings for these two cases.

• fn test() -> usize and fn test() -> NonZeroUsize

I'm not sure about these two, because although this would not be unsound from a memory safety perspective, there's still a change in invariance between the two signatures. If the former is correct, then the latter's signature (and the corresponding invariant that the return value is non-zero) is incorrect and needs to be corrected anyway; if the latter's is correct, then there is an additional invariant that correcting the former adds -- a win-win situation for both cases.

NonZeroUsize is a wrapper of usize with an attribute to restrict the range. Because C cannot express that, they'll need to be considered compatible on the Rust side. Similar things apply to NonNull pointers.

• fn test() -> usize and fn test() -> Option

This one's because the size of both return types is the same, right? This relies on the memory layout optimisation of None being 0, which I can't find any mention of being guaranteed (though, to be fair, I'd imagine is a pretty safe bet). It also needs#[allow(improper_ctypes)], which makes doing this seem a bit smelly. I'd be hesitant to not warn on this case unless None-is-0 is guaranteed (in which case, perhaps improper_ctypes would also need to except this case).

NonZeroUsize is marked with #[rustc_nonnull_optimization_guaranteed], which guarantees that Option<NonZeroUsize> will always fit in a usize. Similar things apply to any non-nullable pointers.

Ah... it seems like what these cases have in common is a differing view on whether clashing_extern_decl should be guarding solely against uninitialised memory reads (which implies to me that this lint would boil down to a simple size + alignment check), or whether the lint should be also guarding against "transmuting" memory reads (which makes these additional cases worth linting against). I'm of the opinion that this lint should also be guarding against transmuting reads, but I'd be keen to hear from any other perspectives.

Just a note, size and alignment check is insufficient for capturing ABI compatibility issues. For example fn(f32) -> f32 is incompatible with fn(u32) -> u32 on most platforms, so just a size and alignment check is insufficient, and we would need something similar to what you implemented.

[nbdd0121]

One idea, we might define something like

struct ExternDeclCompat {
    Compat,
    Incompat,
    Maybe(GuessedCFunctionType) // We unify and infer the actual C type that would make the two declaration compatible
}

and generate different warning for the incompatible and maybe compatible case.