Allow arbitrary extern ABI

[dtolnay]

Rustfmt fails to format the following file, even though rustc happily compiles it.

I am using cfg here to repro, but the more important case in practice would be to allow non-standard ABI in the input of procedural macro attributes like https://github.com/dtolnay/cxx.

#[cfg(any())]
extern "C++" {}

error[E0703]: invalid ABI: found `C++`
 --> /git/testing/src/main.rs:2:8
  |
2 | extern "C++" {}
  |        ^^^^^ invalid ABI
  |
  = help: valid ABIs: cdecl, stdcall, fastcall, vectorcall, thiscall, aapcs, win64, sysv64, ptx-kernel, msp430-interrupt, x86-interrupt, amdgpu-kernel, Rust, C, system, rust-intrinsic, rust-call, platform-intrinsic, unadjusted

[rchaser53]

I cannot reproduce this. Could you tell me the version you use?

[dtolnay]

I guess the error message was from the rustfmt that ships with stable 1.42, but after trying it with today's rustfmt master branch (9124dd8) the current behavior is even worse; it silently replaces the ABI with "Rust".

$ echo '#[cfg(any())] extern "C++" {}' | rustup run 1.42.0 rustfmt
error[E0703]: invalid ABI: found `C++`
 --> <stdin>:1:22
  |
1 | #[cfg(any())] extern "C++" {}
  |                      ^^^^^ invalid ABI
  |
  = help: valid ABIs: cdecl, stdcall, fastcall, vectorcall, thiscall, aapcs, win64, sysv64, ptx-kernel, msp430-interrupt, x86-interrupt, amdgpu-kernel, Rust, C, system, rust-intrinsic, rust-call, platform-intrinsic, unadjusted

$ echo '#[cfg(any())] extern "C++" {}' | rustup run nightly-2020-03-19 rustfmt
#[cfg(any())]
extern "Rust" {}

$ git rev-parse HEAD
9124dd88d6ef14d0ae77dc52ff5e9598f24a75a0

$ echo '#[cfg(any())] extern "C++" {}' | cargo run --bin rustfmt
#[cfg(any())]
extern "Rust" {}

[calebcartwright]

Rust is the fallback in the event of an abi lookup failure, which I believe would happen here since C++ isn't an Abi variant

rustfmt/rustfmt-core/rustfmt-lib/src/utils.rs

Lines 137 to 148 in 9124dd8

	pub(crate) fn format_extern(
	ext: ast::Extern,
	explicit_abi: bool,
	is_mod: bool,
	) -> Cow<'static, str> {
	let abi = match ext {
	ast::Extern::None => abi::Abi::Rust,
	ast::Extern::Implicit => abi::Abi::C,
	ast::Extern::Explicit(abi) => {
	abi::lookup(&abi.symbol_unescaped.as_str()).unwrap_or(abi::Abi::Rust)
	}
	};

Updating that logic to keep the original string should be doable, though I'm not sure if that'd have any other side effects

[dtolnay]

Updating that logic to keep the original string should be doable, though I'm not sure if that'd have any other side effects

I think keeping the original string would be the obvious correct behavior for rustfmt. It's definitely what I would want in https://github.com/dtolnay/cxx.

[Mark-Simulacrum]

Upstream, I believe the validation of the string was moved from parsing to a later stage recently as well so it's plausible a bump of the syntax version used here would be reasonable as well.

[calebcartwright]

I think keeping the original string would be the obvious correct behavior for rustfmt.

Agreed, just needs some refactoring to support doing so while still honoring the force_explicit_abi option in the other cases

[calebcartwright]

it's plausible a bump of the syntax version used here would be reasonable as well.

Also agreed, though we're going to have some challenges bumping the version of the syntax/ast crate and friends in rustfmt given some of those usptream changes in parsing, especially rust-lang/rust#69838

[calebcartwright]

@rchaser53 are you working on this one already? if not i'll take a look in the next day or so

[Centril]

Upstream, I believe the validation of the string was moved from parsing to a later stage recently as well so it's plausible a bump of the syntax version used here would be reasonable as well.

Yep; the grammar of the language now only cares about string literals, deferring translation actual ABIs (in the sense of rustc_target) to HIR lowering.

(Aside: very nice, and unexpected, to see @dtolnay taking advantage of this =P.)

[rchaser53]

@calebcartwright No, I haven't yet. I cannot have the enough time until this Sunday. So I will leave it to you. Thank you.

[calebcartwright]

We're still a good ways out from being ready to release v2.x of rustfmt, so I've also opened #4089 against the latest 1.x branch in case there's a desire to have the fix available sooner.