fix: don't make ABI explicit when not guaranteed to preserve semantics (#4293)

Author: calebcartwright

src/formatting/items.rs

@@ -195,13 +195,19 @@ struct Item<'a> {
 }
 
 impl<'a> Item<'a> {
-    fn from_foreign_mod(fm: &'a ast::ForeignMod, span: Span, config: &Config) -> Item<'a> {
+    fn from_foreign_mod(
+        fm: &'a ast::ForeignMod,
+        span: Span,
+        config: &Config,
+        attrs: &[ast::Attribute],
+    ) -> Item<'a> {
         Item {
             keyword: "",
             abi: format_extern(
                 ast::Extern::from_abi(fm.abi),
                 config.force_explicit_abi(),
                 true,
+                Some(attrs),
             ),
             vis: None,
             body: fm
@@ -293,6 +299,7 @@ impl<'a> FnSig<'a> {
             self.ext,
             context.config.force_explicit_abi(),
             false,
+            None,
         ));
         result
     }
@@ -337,8 +344,13 @@ impl<'a> FmtVisitor<'a> {
         }
     }
 
-    pub(crate) fn format_foreign_mod(&mut self, fm: &ast::ForeignMod, span: Span) {
-        let item = Item::from_foreign_mod(fm, span, self.config);
+    pub(crate) fn format_foreign_mod(
+        &mut self,
+        fm: &ast::ForeignMod,
+        span: Span,
+        attrs: &[ast::Attribute],
+    ) {
+        let item = Item::from_foreign_mod(fm, span, self.config, attrs);
         self.format_item(&item);
     }
 

src/formatting/types.rs

@@ -785,6 +785,7 @@ fn rewrite_bare_fn(
         bare_fn.ext,
         context.config.force_explicit_abi(),
         false,
+        None,
     ));
 
     result.push_str("fn");

src/formatting/utils.rs

@@ -147,19 +147,28 @@ pub(crate) fn format_extern(
     ext: ast::Extern,
     explicit_abi: bool,
     is_mod: bool,
+    attrs: Option<&[ast::Attribute]>,
 ) -> Cow<'static, str> {
-    let abi = match ext {
-        ast::Extern::None => "Rust".to_owned(),
-        ast::Extern::Implicit => "C".to_owned(),
-        ast::Extern::Explicit(abi) => abi.symbol_unescaped.to_string(),
-    };
-
-    if abi == "Rust" && !is_mod {
-        Cow::from("")
-    } else if abi == "C" && !explicit_abi {
-        Cow::from("extern ")
-    } else {
-        Cow::from(format!(r#"extern "{}" "#, abi))
+    let format_explicit_abi = |abi: &str| Cow::from(format!(r#"extern "{}" "#, abi));
+    let explicit_conversion_preserves_semantics =
+        || !is_mod || (is_mod && attrs.map_or(true, |a| a.is_empty()));
+
+    match ext {
+        ast::Extern::None if !is_mod => Cow::from(""),
+        ast::Extern::Explicit(ast::StrLit {
+            symbol_unescaped, ..
+        }) if !is_mod && symbol_unescaped == rustc_span::sym::rust => Cow::from(""),
+        ast::Extern::Implicit if !explicit_abi || !explicit_conversion_preserves_semantics() => {
+            Cow::from("extern ")
+        }
+        ast::Extern::Explicit(ast::StrLit {
+            symbol_unescaped, ..
+        }) if !explicit_abi && symbol_unescaped == rustc_span::sym::C => Cow::from("extern "),
+        ast::Extern::None => format_explicit_abi("Rust"),
+        ast::Extern::Implicit => format_explicit_abi("C"),
+        ast::Extern::Explicit(ast::StrLit {
+            symbol_unescaped, ..
+        }) => format_explicit_abi(&symbol_unescaped.to_string()),
     }
 }
 

src/formatting/visitor.rs

@@ -513,7 +513,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
                 }
                 ast::ItemKind::ForeignMod(ref foreign_mod) => {
                     self.format_missing_with_indent(source!(self, item.span).lo());
-                    self.format_foreign_mod(foreign_mod, item.span);
+                    self.format_foreign_mod(foreign_mod, item.span, item.attrs());
                 }
                 ast::ItemKind::Static(..) | ast::ItemKind::Const(..) => {
                     self.visit_static(&StaticParts::from_item(item));

tests/source/extern.rs

@@ -82,3 +82,20 @@ libc::c_long;
 extern {
 
 }
+
+// #2908 - https://github.com/rust-lang/rustfmt/issues/2908
+#[wasm_bindgen(module = "child_process", version = "*")]
+extern {
+     #[wasm_bindgen(js_name = execSync)]
+     fn exec_sync(cmd: &str) -> Buffer;
+
+     fn foo() -> Bar;
+ }
+
+// Users that have an existing explicit ABI would need to convert to implicit
+// manually, as rustfmt will be conservative and not attempt to convert explicit
+// to implicit in the wasm case.
+#[wasm_bindgen]
+extern "C" {
+       fn foo() -> Bar;
+}

tests/target/extern.rs

@@ -84,3 +84,20 @@ extern "C" {
 }
 
 extern "C" {}
+
+// #2908 - https://github.com/rust-lang/rustfmt/issues/2908
+#[wasm_bindgen(module = "child_process", version = "*")]
+extern {
+    #[wasm_bindgen(js_name = execSync)]
+    fn exec_sync(cmd: &str) -> Buffer;
+
+    fn foo() -> Bar;
+}
+
+// Users that have an existing explicit ABI would need to convert to implicit
+// manually, as rustfmt will be conservative and not attempt to convert explicit
+// to implicit in the wasm case.
+#[wasm_bindgen]
+extern "C" {
+    fn foo() -> Bar;
+}