extern is replaced with extern "C" for wasm extern functions

[gnzlbg]

I am getting this diff with rustfmt in stdsimd, is this intended (@nrc , @topecongiro ) / correct (cc @alexcrichton) ?

 #[wasm_bindgen(module = "child_process", version = "*")]
-extern {
+extern "C" {
     #[wasm_bindgen(js_name = execSync)]
     fn exec_sync(cmd: &str) -> Buffer;
 }

[alexcrichton]

This was briefly discussed at rustwasm/wasm-bindgen#422, and while wonky it was seen as "we don't have much choice!"

[gnzlbg]

The simd128 PR in stdsimd contains these changes and the build is green, so 👍

[gnzlbg]

I ran into this again now that rustfmt is able to format most of the libc crate and I'm not sure if I follow the rationale given in that issue:

extern { } is a synonym for extern "C" { }.

However, #[wasm_bindgen] extern { } is not the same as extern { }. It is actually more similar to wasm_bindgen! { extern { } }, which rustfmt should probably not format to wasm_bindgen! { extern "C" { } } because that would possibly break the macro expansion.

@topecongiro I think this is just an instance of rustfmt trying to format code inside a macro, and ending up breaking the macro. A procedural macro is still a macro.

[calebcartwright]

@topecongiro - what are we thinking in terms of resolving this one? do we want to allow an implicit abi on foreign item mods even when force_explicit_abi is true (which is the default) for mods that have the wasm_bindgen attribute? If so, should rustfmt do this by default/always or would we want an additional config option to let users control whether or not rustfmt inserts an explicit "C" abi;

I've yet to do anything with wasm, but my sense from these threads is that the insertion of the explicit abi is undesirable and a bit puzzling so I'm guessing users don't actually want or need to be able to have the explicit abi inserted.

[topecongiro]

I've been thinking about it lately that when using the default configuration, rustfmt should not modify code that could lead to semantic changes. It is hard to do this correctly because of the proc macro, but we should try whenever possible.

With that mind, regarding this issue, I'd like to change the default value of force_explicit_abi to false and include a note in the documentation about the potential issue when setting it to true.

[calebcartwright]

I'd like to change the default value of force_explicit_abi to false

The style guide prescribes being explicit which I presume is why rustfmt defaults to true. Do we need to update the style guide as well?

https://github.com/rust-dev-tools/fmt-rfcs/blob/f4fb7b84eacb8161905157b761dd8002b0eed044/guide/items.md#extern-items

When writing extern items (such as extern "C" fn), always be explicit about the ABI. For example, write extern "C" fn foo ..., not extern fn foo ..., or extern "C" { ... }.

[topecongiro]

Yes, that's right. Maybe tone it down a bit and change always be explicit about the ABI. to prefer to be explicit about the API?

[calebcartwright]

Maybe tone it down a bit and change always be explicit about the ABI. to prefer to be explicit about the API?

Even if we softened the language a bit, I still feel like we'd end up in a scenario where rustfmt's default wouldn't follow the prescribed style guide approach for ABIs. I also have some concerns that changing the default to not be explicit across the board to address the wasm use case may negatively impact the other cases where the conversion to explicit ABI is preferred (rust-lang/style-team#52) 🤔

I see a few options:

1. As of rustfmt 1.4.13, rustfmt supports formatting arbitrary ABIs, so a different explicit ABI like "wasm-bindgen" could be used as mentioned in Should extern "C" { ... } be disallowed? rustwasm/wasm-bindgen#422 (comment)
2. Update rustfmt's default and the corresponding style guide language as discussed above, but also mention that rustfmt won't do so without overriding the default configuration
3. Enhance the extern formatting logic in rustfmt to not convert from implicit to explicit when doing so could fail to be semantics preserving

I'm going to try to set aside some time in the next few days to work on this (assuming on one beats me to it), and would be happy to open PRs for options 2 and 3 for consideration. However, let me know if you definitely want to just press on with option 2 @topecongiro

[topecongiro]

@calebcartwright I agree that in most scenarios, it is desirable to normalize to explicit abi. As I said, I would like to be careful about adding/removing non-whitespace tokens. In this particular case, however, as you mentioned in option 1, users can work around this issue by adding an arbitrary abi by themselves; so just implementing the option 3 may be enough. Thank you for your insight.

I'm going to try to set aside some time in the next few days to work on this (assuming on one beats me to it), and would be happy to open PRs for options 2 and 3 for consideration.

Amazing ❤️ Note that the semantic change of the code is not limited to wasm-bindgen but can happen in any kind of proc macros. So I think it is safer not to add an explicit abi to the block if it has attributes.