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:

https://github.com/rust-lang/rustfmt/blob/34f9ca28f2f4ae3ac462c6dcd862b513e107e459/src/utils.rs#L145-L146

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 https://github.com/rust-lang/rustfmt/pull/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.

[ojeda]

https://github.com/rust-lang/rust-clippy/pull/10420 got merged, though I think the intention was to wait for this one (it said "Pending ..."), thus the diagnostic's suggestion currently does not work given rustfmt overwrites it, forcing the use of #[allow(clippy::no_mangle_with_rust_abi)] on #[no_mangle] functions intended to be extern "Rust".

[Jarcho]

Technically clippy won't lint if extern "Rust" is there, but the suggestion to add it is less than useless when paired with rustfmt given this issue. Had I known the issue would stall for this long the suggestion would not have been in that PR.

[ojeda]

Yeah, exactly, that was my point, i.e. rustfmt overwrites the suggestion and thus forces to use another approach (edited comment above for more clarity).

[tgross35]

Is the expected behavior that rustfmt shouldn't remove the ABI if #[no_mangle] is present? This seems reasonable and would keep it in sync with the Clippy lint

[calebcartwright]

To address the somewhat more meta topics:

While consistency across the landscape of official tools is of course important, clippy does not dictate rustfmt and vice versa. There's a few cases where given a certain input snippet, rustfmt will correctly produce a certain output (as required by the Rust Style Guide which does dictate rustfmt default behavior) that would subsequently be flagged by clippy. In those cases though it's still possible for the user to subsequently apply the clippy fix in a manner that remains compatible with additional rustfmt runs (the two tools aren't dueling, so to speak).

The presence or absence of an attribute (regardless of which) shouldn't drive the ABI behavior either. The Style Guide is very explicit about this (https://doc.rust-lang.org/nightly/style-guide/items.html#extern-items), and extern "Rust"... was part of the most persuasive argument (at least imo - https://github.com/rust-lang/style-team/issues/52#issuecomment-274697863) that led to the resultant Style prescription.

To be more explicit about my own position:

Even though the removal of this particular ABI doesn't technically change the semantics, I still view the current behavior as a bug given what the Style Guide requires (albeit an incredibly long standing one).

I'll share this with the Style team to ensure we get the guide updated one way or another, and even if the team decides the removal is the desirable default behavior then we can consider adding a new option to rustfmt to allow users to control the behavior and opt into maintaining the abi.

I don't intend to comment on any aspects related to perceptions that this has stalled or is taking too long beyond noting that one way or another I anticipate some action on this soon.