Stripping `extern "Rust"` from functions

[Jarcho]

Rustfmt currently strips extern "Rust" from function signatures. e.g formatting extern "Rust" fn foo() {} to fn foo() {}. This seems to be intentional from the current code.

The problem here is this essentially deletes documentation from the code. Given a function such as:

#[no_mangle]
extern "Rust" fn foo() { /* do stuff */ }

With the ABI explicitly added here, it's clear that the odd ABI choice is intentional. Without the explicit ABI, it's unclear whether the choice was intentional, or an ABI was forgotten (e.g. extern "C").

Can't find anything relevant from the style guide about this removal.

[ytmimi]

@Jarcho what version of rustfmt are you using? Also, are you using any non-default config options?

using the latest master rustfmt 1.5.2-nightly (34f9ca28 2023-02-16) I'm unable to reproduce the issue you've described.

running rustfmt on

#[no_mangle]
extern "rust" fn foo() { /* do stuff */ }

produces:

#[no_mangle]
extern "rust" fn foo() { /* do stuff */
}

I believe support for formatting arbitrary extern ABIs was implemented by #4089 and has been available for quite some time.

[Jarcho]

Latest version on the playground. Listed as 1.5.2-nightly (2023-02-27 7281249). Playground link showcasing the problem.

The cause I assume is from this part of format_extern:

if abi == "Rust" && !is_mod {
    Cow::from("")
}

Edit: Should have been Rust not rust. Issue has been updated.

[ytmimi]

@Jarcho thanks for the clarification and for updating the description. I can confirm that with extern "Rust" instead of extern "rust" the explicit ABI is removed:

running rustfmt 1.5.2-nightly (34f9ca28 2023-02-16) on:

#[no_mangle]
extern "Rust" fn foo() { /* do stuff */ }

produces:

#[no_mangle]
fn foo() { /* do stuff */
}

You're also correct about where the issue is coming from:

rustfmt/src/utils.rs

Lines 145 to 146 in 34f9ca2

	if abi == "Rust" && !is_mod {
	Cow::from("")

Given that force_explicit_abi=true is the default, I'd expect that we wouldn't remove extern "Rust".

[ytmimi]

@calebcartwright give that a fix for this would change the behavior of force_explicit_abi, which is a stable config, would we need to version gate the change?

[calebcartwright]

Having a bit of déjà vu with this one. Not able to sit down with my dev machine for a while, but if you have bandwidth can you check to see what, if any impact, something like #4293 would have on addressing this?

[ytmimi]

@calebcartwright Thanks for pointing that PR out. I can certainly take a look 😁

[calebcartwright]

@calebcartwright Thanks for pointing that PR out. I can certainly take a look 😁

Thanks. IIRC that was going after a different use case, but wonder if the same reasoning can be applied. In general I don't think rustfmt should be doing these types of things, even if it can (i.e. this type of conversion is better suited to something like rustfix imo)

[ytmimi]

@calebcartwright I looked into #4293, and I believe that it will prevent rustfmt from removing the explicit extern "Rust" declaration. Do the changes need to be version gated?

Additionally, I'm wonder if there's a clear process for removing or deprecating stable options. After reading through the discussions on the linked PR and the associated issue I feel like we could simplify the implementation of format_extern if we removed force_explicit_abi altogether. Searching for "force_explicit_abi = false" doesn't turn up much so I don't think there would be a major impact by removing the option.