rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.2k stars 1.59k forks source link

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

Closed fasterthanlime closed 2 years ago

fasterthanlime commented 2 years ago

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:

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: https://github.com/rust-lang/rust-analyzer/pull/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.

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:

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:

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

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

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:

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:

In summary

Using cfg gates, we make rust-analyzer support:

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:

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.

bjorn3 commented 2 years ago

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 commented 2 years ago

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- commented 2 years ago

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 commented 2 years ago

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- commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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)

fasterthanlime commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

It's been renamed to rust-analyzer in https://github.com/rust-lang/rust/pull/98640.

cuviper commented 2 years ago

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 commented 2 years ago

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:

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))

matklad commented 2 years ago

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 commented 2 years ago

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:

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

matklad commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

@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 commented 2 years ago

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 commented 2 years ago

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

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:

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:

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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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.

...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 commented 2 years ago

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.

fasterthanlime commented 2 years ago

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.