rustfmt no longer builds after rust-lang/rust#84310

[rust-highfive]

Hello, this is your friendly neighborhood mergebot.
After merging PR #84310, I observed that the tool rustfmt no longer builds.
A follow-up PR to the repository https://github.com/rust-lang/rustfmt is needed to fix the fallout.

cc @RalfJung, do you think you would have time to do the follow-up work?
If so, that would be great!

[RalfJung]

The fix should be to use the const_fn_trait_bound and/or const_fn_unsize feature gates instead of const_fn. Let me know if/how I can help with that.

[RalfJung]

The latest version 716 of rustc-ap-rustc_ast is still too old, it does not yet use the const_fn_unsize feature gate... shouldn't new versions be published automatically?

[calebcartwright]

shouldn't new versions be published automatically?

They get published weekly, on Tuesday

However, depending on how big an impact this round of updates has, we may end up resolving this with #82208 (convert to subtree and drop the AP crate dependencies)

[RalfJung]

Oh, just once a week? Wow yeah that sounds like a high-friction workflow...

Looks like the updates are out. And looks like the build failures are all related to #82608 which changed many things in and around TokenStream -- Cc @Aaron1011

error[E0046]: not all trait items implemented, missing: `SUPPORTS_CUSTOM_INNER_ATTRS`
   --> src/formatting/modules.rs:102:1
    |
102 | impl<'a> AstLike for Module<'a> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `SUPPORTS_CUSTOM_INNER_ATTRS` in implementation
    |
    = help: implement the missing item: `const SUPPORTS_CUSTOM_INNER_ATTRS: bool = true;`

error[E0277]: the trait bound `TokenStream: CreateTokenStream` is not satisfied
    --> src/formatting/macros.rs:1236:47
     |
1236 |             tokens: Some(LazyTokenStream::new(ts)),
     |                                               ^^ the trait `CreateTokenStream` is not implemented for `TokenStream`
     | 
    ::: /home/r/.cargo/registry/src/github.aaakk.us.kg-1ecc6299db9ec823/rustc-ap-rustc_ast-717.0.0/src/tokenstream.rs:143:28
     |
143  |     pub fn new(inner: impl CreateTokenStream + 'static) -> LazyTokenStream {
     |                            ----------------- required by this bound in `LazyTokenStream::new`

error[E0599]: no method named `joint` found for enum `TokenTree` in the current scope
    --> src/formatting/macros.rs:1287:24
     |
1287 |         let args = tok.joint();
     |                        ^^^^^ method not found in `TokenTree`

The missing associated const is easy, but I am not good enough at type golf to figure out the other two problems.^^

(That PR landed more than 2 weeks ago, so I am quite surprised that rustfmt wasn't marked as broken already before that... looks like using these "ap" library copies leads to huge delays and a lot of pain. FWIW in Miri we instead use rustup-toolchain-install-master and link directly against the latest internal rustc libs, that at least means we get notified immediately when a PR changes APIs that we use which makes fixing things a lot easier. But using subtrees sounds even better. :D )

[Aaron1011]

I've opened rust-lang/rustfmt#4817 to fix the rustfmt breakage

[calebcartwright]

Oh, just once a week? Wow yeah that sounds like a high-friction workflow...
looks like using these "ap" library copies leads to huge delays and a lot of pain

Yeah it's a real time sink to say the least 🙃

FWIW in Miri we instead use rustup-toolchain-install-master and link directly against the latest internal rustc libs, that at least means we get notified immediately when a PR changes APIs that we use which makes fixing things a lot easier

That's great to hear. It sounds like a much better approach for working on tools, and I'm super excited that we're finally moving rustfmt to this model. I think the additional change to a subtree will help us out a lot too, but even on the off chance we end up going back to a submodule, not being constrained by the auto publish crates is going to be tremendously helpful!

[RalfJung]

That's great to hear. It sounds like a much better approach for working on tools, and I'm super excited that we're finally moving rustfmt to this model. I think the additional change to a subtree will help us out a lot too, but even on the off chance we end up going back to a submodule, not being constrained by the auto publish crates is going to be tremendously helpful!

Agreed. :) If you have any questions about how we're solving some specific issue that comes up in Miri, feel free to ping me any time. :) (But we haven't moved to subtrees yet with Miri, so for subtree-specific questions, the clippy people would probably be the right people to ask. When clippy moved from submodule to subtree, it took a few weeks to iron out the kinks and get all the tooling around tools right again, I hope that will be easier this time.^^)

[RalfJung]

rust-lang/rustfmt#4817 landed, what's next? Does just updating the submodule work, or is there more work required?

[calebcartwright]

rust-lang/rustfmt#4817 landed, what's next? Does just updating the submodule work, or is there more work required?

We typically have to first update racer, ensure we've got a release or branch in the rustfmt repo for RLS, and then update RLS before the submodules can be updated here. It's a fun dance.

Those are other pieces are in flight already and will post updates here as they move along

[calebcartwright]

Actually it looks like we could need another round of updates for the auto publish crates. I think this may have some correlation to the items discussed/addressed in #84614 as the compile errors I'm seeing now using 2021-04-29 are related to const_fn_trait_bound on lock_api, which gets pulled in to the dep trees of both racer and rustfmt via the auto published version of rustc_data_structures (the latest of which is from Tuesday).

[RalfJung]

Argh, sorry. :/ This is caused by #84556. We can revert that PR if needed...

Is there a way to trigger a rustc-ap release? Just making a new release now would really help, all that is needed is a tiny change in rustc_data_structures...

[calebcartwright]

No worries! It's really easy for these crates to get knocked out of sync and happens fairly regularly.

It is possible to trigger an early release, and we typically do so via empty commits in the publish repo that Alex manages. I've opened alexcrichton/rustc-auto-publish#31 to get that started

[RalfJung]

@calebcartwright lock_api 0.4.4 has been released; updating to that should also solve this problem.

[calebcartwright]

@calebcartwright lock_api 0.4.4 has been released; updating to that should also solve this problem.

Thanks @RalfJung! I think on the rustfmt side we're more likely to get this sorted with the subtree conversion. I'll check with the folks that support racer to see if they'd like to go that route or wait for the next auto publish bump to address things in RLS

[RalfJung]

lock_api 0.4.4 is semver-compatible, so why can't you just bump its version in the rustc and rustfmt Cargo.lock and proceed with the already released ap crates?

[calebcartwright]

so why can't you just bump its version in the rustc and rustfmt Cargo.lock and proceed with the already released ap crates?

it's not just a matter of changing the lockfile in rustfmt. We're just about over the finish line of converting rustfmt to a subtree and in doing so have removed our dependency on the auto publish crates entirely which is why #82208 will resolve this toolstate issue for rustfmt.

in order to address the toolstate via the auto publish crate route, the below would need to be done (largely in sequence). This happens because both racer and rustfmt (historically) utilize the ap crates, RLS depends on both of those, and the ap crate versions need to be the same when building those here in tree.

1. update ap crates and lockfile update for lock_api in racer, and get a new version of racer published to crates.io (https://github.com/racer-rust/racer)
2. update a rustfmt feature branch with ap crates and changes and lockfile update in rustfmt
3. update RLS references with the updated versions of both racer and rustfmt
4. update the RLS and rustfmt submodules and lockfile here

I don't maintain/have access to racer or RLS so a few of those steps are out of my hands. It's that step 1 in the racer repo that I was alluding to earlier. I'll send them a patch later today that includes those changes, but there's a decent chance that either alexcrichton/rustc-auto-publish#31 will land and/or the scheduled Tuesday publish will happen before step 1 would be completed anyway.

[RalfJung]

Oh I see... yeah that's even worse than I thought.^^ I figured doing the subtree conversion so short before the release would be risky. But I'll leave you to it, sorry for assuming that I know more about this than I actually do.

[Mark-Simulacrum]

Release is no longer attached to the master branch (we've effectively branched) so there's no worry there.

[calebcartwright]

Oh I see... yeah that's even worse than I thought.^^ I figured doing the subtree conversion so short before the release would be risky. But I'll leave you to it, sorry for assuming that I know more about this than I actually do.

Good point about the release, glad to know that won't be a potential issue, and no worries, the auto publish crate utilization sometimes feels like a bit of a rube goldberg machine 😆