Update improper_ctypes warning to add info about extern "Rust"

[jam1garner]

The Rust ABI, while rarely the right tool for the job due to its unstable nature, has a discoverability problem when it is worth using. For example, in some embedded and/or unconventional platforms, it tends to be helpful for linkage trickery even if it is unstable.

Minimal example:

fn main() {
    unsafe {
        dbg!(test1().unwrap());
    }
}

extern {
    #[link_name = "test"]
    fn test1() -> Result<u32, u32>;
}

#[no_mangle]
fn test() -> Result<u32, u32> {
    Ok(3)
}

In this situation, it may be unclear to some users that not specifying an ABI for the extern block and not specifying an ABI for the exposed #[no_mangle] function will actually lead to a mismatch of ABI due to the fact the default ABIs for these two contexts are different.

The resulting warning:

warning: `extern` block uses type `std::result::Result<u32, u32>`, which is not FFI-safe
 --> src/main.rs:9:19
  |
9 |     fn test1() -> Result<u32, u32>;
  |                   ^^^^^^^^^^^^^^^^ not FFI-safe
  |
  = note: `#[warn(improper_ctypes)]` on by default
  = help: consider adding a `#[repr(C)]`, `#[repr(transparent)]`, or integer `#[repr(...)]` attribute to this enum
  = note: enum has no representation hint

This error doesn't at all allude to the actual problem causing their issue, and has (multiple times, in my experience) lead to a rather dead-in debugging experience. (Maybe it should explicitly say extern defaults to extern "C"?)

While I believe this style of:

1. Not specifying an ABI
2. Abusing #[no_mangle]

is not good practice, it is something I have seen happen multiple times (from myself and from others asking questions) and while extern "Rust" is not recommended, giving the user things (to google, for example) which will lead to the discovery of information about the instability of the Rust ABI and the nature, it will direct users towards the information they need to solve their problem.

While I would argue #[no_mangle] without extern should raise a warning or error (as this would lead to exposing a Rust ABI function being opt-in, which better matches the unstable nature of the Rust ABI), I believe that is a significantly more controversial way to go about improving the user experience around this, hence this PR only being an update to existing warning, however I'm totally open to others' ideas on the subject.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[lcnr]

r? @davidtwco

[davidtwco]

I agree that we should make a change to avoid the confusion here, but I'm not sure that this change is it.

This PR will add the note to every improper_ctypes and improper_ctypes_definitions lint. In most cases, I'd expect the user to have specified the ABI, so this addition would only add confusion (e.g. this message could be interpreted as suggesting that the compiler has determined that you are using the unstable Rust ABI but haven't specified it explicitly, despite having specified another ABI). In fact, this lint doesn't even trigger when using the Rust ABI, so this note would only appear when the user has specified (or was defaulted to) a non-Rust-internal ABI.

Unfortunately, at this point in compilation, we don't know whether the user did not specify extern (and thus defaulted to a Rust ABI), specified only extern (and defaulted to the C ABI), or specified the ABI manually. If we were able to easily determine this, then I'd be happy with adding a note when we defaulted to the C ABI which informs the user that has happened.

As you suggest, a lint/warning/error on #[no_mangle] without extern might be appropriate (I don't know how common this is to say whether or not it makes sense); alternatively, we could keep track of whether the ABI was specified further into the compiler to make this addition contextual.

---

If you want to try and make this contextual, the approach I would take is to introduce a Abi type in src/librustc_hir/hir.rs:

enum AbiSource {
	Explicit,
	Implicit,
	None
}

struct Abi {
    kind: rustc_target::spec::abi::Abi,
    source: AbiSource,
}

Replace rustc_target::spec::abi::Abi's uses in all of src/librustc_hir/hir.rs and src/rustc_middle/ty/sty.rs (all of the HIR and Ty more generally, but it'll be mostly those files) with your new type and create it here:

rust/src/librustc_ast_lowering/item.rs

Lines 1254 to 1260 in e614f66

	pub(super) fn lower_extern(&mut self, ext: Extern) -> abi::Abi {
	match ext {
	Extern::None => abi::Abi::Rust,
	Extern::Implicit => abi::Abi::C,
	Extern::Explicit(abi) => self.lower_abi(abi),
	}
	}

Then, in this lint, adjust callers of check_type_for_ffi_and_report_errors to thread the AbiSource through to the emit_ffi_unsafe_type_lint and then use that to only emit the note that you added here when appropriate (and change it to inform the user what the default is, not what they might have meant).

rust/src/librustc_lint/types.rs

Lines 1038 to 1044 in e614f66

	fn check_type_for_ffi_and_report_errors(
	&mut self,
	sp: Span,
	ty: Ty<'tcx>,
	is_static: bool,
	is_return_type: bool,
	) {

I'd also file an issue to track the potential improvement to #[no_mangle] w/out extern (or open another PR, but it might warrant some discussion first).

[JohnCSimon]

@jam1garner Ping from triage -
Can you please address the comments from davidtwco. Thanks!

[jam1garner]

I certainly agree with a lot of what davidtwco said, just haven't had time to implement his suggestions. Certainly still intend to

[bors]

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.