rustfmt removes "async" keyword in experimental feature, changing the meaning of the code

[RalfJung]

When formatting this file

#![feature(async_closure, noop_waker, async_fn_traits)]
//@compile-flags: --edition 2018

use std::future::Future;
use std::pin::pin;
use std::task::*;

pub fn block_on<T>(fut: impl Future<Output = T>) -> T {
    let mut fut = pin!(fut);
    let ctx = &mut Context::from_waker(Waker::noop());

    loop {
        match fut.as_mut().poll(ctx) {
            Poll::Pending => {}
            Poll::Ready(t) => break t,
        }
    }
}

async fn call_once(f: impl async FnOnce(DropMe)) { // <--- this line is changed
    f(DropMe("world")).await;
}

#[derive(Debug)]
struct DropMe(&'static str);

impl Drop for DropMe {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

pub fn main() {
    block_on(async {
        let b = DropMe("hello");
        let async_closure = async move |a: DropMe| {
            println!("{a:?} {b:?}");
        };
        call_once(async_closure).await;
    });
}

rustfmt removes the 2nd async in the marked line, changing impl async FnOnce to impl FnOnce. The code then fails to compile.

Of course rustfmt can't be expected to properly format experimental syntax, but it'd be better if rustfmt could at least not entirely remove experimental syntax, and rather leave it unchanged. :) This caused a ton of confusion in rust-lang/miri#3298 while I was trying to figure out why the test worked on rustc CI but failed on Miri CI... until Oli realized the automatically applied formatting broke the code.

[ytmimi]

@RalfJung thanks for reaching out. We've had several instances recently where feature flag enabled syntax isn't properly handled by rustfmt. #6043, and #5995 immediately come to mind.

I 100% agree that rustfmt shouldn't mess with experimental syntax, though I don't think rustfmt is totally at fault here. I think the underlying issue is that when the feature was developed no one modified rustfmt to make sure that it would work correctly with the new experimental syntax. Also, to my knowledge, there isn't a way for rustfmt to know that it's currently formatting experimental syntax. It just walks the AST and applies the formatting rules for the nodes it knows about.

I think it would be reasonable to add an idempotency test to rustfmt in the rust-lang/rust tree when working on experimental syntax. The process should be as simple as adding a code snippet to a test file in src/tools/rustfmt/tests/target. This way r-l/rust contributors can address this earlier in the development process.

We typically discourage adding formatting rules to rustfmt in the r-l/rust tree, but I think we can strike a balance and make sure rustfmt at least echos back out the span for the AST node with Some(context.snippet(span).to_owned()).

The rustfmt changes made in rust-lang/rust#115367 and the associated idempotence test case are a good example of what I think could be done to alleviate this issue moving forward.

@RalfJung for my own awareness, when was this experimental syntax added?

[RalfJung]

Yeah, it makes sense to say "when you add new syntax, do something to rustfmt to make it not break that syntax". This should probably be discussed with the compiler team then.

for my own awareness, when was this experimental syntax added?

I don't exactly know, it's a recent PR by @compiler-errors I think.

[fmease]

T-style's nightly style procedure takes the opposite stance.

[ytmimi]

@fmease could you elaborate on which lines you're referring to? I want to make sure we're on the same page. When I read

Initial PR(s) implementing new syntax filed against rust-lang/rust should generally leave the rustfmt subtree untouched. In cases that that need to modify rustfmt (for example, to fix compiler errors that come from adding new variants to AST representation), they should modify rustfmt in such a way to keep existing formatting verbatim.

I think what's trying to be said is "Don't implement formatting rules yourself", instead "leave formatting as is".

From my comment above #6070 (comment):

We typically discourage adding formatting rules to rustfmt in the r-l/rust tree, but I think we can strike a balance and make sure rustfmt at least echos back out the span for the AST node with Some(context.snippet(span).to_owned()).

[calebcartwright]

Closing as this should be resolved by rust-lang/rust#121035

If the issue persists in subsequent nightly releases we can reopen.

Regarding discussion about process, and speaking personally albeit as a member of both teams, I do not view anything said in this issue as being contradictory to the wording nor the intent of our policy on the style team.

[fmease]

Initial PR(s) implementing new syntax filed against rust-lang/rust should generally leave the rustfmt subtree untouched. In cases that that need to modify rustfmt (for example, to fix compiler errors that come from adding new variants to AST representation), they should modify rustfmt in such a way to keep existing formatting verbatim.

My reading of this paragraph is “don't touch rustfmt unless it no longer compiles (and if it no longer compiles you should fix it without making any big changes and without breaking existing formatting)”. At no point does it say to add makeshift formatting for newly introduced syntax. And syntax additions often won't lead to compile errors in rustfmt since people usually don't exhaustively match on structs. This was the case for generic_const_items — namely the generics field — as well as for async_closures – namely the modifiers field (that's probably why errs has replaced it with an exhaustive match).

We typically discourage adding formatting rules to rustfmt in the r-l/rust tree, but I think we can strike a balance and make sure rustfmt at least echos back out the span for the AST node with Some(context.snippet(span).to_owned()).

Thanks! This is great advice. Could this maybe be added to some guide or something?

[compiler-errors]

I'd appreciate if the wording were modified to say something like "avoid undue modification to rustfmt in the rust-lang/rust tree" but then also saying "however, you can modify rustfmt to preserve syntax elements so that they are not removed". It's my fault for having authored that nightly style policy.

[compiler-errors]

The problem here is that it would be nice if we could avoid all changes to rustfmt in rust-lang/rust, and then open follow-up PRs in rust-lang/rustfmt to implement formatting for new AST; however, subtree syncs happen so rarely that this is not feasible in practice, leading to situations like this. :/

[calebcartwright]

What Ralf initially said in the issue description 👇 is what we on the rustfmt would prefer to see and typically encourage PR authors in r-l/rust to do:

but it'd be better if rustfmt could at least not entirely remove experimental syntax, and rather leave it unchanged. :)

sometimes things are missed, but it's nightly, it happens.

Now speaking with my style team hat on, our primary objective for the style team document being referenced to was to explicitly establish procedures around nightly syntax and that the formatting for nightly syntax could change, something which was previously entirely ambiguous. It was never intended to be an authoritative development guide for rustfmt, nor should it be interpreted that way.

If there's suggested tweaks to a document owned by a different team then perhaps those could be made elsewhere (ideally in a PR to the document in question 🙏)

[calebcartwright]

In the hope of providing some additional clarity here, I'll also add that on the rustfmt side we make a very explicit distinction between actively formatting a construct vs. not butchering/breaking a construct that we don't yet support formatting (or for which the style guide does not yet have formatting prescriptions).

As a general practice, we don't want to see rustfmt feature development, including implementing formatting support, happen in r-l/rust for several reasons. However, changes to the rustfmt in r-l/rust will absolutely be required at times to avoid breaking/butchering constructs and/or compilation issues.

In some circumstances implementing an acceptable initial formatting for experimental syntax is so trivially simple and noncontroversial (e.g. https://github.com/rust-lang/rust/pull/120131/files#diff-48d9c8ae6d4ad56bb7ccde2faf020223463cf3ded39f1b70aad9055586e4b3cd) that neither the style team nor the rustfmt team has concerns with cursory formatting being added alongside the feature.

The intent there is to be more iterative and better support experimental syntax more quickly; not to do anything "makeshift"

Conversely, the types of changes made to rustfmt within r-l/rust that are particularly frustrating and challenging for us are things that we feel are truly unnecessary as they don't impact users of rustfmt (e.g. typo corrections, lints we don't check for in our own CI process, etc.) and especially since git subtree seems to get so easily choked up on subtree syncs