Stripping `extern "Rust"` from functions #5701

Jarcho · 2023-02-28T02:40:08Z

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.

The text was updated successfully, but these errors were encountered:

ytmimi · 2023-02-28T05:25:14Z

@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 · 2023-02-28T06:01:54Z

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 · 2023-02-28T15:00:11Z

@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 · 2023-02-28T15:01:51Z

@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 · 2023-02-28T15:17:15Z

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 · 2023-02-28T15:18:48Z

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

calebcartwright · 2023-02-28T15:29:57Z

@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 · 2023-03-09T00:29:30Z

@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.

Improve diagnostic of `no_mangle_with_rust_abi` fixes #10409 Pending rust-lang/rustfmt#5701 This rewords the message to focus on the error being an implicit ABI, rather than the `Rust` ABI. Also downgrades the suggestion to `MaybeIncorrect` and changes the suggestion span to better highlight the change. --- changelog: None

Here are two paths of behavior this changes: 1. whenever we see an `extern "Rust"` on a function, we don't strip it from the function (which fixes rust-lang#5701) 2. if `force_explicit_abi` is disabled, we preserve what the user has written. Previously, rustfmt would change `extern "C"` to `extern `, which is not something a code formatter should do.

ojeda · 2023-07-29T16:39:28Z

rust-lang/rust-clippy#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".

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Jarcho · 2023-07-30T07:52:51Z

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.

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: #967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Link: https://lore.kernel.org/r/20230729220317.416771-2-ojeda@kernel.org

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

ojeda · 2023-07-31T17:36:51Z

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 · 2023-08-02T07:48:27Z

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 · 2023-08-02T17:14:30Z

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 - rust-lang/style-team#52 (comment)) 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.

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Here are two paths of behavior this changes: 1. whenever we see an `extern "Rust"` on a function, we don't strip it from the function (which fixes rust-lang#5701) 2. if `force_explicit_abi` is disabled, we preserve what the user has written. Previously, rustfmt would change `extern "C"` to `extern `, which is not something a code formatter should do.

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Link: https://lore.kernel.org/r/20230729220317.416771-2-ojeda@kernel.org

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Reviewed-by: Trevor Gross <tmgross@umich.edu> Reviewed-by: Gary Guo <gary@garyguo.net> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Link: https://lore.kernel.org/r/20230729220317.416771-2-ojeda@kernel.org Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Introduced in Rust 1.69.0 [1], this lint prevents forgetting to set the C ABI when using `#[no_mangle]` (or thinking it is implied). For instance, it would have prevented the issue [2] fixed by commit c682e4c ("rust: kernel: Mark rust_fmt_argument as extern "C""). error: `#[no_mangle]` set on a function with the default (`Rust`) ABI --> rust/kernel/print.rs:21:1 | 21 | / unsafe fn rust_fmt_argument( 22 | | buf: *mut c_char, 23 | | end: *mut c_char, 24 | | ptr: *const c_void, 25 | | ) -> *mut c_char { | |________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#no_mangle_with_rust_abi = note: requested on the command line with `-D clippy::no-mangle-with-rust-abi` help: set an ABI | 21 | unsafe extern "C" fn rust_fmt_argument( | ++++++++++ help: or explicitly set the default | 21 | unsafe extern "Rust" fn rust_fmt_argument( | +++++++++++++ Thus enable it. In rare cases, we may need to use the Rust ABI even with `#[no_mangle]` (e.g. one case, before 1.71.0, would have been the `__rust_*` functions). In those cases, we would need to `#[allow(...)]` the lint, since using `extern "Rust"` explicitly (as the compiler suggests) currently gets overwritten by `rustfmt` [3]. Link: rust-lang/rust-clippy#10347 [1] Link: Rust-for-Linux/linux#967 [2] Link: rust-lang/rustfmt#5701 [3] Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Jarcho mentioned this issue Feb 28, 2023

Improve diagnostic of no_mangle_with_rust_abi rust-lang/rust-clippy#10420

Merged

ytmimi added the needs-mcve needs a Minimal Complete and Verifiable Example label Feb 28, 2023

ytmimi removed the needs-mcve needs a Minimal Complete and Verifiable Example label Feb 28, 2023

fee1-dead mentioned this issue Jul 18, 2023

refactor ABI formatting #5845

Merged

ojeda mentioned this issue Jul 29, 2023

rustfmt wanted features & bugfixes Rust-for-Linux/linux#398

Open

9 tasks

ytmimi mentioned this issue Jan 20, 2024

Sync rustfmt subtree 2024-01-20 ytmimi/rust#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stripping `extern "Rust"` from functions #5701

Stripping `extern "Rust"` from functions #5701

Jarcho commented Feb 28, 2023 •

edited

Loading

ytmimi commented Feb 28, 2023

Jarcho commented Feb 28, 2023 •

edited

Loading

ytmimi commented Feb 28, 2023

ytmimi commented Feb 28, 2023

calebcartwright commented Feb 28, 2023

ytmimi commented Feb 28, 2023 •

edited

Loading

calebcartwright commented Feb 28, 2023

ytmimi commented Mar 9, 2023 •

edited

Loading

ojeda commented Jul 29, 2023 •

edited

Loading

Jarcho commented Jul 30, 2023 •

edited

Loading

ojeda commented Jul 31, 2023

tgross35 commented Aug 2, 2023

calebcartwright commented Aug 2, 2023

Stripping extern "Rust" from functions #5701

Stripping extern "Rust" from functions #5701

Comments

Jarcho commented Feb 28, 2023 • edited Loading

ytmimi commented Feb 28, 2023

Jarcho commented Feb 28, 2023 • edited Loading

ytmimi commented Feb 28, 2023

ytmimi commented Feb 28, 2023

calebcartwright commented Feb 28, 2023

ytmimi commented Feb 28, 2023 • edited Loading

calebcartwright commented Feb 28, 2023

ytmimi commented Mar 9, 2023 • edited Loading

ojeda commented Jul 29, 2023 • edited Loading

Jarcho commented Jul 30, 2023 • edited Loading

ojeda commented Jul 31, 2023

tgross35 commented Aug 2, 2023

calebcartwright commented Aug 2, 2023

Stripping `extern "Rust"` from functions #5701

Stripping `extern "Rust"` from functions #5701

Jarcho commented Feb 28, 2023 •

edited

Loading

Jarcho commented Feb 28, 2023 •

edited

Loading

ytmimi commented Feb 28, 2023 •

edited

Loading

ytmimi commented Mar 9, 2023 •

edited

Loading

ojeda commented Jul 29, 2023 •

edited

Loading

Jarcho commented Jul 30, 2023 •

edited

Loading