Rustfmt for<'a> async correctly
#131657
Rustfmt for<'a> async correctly
#131657
Conversation
rustbot
added
S-waiting-on-review
compiler-errors
force-pushed
the
rustfmt-modifiers
branch
from
56bebed
to
dca646a
Compare
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
|
r? @ytmimi or @calebcartwright (doing this PR on top of rust-lang/rust since i don't know when the next subtree sync is, and I'm looking to stabilize in the next few months.) |
|
@compiler-errors we can do a sync relatively soon. Definitely within the next few months. We'll need to do a sync once style_edition 2024 is stabilized. Could you move this PR over to rustfmt? |
|
@ytmimi: Given the fact that this is affecting users on nightly (see the linked issue -- rustfmt rewrites the code to not compile anymore), I'd like to perform the subtree sync immediately or land it here. |
|
Like, I'm happy to do anything in my power to help accelerate this process (prepare the subtree pushes, resolve any merge conflicts, etc) but I frankly think that waiting months to fix this issue is a very bad experience for users of async closures :) |
|
Given that extra context I'm happy to merge here. Async closures are still unstable, right? So this is only impacting users who're opting into the feature? Also, I'm wondering if it's possible for the rustfmt team to be notified about these types of syntax changes to hopefully catch some of these issues earlier on in the process |
Correct.
Yeah, it was an oversight when implementing #127054, otherwise I would've probably included the rustfmt changes directly in that PR. |
Totally understandable. We only get notified if the rustfmt tree is modified so I'm wondering if there's an easy way to communicate syntax changes to the team. |
|
@bors r=ytmimi |
bors
added
S-waiting-on-bors
… r=ytmimi Rustfmt `for<'a> async` correctly In rust-lang#127054, we decided to move the trait bound modifier for `async for<'a> Fn()` to `for<'a> async Fn()`. This wasn't adjusted in rustfmt, so this PR implements that. It also requires consolidating the bound formatting into the `Rewrite` impl for `PolyTraitRef`. Fixes rust-lang#131649
Rollup of 10 pull requests Successful merges: - rust-lang#129181 (Pass end position of span through inline ASM cookie) - rust-lang#130989 (Don't check unsize goal in MIR validation when opaques remain) - rust-lang#131657 (Rustfmt `for<'a> async` correctly) - rust-lang#131691 (Delay ambiguous intra-doc link resolution after `Cache` has been populated) - rust-lang#131730 (Refactor some `core::fmt` macros) - rust-lang#131751 (Rename `can_coerce` to `may_coerce`, and then structurally resolve correctly in the probe) - rust-lang#131753 (Unify `secondary_span` and `swap_secondary_and_primary` args in `note_type_err`) - rust-lang#131776 (Emscripten: Xfail backtrace ui tests) - rust-lang#131777 (Fix trivially_copy_pass_by_ref in stable_mir) - rust-lang#131778 (Fix needless_lifetimes in stable_mir) r? `@ghost` `@rustbot` modify labels: rollup
|
I don't have any concerns with this PR being made in-tree here, and in general there's no concerns with urgent fixes being made here; I've fixed a few rustfmt bugs myself here when urgency warranted. I would, however, push back on the implied characterization of how we'd hypothetically have addressed this. Yes, it's true we don't do syncs on a regular cadence, and sometimes we will go quite a while without doing a sync. However, every time there's been an issue of priority or urgency brought to our attention, we address it promptly and can and do perform syncs over timespans measured in hours or days, not months. There are multiple pain points and friction we experience during syncs, and while we are working on addressing those we don't do a full sync unless and until we've crossed a threshold that warrants it (which could be an accumulation of enough worthwhile features, minor bug fixes, etc., or a high priority item needing urgent attention). But we shouldn't conflate those timelines with our readiness and willingness to take action to deal with time sensitive issues. |
I can't see any path to this via the existing notification mechanisms, and there will always be some surface for AST changes to be made which won't break rustfmt. I think it's okay for this potential to exist with nightly features and a quick turn around to resolve them, and the only other approach I could think of to help prevent them would be more process oriented in nature (e.g. a snippet with the new/modified syntax being added to rustfmt tests under |
Yeah. If we tracked the grammar more formally (with a grammar specification or something -- specifically, separately from the parser) the perhaps rustfmt or style could be modified everytime that gets changed, but currently that isn't so. |
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#130989 (Don't check unsize goal in MIR validation when opaques remain) - rust-lang#131657 (Rustfmt `for<'a> async` correctly) - rust-lang#131691 (Delay ambiguous intra-doc link resolution after `Cache` has been populated) - rust-lang#131730 (Refactor some `core::fmt` macros) - rust-lang#131751 (Rename `can_coerce` to `may_coerce`, and then structurally resolve correctly in the probe) - rust-lang#131753 (Unify `secondary_span` and `swap_secondary_and_primary` args in `note_type_err`) - rust-lang#131776 (Emscripten: Xfail backtrace ui tests) - rust-lang#131777 (Fix trivially_copy_pass_by_ref in stable_mir) - rust-lang#131778 (Fix needless_lifetimes in stable_mir) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131657 - compiler-errors:rustfmt-modifiers, r=ytmimi Rustfmt `for<'a> async` correctly In rust-lang#127054, we decided to move the trait bound modifier for `async for<'a> Fn()` to `for<'a> async Fn()`. This wasn't adjusted in rustfmt, so this PR implements that. It also requires consolidating the bound formatting into the `Rewrite` impl for `PolyTraitRef`. Fixes rust-lang#131649
In #127054, we decided to move the trait bound modifier for
async for<'a> Fn()tofor<'a> async Fn(). This wasn't adjusted in rustfmt, so this PR implements that. It also requires consolidating the bound formatting into theRewriteimpl forPolyTraitRef.Fixes #131649