Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async gets removed from extern blocks. #4288

Closed
rodrigorc opened this issue Jun 29, 2020 · 2 comments · Fixed by #4291
Closed

Async gets removed from extern blocks. #4288

rodrigorc opened this issue Jun 29, 2020 · 2 comments · Fixed by #4291
Assignees
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted
Milestone

Comments

@rodrigorc
Copy link

Describe the bug

The async keyword is removed when used inside an extern "C" block.

Currently this is not valid in Rust, because in C there are no async functions. But when using wasm-bindgen it is processes from a macro, and the functions are actually Javascript, not C, and there there are JS promises. Anyway, even if it is not valid, the keyword should not be removed while formatting.

To Reproduce

Format the following code:

extern "C" {
    async fn foo();
}

And see how the async keword disappears.

Expected behavior

It should keep the async keyword.

Meta

  • rustfmt version: 1.4.15-beta (530eadf 2020-06-02)
  • From where did you install rustfmt?: Rustup
  • How do you run rustfmt: From console: rustfmt --edition 2018 test.rs
@rodrigorc rodrigorc added the bug Panic, non-idempotency, invalid code, etc. label Jun 29, 2020
@calebcartwright
Copy link
Member

calebcartwright commented Jun 30, 2020

The root cause is that the fields from the FnHeader aren't leveraged in the format handling for functions in extern blocks so qualifiers like asyncness are dropped. I believe this was done intentionally given that such qualifiers are not valid Rust code (as you noted in the issue description), and rustfmt's focus on working with and formatting valid Rust code.

However, we could certainly tweak this to support maintaining the original qualifier, and it'd be up to users to remove their invalid usage of it (outside the wasm context) vs. rustfmt removing it for them.

The relevant code for anyone interested in working on this can be found here:

ast::ForeignItemKind::Fn(_, ref fn_sig, ref generics, _) => rewrite_fn_base(
context,
shape.indent,
self.ident,
&FnSig::new(&fn_sig.decl, generics, self.vis.clone()),
span,
FnBraceStyle::None,
)
.map(|(s, _)| format!("{};", s)),

pub(crate) fn new(
decl: &'a ast::FnDecl,
generics: &'a ast::Generics,
vis: ast::Visibility,
) -> FnSig<'a> {
FnSig {
decl,
generics,
ext: ast::Extern::None,
is_async: Cow::Owned(ast::Async::No),
constness: ast::Const::No,
defaultness: ast::Defaultness::Final,
unsafety: ast::Unsafe::No,
visibility: vis,
}
}

IMO the values set via FnSig::new still feel like a reasonable/correct set of defaults but perhaps it could be augmented or another from_* function could be added for building a FnSig geared towards formatting foreign item fns and then leverage that (instead of new) accordingly.

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Jun 30, 2020
@topecongiro topecongiro added this to the 2.0.0 milestone Jun 30, 2020
@topecongiro topecongiro self-assigned this Jun 30, 2020
ayazhafiz added a commit to ayazhafiz/rustfmt that referenced this issue Jun 30, 2020
Closes rust-lang#4288

And we get to drop a method, which I think is a win :)
@ayazhafiz
Copy link
Contributor

Shoot, sorry @topecongiro, I had started working on this before you assigned yourself and failed to see your assignment. Feel free to close my PR if you'd like. Apologies for the duplication of work.

topecongiro pushed a commit to topecongiro/rustfmt that referenced this issue Jun 30, 2020
Closes rust-lang#4288

And we get to drop a method, which I think is a win :)
dtolnay pushed a commit to dtolnay-contrib/rustfmt that referenced this issue Nov 29, 2020
Closes rust-lang#4288

And we get to drop a method, which I think is a win :)
calebcartwright pushed a commit that referenced this issue Nov 29, 2020
Closes #4288

And we get to drop a method, which I think is a win :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted
Projects
None yet
4 participants