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

Resolving proc-macro ABI breakage for rustc nightly users: one way forward #12803

Closed
fasterthanlime opened this issue Jul 18, 2022 · 30 comments
Closed
Labels
A-proc-macro proc macro C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )

Comments

@fasterthanlime
Copy link
Contributor

fasterthanlime commented Jul 18, 2022

I think I've found a way forward for #12579 that requires minimal changes and should make the situation better for a whole lot of folks.

Onbrandedly, I'm first going to provide way too much background.

Background

(Disclaimer: some of those details may be inaccurate - inaccuracies here do not make the whole issue irrelevant)

rust-analyzer (aka "ra") largely leverages its own infrastructure to provide code intelligence: for example, it has its own parser, separate from rustc. It leverages the chalk trait engine, whereas in rustc, chalk is behind an unstable flag, and is not fully integrated (I learned that the hard way).

When it comes to procedural macros (aka "proc macros") however, ra leverages the exact same infrastructure rustc does when actually compiling code. This lets ra expand proc macros, and "index" them, just like regular code ("Go to definition" becomes less than useful, but most everything else works fantastically).

How rust-analyzer expands macros right now

Glossing over many details, essentially:

  • The rust-analyzer binary, acting as an LSP server (hereafter referred to as ra-lsp), starts rust-analyzer proc-macro, the "proc macro server" (hereafter referred to as proc-macro-srv)
  • ra-lsp and proc-macro-srv (separate processes) communicate using newline-delimited JSON messages over stdin/stdout (the JSON API is defined in the proc-macro-api crate)
  • the "expand macro" JSON API request contains the path of a "dylib" (dynamic library / shared object) that should be used to expand the macro.
  • that dylib is the result of compiling the proc macro crate with a certain version of rustc (remember, proc macros are "just" code that transforms token streams - they end up being dylibs, TIL!)
  • proc-macro-srv opens that dylib, checks the version, and depending on the version it reads (from the .rustc section of the dylib), picks one of its abi_x_y modules to talk to it. It also finds a "registrar" symbol.
    • Note that reading the version and finding the registrar symbol involves parsing ELF/Mach-O/COFF with the object crate. I know, amazing!
  • proc-macro-srv then uses three different sets of unstable APIs from rustc/libstd's proc_macro crate to act as a "proc macro server" to the proc macro dylib. It expands the macro as requested, then writes the result as JSON to stdout, which ra-lsp is then able to process further.

And there lies the fragility. The proc-macro-srv <=> some proc-macro crate's compiled dylib interface is unstable by design, and there are no plans to stabilize it. It uses its own serialization, allows plugging your own types (an ability which ra uses, using its own "Span", "TokenStream" types from its tt crate), etc.

In fact, that interface (the "proc macro bridge") was changed multiple times in incompatible ways in the past few weeks alone — to improve performance (remove unneeded stringification/parsing, etc.). More changes can be expected in the future, for additional features.

Here's a diagram to summarize:

vscode speaks to the vscode extension over LSP protocol which is JSON. the vscode extension executes cargo check, which generates a dylib for proc macro crates. the vscode extension speaks pro-macro-api, a newline-delimited JSON, relatively stable but unversioned protocol, to proc-macro-srv, which in turns talks proc_macro bridge to the dylib. the bridge is unstable but versioned

(edit: the diagram was slightly wrong)

How a single rust-analyzer binary supports multiple rustc versions right now

Because ra should be usable across multiple rustc versions, but the "proc macro bridge" might change from one version to the next, ra adopted a "multi ABI support" proof of concept approach, merged in July 2021: #9550

Because ra is able to read the "rustc version" from proc macro crates' dylibs (e.g. lib_some_proc_macro.so) after it's been built by cargo check (or `cargo clippy, or... etc), it's able to pick from one of the ABIs it knows about: as of two hours ago, that's 1.58, 1.63, and 1.64 (the older ones were just removed).

However, there's several problems with that approach.

One ra binary vs multiple ABIs: the maintenance cost

The first is that it's a maintenance nightmare. I don't want to speak for @jonas-schievink (who I keep seeing in the Git history for that corner of ra), but I've gone through the same step myself of "upgrading the ABI" and a bunch of changes are needed.

  • Sources need to be manually copied from https://github.com/rust-lang/rust/tree/master/library/proc_macro/src to the rust-analyzer repo, in a new abi_x_y directory
  • lib.rs is renamed to mod.rs
  • All the unstable features need to be removed in some way, stuff like:
    • #[stable()] and #[unstable()] attributes
    • negative impls (!Send and !Sync)
  • Some symbols need to be re-exported under different names
  • etc.

As I was writing this, a PR You can see landed. Since it's mostly additions, you don't really see all the manual work that goes into adjusting upstream code just so it builds within rust-analyzer.

This is not the main problem I'm trying to solve. In fact, the approach I'm suggesting doesn't immediately remove this work. I just think it's a shame.

One ra binary vs multiple ABIs: the rustc nightly issue

At the time of this writing, r-a has been broken on nightly for roughly a month, cf. #12600.

"Broken" is an oversimplification, let me explain:

  • For a while, r-a (any version) only worked with rustc nightly 2022-06-08. For any newer rustc, it would fail to expand proc macros.
  • On 2022-07-12, fix: Update 1.63 proc macro ABI to match rustc #12747 was merged, which was thought to address the issue. But it didn't: it added the "rustc 1.63 beta" ABI, which is not what nightly uses (as explained in feat: Support the 1.64 nightly proc macro ABI #12795)
  • Because today is monday, today (and for another week), r-a stable is broken for rustc nightlies between 2022-06-08 (which used to work) and 2022-07-18

If my understanding is correct, within 24h, there'll be a new "release preview" version of rust-analyzer, and that one will support rustc nightly 2022-07-18. I'm not sure if it'll support nightly 2022-06-08, because the PR only "adds the 1.64 ABI", and "the 1.63 ABI" as it was there previously didn't seem to work with 2022-06-08.

The problems don't stop there. There's no way to know if a version of r-a is compatible with a version of rustc. Versions of rustc that used to work can get broken by r-a upgrades. (One would have to pin the r-a vscode extension version to some older build, since the r-a binary is bundled with the extension now) — so "just picking another nightly" (already not an option for a lot of users) is not even a fix. Even if a "compatibility table" was maintained, it would have to be revised every week (if you only care about r-a stable) or every day (if you care about r-a nightly), and I'm not aware of any infrastructure in place to do automated testing of r-a against rustc versions.

That's still not it. Desperate, I tried installing rust-analyzer from rustup, since it is a rustup component now, and pointing the rust-analyzer vscode extension to it (via the rust-analyzer.server.path vscode setting). But that doesn't work either, because that rust-analyzer binary is also using the "multi ABI support" scheme, and it fails in exactly the same way.

(Also, when ra's proc-macro-srv becomes incompatible with rustc's proc_macro bridge, IntelliJ Rust breaks too, since they literally pull in the proc-macro-srv crate).

Listing concerns

I've reread #12579 maybe 15 times, because there's a lot of concerns here from various sides and I wanted to make sure that whatever I suggested would address them all.

Here are my concerns:

  • AM001 (my first name is "Amos"): I want rust-analyzer to never break for nightly users. That means my work colleagues, rustc contributors (it's possible r-a this doesn't work for other, unrelated reasons), anyone using nightly. There's no technical reason it should ever break.

Here are the concerns I've been able to gather from #12579, from the "rustc project" (rust-lang/rust) side of things:

  • RU001: We do not want to stabilize the proc_macro bridge. It is inherently unstable so that it may change in the future, for performance or feature reasons. source
  • RU002: Shipping any additional files with the rustc dist component falls under T-infra, likely requires coordination across several teams. source

Here are the concerns I've been able to gather from #12579, from the rust-analyzer side of things:

  • RA001: We want rust-analyzer to be backwards- and forwards-compatible over "a few rustc versions"
    • Example: some project is pinned to rustc 1.56, yet 1.62 just came out - a recent r-a should still work, with all the new features added since. source
  • RA002: We want rust-analyzer to work regardless of whether the rust-analyzer rustup component is installed. (rust can be installed through other sources than rustup.) source
  • RA003: We want rust-analyzer to work in "VSCode workspaces" that may contain multiple Cargo workspaces, each using different rust toolchain versions. source
  • RA004: It would be nice if rust-analyzer would build with rustc stable, not requiring nightly features or RUSTC_BOOTSTRAP=1. source

A proposed way forward

I'm suggesting adding a new Abi implementation to proc-macro-srv that uses the sysroot version of proc_macro instead of copying+tuning upstream sources regularly.

I've gone ahead and done that in a fork:

This was significantly less work than the other approach (copying+tuning, cf. this branch) and works flawlessly, today, on rust 2022-07-18.

That implementation will:

  • Keep working in case of ABI-breaking but API-compatible changes to the proc_macro bridge
  • Stop building in case of API-breaking changes to the proc_macro bridge

If it stops building, "rust-analyzer" will show as "missing" in the rustup component history table. This serves as a signal for users that rust-analyzer cannot be used with that rustc nightly.

Breakages will last at most a week, and should be fixed as part of rust-analyzer's already-existent weekly (-ish?) merge with rust-lang/rust: see these pull requests.

I'm suggesting making that Abi implementation be cfg-gated, and that it be the only Abi built (and selected at runtime) for the "rust-analyzer rustup component".

I'm suggesting the non-rustup build of rust-analyzer not set the relevant cfg feature, which would allow it to keep building under rustc stable, and keep using the "multi ABI" compatibility scheme.

Furthermore, I'm suggesting a rust-analyzer shim be added to rustup, so that one can invoke rust-analyzer +nightly just like one can do rustc +nightly or cargo +nightly - but more importantly, so that the rust-analyzer in $PATH is rustup-aware (picks up rust-toolchain.toml, etc.) and all that's needed to use it is to add the rust-analyzer component to rust-toolchain.toml and set rust-analyzer.server.path to "rust-analyzer").

Let's review concerns one by one:

  • AM001: As a user, I am now very happy and relieved. By looking at "rustup component history", I know which rustc versions to pick and which to avoid if I want a working rust-analyzer. I can opt into using the rustup version of rust-analyzer with one change to my rust-toolchain.toml and .vscode/settings.json). rust-analyzer proc macros never ever break for me ever again.
  • RU001: This doesn't change - the proc_macro bridge is still unstable. r-a's copypasta approach to "multi ABI compatibility" is still mildly iffy to everyone, but nobody is stabilizing anything.
  • RU002: No additional files are shipped under the "rustc" dist component. The existing rust-analyzer component is used, with the existing merge and release process. No coordination is required across team (we just need to ask the rustup team nicely for a rust-analyzer shim) and there's actually a good chance of this happening.
  • RA001: vscode-marketplace-distributed RA is still backwards-/forwards-compatible with rustc versions just like it was before. This works "well enough" for stable rustc versions (since there's ample time to upgrade proc-macro-srv before stable is minted) and the majority of ra users don't ever need to learn about this issue.
  • RA002: this still works, too. the rust-analyzer binary is still bundled with the vscode extension and is the default.
  • RA003: I believe the "VSCode workspace" feature would spin up multiple rust-analyzer instances anyway. Some maybe the bundled version, and some may be from rustup (according to vscode settings) - I don't believe r-a currently has any code specifically to support that, and I think things should not end up any worse or better than they are now.
  • RA004: rust-analyzer standalone still builds fine under rustc stable, since all the "sysroot proc_macro" code is cfg'd away, including the unstable opt-ins.

In summary

Using cfg gates, we make rust-analyzer support:

  • multiple ABIs (just like now) in the standalone (vscode marketplace) build
  • the exact same ABI as rustc in the rustup component build

The former has no changes at all, and retains all the things the r-a team cares about (as per #12579).

The latter means r-a is usable with all rustc nightlies, unless the API breaks, in which case it's the component is missing, which can be checked by consulting the rustup component history.

proc_macro bridge API breakages are resolved during the regular rust-lang/rust-analyzer sync with rust-lang/rust. The code still canonically lives under rust-lang/rust-analyzer (and as a git submodule in rust-lang/rust).

A rustup shim is created for rust-analyzer, which makes opting into "rustup rust-analyzer" as simple as having a rust-toolchain.toml with:

[toolchain]
channel = "nightly-2022-07-18" # or whatever
components = [ "rustfmt", "clippy", "rust-analyzer" ]

And a .vscode/settings.json with:

{
  "rust-analyzer.server.path": "rust-analyzer"
}

Bonus: what if we don't want a shim?

The only real drawback I can see here is that r-a devs are used to installing rust-analyzer to ~/.cargo/bin/. A rustup shim might overwrite that, or just fail to install.

There's several alternatives that would work just as well:

  • Provide a different setting: "rust-analyzer.server.fromRustup": true
  • Extend rustup run to accept a special toolchain string that means "whatever is right for the current directory" (it currently wants a toolchain string like "nightly" or "stable", which is not what you want if you have a rust-toolchain.toml file), and make sure the rust-analyzer vscode settings allow passing a "shell command" rather than just an "executable path"

The point is to avoid having to provide user-specific absolute paths like these:

{
  "rust-analyzer.server.path": "/home/amos/bearcove/rust/build/x86_64-unknown-linux-gnu/stage1-tools-bin/rust-analyzer"
}

...which would make it impossible to share settings across multiple developers of the same Rust project.

@Veykril Veykril added the A-proc-macro proc macro label Jul 18, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jul 18, 2022

The only real drawback I can see here is that r-a devs are used to installing rust-analyzer to ~/.cargo/bin/. A rustup shim might overwrite that, or just fail to install.

For rustfmt (or was it clippy) I believe there is logic to make rustup not install the shim if it is already manually installed as in the past you had to use cargo install rather than rustup component add to get it.

@fasterthanlime
Copy link
Contributor Author

forgot this bit:

Bonus 2: what about a proc-macro-srv component?

In #12579, it was suggested to ship a proc-macro-srv component, which rust-analyzer could then use. That'd move from a single binary with a proc-macro subcommand, to two binaries: one being rust-analyzer, and the other being proc-macro-srv.

I'd like to keep that as out of scope for that discussion since, "in theory it's nice" but it's also a way of stabilizing (a subset of) the proc macro bridge, it's a new rustup component binary file, etc. etc.

I do think r-a's "proc-macro-api" JSON API should be versioned so it's future-proofed (and rust-analyzer 2023-01-01 can be used with a rustc from six months ago), but it's not a prerequisite for the scheme I'm suggesting here.

@lf-
Copy link
Contributor

lf- commented Jul 18, 2022

Re: the last concern with absolute paths, could we make it resolve stuff with PATH? Or possibly leverage rustup toolchain link?

You can already do like, rustc +dev ...

As a (former, burnout sucks) r-a dev I always used the absolute path in my config for prototyping so I'm not sure if the cargo install thing is true for everyone. I don't like cargo install because it clean builds every time.

@bjorn3
Copy link
Member

bjorn3 commented Jul 18, 2022

Re: the last concern with absolute paths, could we make it resolve stuff with PATH?

It already does so, but without rustup shim, there is no rust-analyzer binary in your PATH in case you are getting it from rustup.

Or possibly leverage rustup toolchain link?

rustup toolchain link only adds a symlink from ~/.rustup/toolchain/<toolchain> to the toolchain directory. There is no way to overlay one component on top of an existing local toolchain.

@lf-
Copy link
Contributor

lf- commented Jul 18, 2022

Re: the last concern with absolute paths, could we make it resolve stuff with PATH?

It already does so, but without rustup shim, there is no rust-analyzer binary in your PATH in case you are getting it from rustup.

Or possibly leverage rustup toolchain link?

rustup toolchain link only adds a symlink from ~/.rustup/toolchain/<toolchain> to the toolchain directory. There is no way to overlay one component on top of an existing local toolchain.

Could you configure the language server in your editor to be rust-analyzer +dev and have it hit some project toolchain or something else with no changes? Or is there some further fun there?

@bjorn3
Copy link
Member

bjorn3 commented Jul 18, 2022

That works provided that the dev toolchain has rust-analyzer added to it. I'm not sure how to convince ./x.py to add tools to the sysroot it created without using ./x.py install or something like that.

@flodiebold
Copy link
Member

I think this would be an ok temporary solution, but does not solve all the concerns that making the proc macro server a separate binary solves.

In particular I think you misunderstood RA0003 a bit. I guess I shouldn't have used the term "VSCode workspace" -- just opening a folder containing multiple Cargo projects is enough. rust-analyzer can handle a workspace containing multiple separate crate graphs using different rustc versions just fine, but if the rust toolchain is being overridden in one of the subfolders your approach would not work.

The main problem with this approach in my opinion is that it requires special configuration from the user if they want nightly support to work, though. (And that it fixes the used rust-analyzer version to the nightly version if you want nightly support.)

I think the rust-analyzer shim is being added, btw.

@lnicola
Copy link
Member

lnicola commented Jul 18, 2022

Breakages will last at most a week, and should be fixed as part of rust-analyzer's already-existent weekly (-ish?) merge with rust-lang/rust: see these pull requests.

One remark about this: what's being merged upstream is the stable release, so if any upstream changes make it fail to build, it will be fixed at the earliest one week later (in the next stable version) + 2 days or so to get merged.

@fasterthanlime
Copy link
Contributor Author

In particular I think you misunderstood RA0003 a bit. I guess I shouldn't have used the term "VSCode workspace" -- just opening a folder containing multiple Cargo projects is enough. rust-analyzer can handle a workspace containing multiple separate crate graphs using different rustc versions just fine, but if the rust toolchain is being overridden in one of the subfolders your approach would not work.

Ah, I see what you mean. I've never seen r-a work in that scenario but I believe you.

Let me then withdraw that section and emphasize that, although the suggestions I'm making don't address all concerns in an ideal way, it doesn't make anything worse, and it makes the "one cargo workspace" situation much better with minimal configuration (and very few code changes / coordination requires across rust projects).

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jul 18, 2022

And that it fixes the used rust-analyzer version to the nightly version if you want nightly support.

Forgot to address that: that's not an inherent limitation of my proposal. You could have a separate setting for the "proc macro server command" — you would then use the main command of the bundled rust-analyzer (up-to-date with new features) and the proc-macro subcommand of the rustup component binary.

(This requires versioning the proc-macro-api JSON interface to be robust)

@Veykril Veykril added the C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) label Jul 18, 2022
@fasterthanlime
Copy link
Contributor Author

One remark about this: what's being merged upstream is the stable release, so if any upstream changes make it fail to build, it will be fixed at the earliest one week later (in the next stable version) + 2 days or so to get merged.

Even if nothing changes about the merge process, I'd still see it as a win. API breakage would show up early in rustc's own CI builds, giving r-a maintainers a heads up to upgrade the "sysroot" ABI before it lands in stable (what gets merged upstream).

But let me repeat that: even if no extra work is done, I still think my proposal is a win for anyone relying on rustc nightly + r-a. Because at least you know which versions do work, and they don't break retroactively.

@edwin0cheng
Copy link
Member

edwin0cheng commented Jul 18, 2022

Provide a different setting: "rust-analyzer.server.fromRustup": true

Is it possible only run on rust-analyzer proc-macro , but not the normal rust-analyzer ?

It is because we don't need the full rust-analyzer matches the rustc version, but only the proc abi part.

@flodiebold
Copy link
Member

Is it possible only run on rust-analyzer proc-macro , but not the normal rust-analyzer ?

There is actually already a rust-analyzer.procMacro.server setting that would pretty much do that.

@fasterthanlime
Copy link
Contributor Author

@lnicola what's the deal with the rust-analyzer-preview rustup component, btw? It's been missing for a while. From which tree is it built? Could fixes be landed sooner on there?

@lnicola
Copy link
Member

lnicola commented Jul 18, 2022

It's been renamed to rust-analyzer in rust-lang/rust#98640.

@cuviper
Copy link
Member

cuviper commented Jul 19, 2022

The rustup components history is one thing, but I think we should also get this going in rust-toolstate, so I've opened rust-lang/rust#99444. However, I'm not sure if that will really work, because I get a bunch of empty "Expect:" errors that I don't understand when I run it in-tree... help would be appreciated!

@fasterthanlime
Copy link
Contributor Author

The rustup components history is one thing, but I think we should also get this going in rust-toolstate

@jyn514 mentioned preferring subtrees over toolstate on zulip

After learning more about subtrees and checking in with @mystor (who's been making changes to proc_macro bridge recently), I agree.

This would mean:

  • rustc PRs can't break the RA rustup component anymore, they'd update the proc-macro-srv too - that can be synced to the RA repo whenever convenient
  • RA features are developed in the RA repository just like now, they can be synced to the rust repo whenever convenient

This is an ideal solution imo, and what I'd expect from an official component (I believe it's also in the spirit of what @eddyb hinted at in #12579).

(The step after that is to teach RA to try and use that component first (for the relevant toolchain, depending on which crate/cargo workspace the macro being expanded is in) and only fall back to its own proc-macro-srv if the component is unavailable (for the time being))

fasterthanlime added a commit to fasterthanlime/rust that referenced this issue Jul 19, 2022
This adds support for testing `rust-analyzer` in-tree.

Test rust-analyzer in CI

Set CARGO_WORKSPACE_DIR directly in `prepare_tool_cargo`

cf. rust-lang#99444 (comment)

cf. rust-analyzer/expect-test#33

Various x.py changes for Submodule => InTree, and removing toolstate

Introduce `rust-analyzer/in-rust-tree` cargo feature

This allows skipping ra's "check_merge_commits" test for now.

Later, it might be used to link against `extern proc_macro` directly,
cf. rust-lang/rust-analyzer#12803

More cleanups around the RustAnalyzer tool in bootstrap

Start fixing lints

Deny the same warnings x.py does in all RA crates

Warn on x.py warnings, don't deny

cf. https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/rust-analyzer.20as.20a.20subtree.3A.20experimental.20PR.20status/near/290114101

Fix rust_2018_idioms with cargo fix

Add warning groups for more RA crates

Fix more 2015 idioms (with 2018_idioms, naming is fun)

RA passes all x.py warnings

Fix formatting after fixing idioms/additional warnings

Enable in-rust-tree feature, RA tests pass

Print stdout+stderr when nested cargo invocation fails
@matklad
Copy link
Member

matklad commented Jul 23, 2022

I think this would be an ok temporary solution, but does not solve all the concerns that making the proc macro server a separate binary solves.

I admittedly have been ignoring this discussion altogether and haven't catch up, but my partially-uninformed opinion is that, medium term, I would strongly prefer to have just the proc-macro server binary be completely moved out of rust-analyzer and into rustc, without any code shading beween ra and that binary (so, separate structs for JSON serilization in the proc-macro server, and in ra client). Entangling rust-analyzer and rustc build process together is something I feel we should try very hard to avioid.

@fasterthanlime
Copy link
Contributor Author

fasterthanlime commented Jul 23, 2022

Entangling rust-analyzer and rustc build process together is something I feel we should try very hard to avoid.

The "sysroot ABI" isn't build in RA CI (it cannot be built there, since it's an unstable interface). There's discussion around adding a proc-macro-srv binary to the rustc component, built in Rust CI. @pietroalbini / @jyn514 don't seem to think it's a big issue, since RA will be an in-tree tool that should, by definition, always build.

Whether the sources for proc-macro-srv live under src/tools/rust-analyzer or src/something-else doesn't really make a difference to the build process afaict.

However, moving proc-macro-srv as-is completely out of src/tools/rust-analyzer means copying a bunch of crates: tt, mbe and proc-macro-api come to mind, there's probably others. And then we have another "unstable interface that's not tested anywhere in CI" situation, just like when RA was a submodule.

If we turn proc-macro-srv into a bin+lib package:

  • Rust CI tests the "sysroot ABI"
  • proc-macro-srv is available for all stable/beta/nightly toolchains starting from the day that PR lands
  • The bridge never ever breaks
  • The VS Code Marketplace version of rust-analyzer simply needs to learn to look into the sysroot, under libexec, for a proc-macro-srv executable, and if it can't find one, fall back to its own proc-macro subcommand

That seems like a pretty ideal solution. I'm currently researching to find how large a standalone proc-macro-srv binary would be.

@matklad
Copy link
Member

matklad commented Jul 23, 2022

However, moving proc-macro-srv as-is completely out of src/tools/rust-analyzer means copying a bunch of crates: tt, mbe and proc-macro-api come to mind, there's probably others

Yeah, that's why "without any code shading between ra and that binary" bit. It is important that the only shared artifact between rust-lang/rust and rust-analyzer is just the informal definition of the JSON-over-stdio API. No crates should be shared. This should work as if rustc and rust-analyzer are implemented in different programming languages.

@pietroalbini
Copy link
Member

As long as the binary we distribute in the rustc component is a different binary than rust-analyzer, how it's built and where its souce code is located is just an implementation detail that can be changed without any user visible impact.

Of course that assumes the separate implementation can keep the same API as the current implementation, but that seems a way bigger effort. As this binary is just an implementation detail, as long as versioning is present somehow there would be no problem breaking compatibility if the need arises.

@fasterthanlime
Copy link
Contributor Author

It is important that the only shared artifact between rust-lang/rust and rust-analyzer is just the informal definition of the JSON-over-stdio API.

@matklad can I ask why / what this solves?

proc-macro-srv relies on 3 sets of unstable proc_macro APIs. It is absolutely sharing code with rustc (by copying it, then modifying it every time there's a proc macro bridge change). Converting rust-analyzer to a git subtree solves that problem: proc-macro-srv still lives under rust-analyzer, but maintaining the "bridge" aspect of it falls on rust-lang/rust contributors.

Building a proc-macro-srv binary from rust-analyzer sources in Rust CI doesn't change the coupling at all: only rust-analyzer crates (and extern proc_macro) are involved. We know that it works because rust-analyzer tests also get run in Rust CI, against the current proc_macro bridge.

@fasterthanlime
Copy link
Contributor Author

Here's what I'll say about the current plan:

  • It doesn't change how rust-analyzer is being developed at all, nor how its CI is being run
  • It doesn't change rust-analyzer's release cadence at all (or how often features reach users)
  • It's not considered problematic by folks who maintain the rust CI/build system
  • It will definitely fix proc macro ABI incompabilities for all rustc versions after it lands (and older versions will still work b/c it'll fall back to the "multi ABI support" scheme)

@matklad I think the discussion will be more constructive if you voice concerns that aren't listed here / ask clarifying questions about the current plan. What you're suggesting earlier is a lot of work, and I don't see the upside right now.

@matklad
Copy link
Member

matklad commented Jul 23, 2022

can I ask why / what this solves?

Two things:

First, no-code-sharing constraint significantly simplifies the build. I see the fact that rust-analyzer tests are run in two contexts (rust-analyzer's CI and rustc's CI) as a complexity multiplier. This adds a very new failure mode when a test works in rust-analyzer's CI, but breaks in rustc CI due to some obscure difference in the environment. Similarly, #[cfg(feature = "in-rust-tree")] forks the build process into two, conditional compilation combinatorially increases the amounts of configuration.

Second, rust-analyzer is very cognizant of boundaries between systems. "Accidental" code re-use is something which adds significant maintenance burden over time, or outright precludes usage in unexpected, new contexts. It could have been the case that rust-analyzer is implemented in Kotlin and rustc in OCaml. Even in this dual-language setup, it would've been totally possible to have proc-macro srv in OCaml and proc-macro client in Rust. From the perspective of boundaries, there shouldn't be any difference in how IntelliJ Rust and rust-analyzer handle "expand proc macros" task.

That is, I don't quite agree with

It doesn't change how rust-analyzer is being developed at all, nor how its CI is being run

The fact that the code in this repo is being used somewhere else is a very significant change. It adds a new dimension to the configuration space.

I understand that rewriting proc-macro-srv in rustc is immediate work, more than just gluing two repos together. I maintain though, that, integrated over time, that'll be less work.

I also wouldn't say that it should be a lot of work? The srv is basically event loop + dylib loading + interaction with abi + serialization. event loop & dylib loading seem relatively small, abi is nasty, but it actually becomes easier as you care only about one, and serialization is just some boilerplate, and, again, it becomes easier as you don't have to go thorough tt intermediary and can just directly convert from ABI to JSON or bincode.

@fasterthanlime
Copy link
Contributor Author

First, let me acknowledge that you're basically advocating for things that are, in isolation, good engineering principles:

  • loose coupling / well-defined, small, versioned boundaries
  • being wary of combinatorial explosion

However, we can't have that conversation and simply ignore the historical context, or human factors at play here.

Here's what historically happened.

rust-analyzer, before it became an official Rust project, started using an unstable, rustc-internal interface: the proc_macro bridge. As far as I can tell, nobody on the rust team was consulted about doing that. But then again, rust-analyzer needs to do that: otherwise, it's blind to any code generated/transformed by a proc macro.

Because the bridge was left "mostly alone" for a while, that hack "worked" for a while. The RA side was updated for a couple point releases (1.58 for example). Recently, because proc macro bridge changes became more frequent, that hack worked less and less, to a point where others outside the RA project started noticing it and politely enquired.

Earlier discussions about the situation always ended in a dead-end: rust-analyzer contributors didn't want to compromise any of the nice things they had (releasing a preview version daily, and a stable version weekly, which doesn't match up with rustc's release schedule via rustup), whereas rustc contributors didn't want to stabilize any part of the proc macro bridge: it is unstable by design and is going to stay that way.

Meanwhile, RA became almost unusable for anyone on nightly. Despite @jonas-schievink's best efforts to update the "1.64" ABI, RA broke retro-actively for users of pinned nightlies because there is no such thing as an 1.64 ABI.

Essentially, we had a situation where neither side was particularly enthusiastic at the idea of recognizing that rust-analyzer used an unstable interface, yes, but that it absolutely needed to do so, and that we needed an incremental plan forward.

Several essential "aHA! moments" happened after that:

  • We realized that if we simply used extern proc_macro, we didn't need to copy/fixup any proc_macro bridge code to the r-a repository, nor rely on reading a section from the dylib to select an ABI version (this was never a good approach for nightly/beta, since a "numbered ABI" doesn't exist until stable is released)
  • We realized we didn't have to couple the version of "rust-analyzer LSP / code intelligence" with the version of "proc-macro-srv" used.
  • We realized that if we ran rust-analyzer's test suite in CI, we could catch any breakage early
  • We realized that shipping the current proc-macro-srv as-is as a rustc component was an option

Creating a proc-macro-srv binary from scratch that doesn't rely on any RA crate is not only a lot of work (that nobody volunteered for!), it's a lot more involved than your comment makes it sound.

RA and rustc currently have wildly different ideas of what token streams, token trees, groups, literals, idents should look like. The "rust analyzer proc macro server" takes a lot of shortcuts, some of which are going to become problematic pretty soon.

A proc macro server is supposed to lex literals (proc-macro-srv doesn't). It's supposed to normalize and validate idents (proc-macro-srv doesn't). It's supposed to provide the same Debug implementation rustc does (etc.). We only discovered this in the past few days of me actually going in and doing that research work.

The combinatorial explosion problem also isn't as bad as you seem to think it is. For the time being, proc-macro-srv is being built both in RA CI (where the "multi ABI scheme" is enabled) and in Rust CI (where the "sysroot ABI" is added on top). This is by design. The sysroot ABI is just dead code in RA CI, it's not an additional burden for you. Any conflicts will show up during an "ra=>rust" sync.

Everything else is being built exactly as-is. Being part of Rust CI actually has a significant upside for RA: if any changes to rustc were to break the RA codebase, it would get fixed before it's even merged to master (much before landing in nightly/beta). At no additional maintenance cost to you.

Trying to convince someone to build a drop-in proc-macro-srv replacement with zero r-a dependencies would've been a multi-week (multi-month?) endeavor. There's a reason it hasn't happened yet.

Instead, here's the timeline we're looking at:

  • Within a week, we'll have a drop-in proc-macro-srv binary shipped with rustc nightly
  • rust-analyzer will know to look for it and use it
  • A new JSON config option will be added so users can opt into using that path
  • That option can be made default once we've verified that it doesn't cause any problems
  • Over time, we can work on shrinking that proc-macro-srv binary
  • Over time, we can work on addressing the "shortcuts" taken in proc-macro-srv
  • Over time, we can stop shipping a proc-macro subcommand in rust-analyzer, because proc-macro-srv is always present. This happens once the "minimum-supported rust version" for rust-analyzer becomes a version that has that component
  • In the meantime, RA is always usable on nightly

Much, much later, at any time really, we can start making proc-macro-srv more self-contained. We can provide a more formal definition for the JSON interface (or better yet, move away from JSON). As long as it's backwards-compatible, anything can happen here.

Because (at that point in time) the "multi ABI scheme" doesn't need to be supported anymore, as you pointed out, we don't have to maintain 3-4 versions of proc-macro-srv, just the one.

Eventually, it might move into something like src/tools/proc-macro-srv and disappear from the rust-lang/rust-analyzer repository altogether.

But that'll take time. A lot of time. The path I've pushed towards fixes nightly breakage in a matter of days. We can absolutely aim for the goals you've laid out over time - there's a path to that, too. In due time.

@fasterthanlime
Copy link
Contributor Author

We can absolutely aim for the goals you've laid out over time - there's a path to that, too. In due time.

In fact, let me make a slightly stronger commitment here: once the current plan is complete and we have a "rust-analyzer always works with rustc nightly" situation going on, I will be happy to personally work towards making proc-macro-srv more self-contained and eventually moving it out of rust-lang/rust-analyzer altogether.

(Although keep in mind you won't want to actually remove it too soon, since you want rust-analyzer to be compatible with older rustcs for the time being, which won't have this component).

@matklad
Copy link
Member

matklad commented Jul 23, 2022

once the current plan is complete and we have a "rust-analyzer always works with rustc nightly" situation going on, I will be happy to personally work towards making proc-macro-srv more self-contained and eventually moving it out of rust-lang/rust-analyzer altogether.

Uhu, this is roughly what I had in mind for medium term.

Regarding the human factors, the balances which seems reasonable to me are:

  • there's zero effort from rustc side to help rust-analyzer, its on ra to keep itself compatible with stable rust, its on enthusiastic nightly users to keep ra compatible with nightly (the original design with copy-pasted code)
  • there's a minimal effort from rustc side to help ra: namely, the crate which defines the proc-macro ABI is compatible with stable and auto-published without any API stability (this is how rustc_lexer works). It's on ra maintainers to adapt ap crates for stable, on nightly users to exert extra efforts to support nightly
  • there's full support from rustc, via exposing proc-macros as unstable-JSON over stdio. Again, its on ra devs to adapt to stable JSONs, and on nightly users to adapt to nightly JSONs. However, by shifting the JSON maintenance over to rustc folks, it is expected that in practice the amount of breakage would be low.

The long-term balance where there's a shared dependency between rustc and ra such that change in one can break the build of the other (outside of usual language stability guarantees) would not seem completely reasonable to me, especially if the goal is to support nightly users.

As long as it's backwards-compatible, anything can happen here.

I would be against guaranteeing any sort of stability for this JSON interface. rustc folks should feel free to change this details whenever, the onus on IDE folks (and nightly users :) ) to adapt to changes. The expectation here would be that de-facto breaking changes would be rare though.

@fasterthanlime
Copy link
Contributor Author

I would be against guaranteeing any sort of stability for this JSON interface. rustc folks should feel free to change this details whenever, the onus on IDE folks (and nightly users :) ) to adapt to changes. The expectation here would be that de-facto breaking changes would be rare though.

This would create another compatibility problem though — if the JSON interface changed in rust, the VSCode marketplace version of rust-analyzer would suddenly be incompatible with newer nightlies. Once rust-analyzer is brought up-to-speed, it becomes incompatible with older nightlies: we've created the same situation over again.

This could be solved by forcing nightly users to use the rustup-provided version of rust-analyzer, but that aligns the release cycle of RA features with rust's own release cycle, which is something RA folks were pretty vocal about wanting not to happen.

The long-term balance where there's a shared dependency between rustc and ra such that change in one can break the build of the other (outside of usual language stability guarantees) would not seem completely reasonable to me, especially if the goal is to support nightly users.

To be clear, this is not what's happening here. It's not like Rust CI is downloading RA crates from somewhere - it has its copy of all of them. Any breakage would be noticed when doing a sync in either direction, and could be addressed at that point in time.


Of the three "balance options" you suggested, none work in practice.

  • zero-support was the status quo until I started pushing for change. it clearly didn't work at all for nightly users. I also feel like it's an unfair characterization of the relationship between the Rust & RA projects - again, the first occurence of reaching out across projects is RA should use the toolchain's proc_macro instead of copying library/proc-macro. #12579
  • minimal effort is not minimal at all and has been turned down repeatedly. It's simply not an option.
  • full support has the same compatibility issues (see above)

...unless I'm seriously misunderstanding something — I'm not sure what you mean by "[it's] on nightly users to adapt to nightly JSONs".

@workingjubilee
Copy link

My primary way of adapting to changes that break rust-analyzer is disabling RA, and this comes up far more often than any other effort I have to go through to adapt to rustc nightly features changing, even compared to maintaining a nightly-only crate. Several others have expressed similar sentiments to me. People who use nightly are slightly more likely to be experienced Rust programmers and thus regard rust-analyzer's aid as less essential.

But I do also submit patches to rustc and the standard library. It is unlikely a patch I submit will be of the kind that breaks RA, but I already know my way around the compiler enough that I could easily fix an in-tree rust-analyzer simply by observing the parallel points.

So to dip back to this,

First, no-code-sharing constraint significantly simplifies the build. I see the fact that rust-analyzer tests are run in two contexts (rust-analyzer's CI and rustc's CI) as a complexity multiplier. This adds a very new failure mode when a test works in rust-analyzer's CI, but breaks in rustc CI due to some obscure difference in the environment. Similarly, #[cfg(feature = "in-rust-tree")] forks the build process into two, conditional compilation combinatorially increases the amounts of configuration.

I would like to note that multipliers to complexity can be less than 2, and that multipliers of utility can be 0 (or, if you are like me, sometimes also NaN). While adding such configuration does increase complexity slightly, I regard the forcing factor of building in multiple environments as a good thing: even if tiny points become more aggressively configured, it encourages reducing the overall dependency on small environmental configuration details, and thus overall better factorization.

bors added a commit that referenced this issue Jul 26, 2022
Add `rust-analyzer-proc-macro-srv` binary, use it if found in sysroot

This adds a `bin` crate which simply runs `proc_macro_srv::cli::run()` (it does no CLI argument parsing, nothing).

The intent is to build that crate in Rust CI as part of the `dist::Rustc` component, then ship it in the sysroot: it would probably land in something like `~/.rustup/toolchains/nightly-2022-07-23-x86_64-unknown-linux-gnu/libexec/proc-macro-srv-cli`.

This makes rust-lang/rustup#3022 less pressing. (Instead of teaching RA about rustup components, we simply teach it to look in the sysroot via `rustc --print sysroot`. If it can't find `proc-macro-srv-cli`, it falls back to its own `proc-macro` subcommand).

This is closely related to #12803 (but doesn't close it yet).

Things to address now:

  * [ ] What should the binary be named? What should the crate be named? We can pick different names with `[bin]` in the `Cargo.toml`

Things to address later:

  * Disable the "multi ABI compatibility scheme" when building that binary in Rust CI (that'll probably happen in `rust-lang/rust`)
  * Teaching RA to look in the sysroot

Things to address much, much later:

  * Is JSON a good fit here
  * Do we want to add versioning to future-proof it?
  * Other bikesheds

When built with `--features sysroot` on `nightly-2022-07-23-x86_64-unknown-linux-gnu`, the binary is 7.4MB. After stripping debuginfo, it's 2.6MB. When compressed to `.tar.xz`, it's 619KB.

In a Zulip discussion, `@jyn514` and `@Mark-Simulacrum` seemed to think that those sizes weren't a stopper for including the binary in the rustc component, even before we shrink it down further.
@fasterthanlime
Copy link
Contributor Author

With these two final pieces landing:

Starting from nightly-2022-07-29, rust-analyzer preview should find the standalone proc macro server in the sysroot and use it. rust-analyzer stable should catch up on August 1.

It is with much relief that I'm closing this issue.

tash-2s added a commit to tash-2s/open-emoji-battler that referenced this issue Aug 22, 2022
1.65.0-nightly (878aef79d 2022-08-20)

for rust-analyzer proc-macro error: rust-lang/rust-analyzer#12803
forbesus added a commit to forbesus/OpenEmojiBattler that referenced this issue Feb 18, 2024
1.65.0-nightly (878aef79d 2022-08-20)

for rust-analyzer proc-macro error: rust-lang/rust-analyzer#12803
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macro proc macro C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) )
Projects
None yet
Development

No branches or pull requests