Stream: t-compiler/rust-analyzer

Topic: Fixed crate graphs and optional builtin crates


Daniel Mcnab (Mar 06 2021 at 10:28, on Zulip):

Regarding #6714:
Would a possible solution be to have a section in Cargo.toml which lists the 'optional' crates which your crate depends on (primarily the rustc_* internal crates).

That is, additionally (or even only) run adding rustc_private crates to packages which have:

[package.metadata.rust-analyzer]
rustc_private = true

in their Cargo.toml

Daniel Mcnab (Mar 06 2021 at 12:17, on Zulip):

Initial implementation in https://github.com/DJMcNab/rust-analyzer/tree/rustc_private_metadata

Daniel Mcnab (Mar 06 2021 at 16:00, on Zulip):

Now in https://github.com/rust-analyzer/rust-analyzer/pull/7891

Jonas Schievink [he/him] (Mar 06 2021 at 16:05, on Zulip):

Hmm, this almost sounds like we need to revamp sysroot handling to be closer to what rustc does

Jonas Schievink [he/him] (Mar 06 2021 at 16:05, on Zulip):

It seems like it isn't entirely correct to treat the CrateGraph as purely an input to the system generated by Cargo or another build system

Daniel Mcnab (Mar 06 2021 at 16:07, on Zulip):

That is probably so. In this case I want a minimum changeset which can even partially improve working with the rustc_private feature. I'm trying to work out what the root of the rustc-dev package is - I suspect it would be rustc_driver, but I can't be certain because the code which manages this is somewhat convoluted

Joshua Nelson (Mar 06 2021 at 16:56, on Zulip):

I'm trying to work out what the root of the rustc-dev package is - I suspect it would be rustc_driver

What do you mean by the root?

Joshua Nelson (Mar 06 2021 at 16:57, on Zulip):

the workspace includes things outside compiler/, including tools: https://github.com/rust-lang/rust/issues/76653#issuecomment-691574806

Daniel Mcnab (Mar 06 2021 at 17:04, on Zulip):

I mean that extern crate miri; then removing miri from Cargo.toml makes priroda no longer compile.
Therefore, the miri library crate is not included in rustc-dev, even though rust-analyzer thinks it is.
Indeed, rust-analyzer thinks the miri dependency priroda is using is the one from rustc_dev, instead of the one in Cargo.toml
And neither miri can actually use the rustc_dev packages in rust-analyzer, since they aren't workspace members.

Joshua Nelson (Mar 06 2021 at 17:05, on Zulip):

miri is separate component:

$ rustup component list | grep miri
miri-x86_64-unknown-linux-gnu
Joshua Nelson (Mar 06 2021 at 17:05, on Zulip):

I don't know how that interacts with rust-analyzer

Daniel Mcnab (Mar 06 2021 at 17:08, on Zulip):

The miri component doesn't ship libmiri.rlib either, only

file:bin/miri
file:bin/cargo-miri
file:share/doc/miri/LICENSE-APACHE
file:share/doc/miri/LICENSE-MIT
file:share/doc/miri/README.md

That is, it only ships binaries, so we use a direct dependency on https://github.com/rust-lang/miri

Daniel Mcnab (Mar 06 2021 at 17:09, on Zulip):

However rust-analyzer thinks that the miri library found in rustcSource/src/tools/miri is the version of miri we are using, even though we have a direct path dependency

Daniel Mcnab (Mar 07 2021 at 10:50, on Zulip):

I'm tempted to make it be required to use [package.metadata.rust-analyzer] rustc_private=true for using the rustc_private crates. So long as it's documented well, I feel like that would be a better state of affairs
Doing that would avoid analysing the rustc_private crates for every workspace if rustcSource is set

Daniel Mcnab (Mar 07 2021 at 13:29, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/7891 is ready for review.

matklad (Mar 08 2021 at 14:41, on Zulip):

Hmm, this almost sounds like we need to revamp sysroot handling to be closer to what rustc does

I'd much rather revamp rustc's sysroot handling to be closer what rust-analyzer does. implicit deps via search paths are not the most convenient model for an IDE )

Joshua Nelson (Mar 08 2021 at 14:42, on Zulip):

what does rust-analyzer do?

Jonas Schievink [he/him] (Mar 08 2021 at 14:44, on Zulip):

hmm, I'm not sure how we could change rustc without a big breaking change

Jonas Schievink [he/him] (Mar 08 2021 at 14:44, on Zulip):

Joshua Nelson said:

what does rust-analyzer do?

r-a assumes that all external crates used by some crate are known in advance (passed in as a CrateGraph)

Joshua Nelson (Mar 08 2021 at 14:46, on Zulip):

Jonas Schievink [he/him] said:

Joshua Nelson said:

what does rust-analyzer do?

r-a assumes that all external crates used by some crate are known in advance (passed in as a CrateGraph)

yeah I don't see how that could work without breaking changes

matklad (Mar 08 2021 at 14:46, on Zulip):

this needn't be breaking

matklad (Mar 08 2021 at 14:47, on Zulip):

We'll need that info for build std anyway, so it is coming long-term

Joshua Nelson (Mar 08 2021 at 14:48, on Zulip):

build-std will use different artifacts than the sysroot though, especially since people want to add feature flags to std

Joshua Nelson (Mar 08 2021 at 14:48, on Zulip):

I don't see how those models work together - build-std is optional

Jonas Schievink [he/him] (Mar 08 2021 at 14:50, on Zulip):

build-std does not help with rustc-internal crates I think

matklad (Mar 08 2021 at 14:53, on Zulip):

We are agnostic of what process is actually used to build the code, we don't use rlibs. We only need to know deps between the crates. If something implements build-std, something knows about dependencies between sysroot crates, and can expose that info to rust-analyzer.

I think it is super duper crucially important that we have a compilation model where everything is known in advance, so I'd rather not support rustc_private properly than to have search-path functionality impleted, which would make maintainint the "in advance" property hard to maintain.

Joshua Nelson (Mar 08 2021 at 14:57, on Zulip):

I think it is super duper crucially important that we have a compilation model where everything is known in advance, so I'd rather not support rustc_private properly than to have search-path functionality impleted, which would make maintainint the "in advance" property hard to maintain.

I just don't see how this is compatible with how rustc_private works though :/ that info is fundamentally not available ahead of time

Joshua Nelson (Mar 08 2021 at 14:57, on Zulip):

you could parse the crate-level attributes for feature(rustc_private) maybe?

matklad (Mar 08 2021 at 14:59, on Zulip):

It's indeed not compatible with how rustc_private. So there's a tradeoff between "sane compilation model" and "supporting rustc_private correnctly". I think we should pick sane compilation model here

Florian Diebold (Mar 08 2021 at 15:01, on Zulip):

would it be possible to handle these as some kind of 'weak' / 'lazy' dependencies? i.e. the possible set and where they come from is known beforehand, but not whether the dependency edge actually exists

Joshua Nelson (Mar 08 2021 at 15:03, on Zulip):

Can you talk about why you think this property is so important? "Rustdoc will never be fully supported" is a really bitter pill to swallow

Joshua Nelson (Mar 08 2021 at 15:05, on Zulip):

Other tools will be broken too, like clippy and miri and potentially rustfmt. And any third party tool that uses the libraries.

matklad (Mar 08 2021 at 15:08, on Zulip):

It doesn't follow that rustdoc will never be supported :D

Joshua Nelson (Mar 08 2021 at 15:09, on Zulip):

The standard compilation model doesn't allow using precompiled libraries

Joshua Nelson (Mar 08 2021 at 15:10, on Zulip):

I worked very hard on https://github.com/rust-lang/rust/pull/79540 and I'm not interested in undoing it

Joshua Nelson (Mar 08 2021 at 15:11, on Zulip):

Is there a guide for setting up rustcSources?

matklad (Mar 08 2021 at 15:27, on Zulip):

No, there are no docs for working with rustc private

matklad (Mar 08 2021 at 15:27, on Zulip):

the following works for me:

18:26:42|~/projects/rust/src/librustdoc|master✓
λ pwd
/home/matklad/projects/rust/src/librustdoc

18:26:44|~/projects/rust/src/librustdoc|master✓
λ bat -p .vscode/settings.json
{
    "rust-analyzer.rustcSource": "/home/matklad/projects/rust/Cargo.toml"
}

18:26:47|~/projects/rust/src/librustdoc|master✓
λ code .
matklad (Mar 08 2021 at 15:28, on Zulip):

(though for some reason it doesn't resolve methods from std)

matklad (Mar 08 2021 at 15:29, on Zulip):

works for me = I click on extern crate rustc_hir; and get to its definiton

matklad (Mar 08 2021 at 15:29, on Zulip):

This was implemented in https://github.com/rust-analyzer/rust-analyzer/pull/6524, that's the best source of documentatin at the momentz

Joshua Nelson (Mar 08 2021 at 15:32, on Zulip):

hmm, it doesn't seem to have done anything for me

Joshua Nelson (Mar 08 2021 at 15:32, on Zulip):

let me try running x.py check and see if it helps

Joshua Nelson (Mar 08 2021 at 15:34, on Zulip):

oh well oops apparently x.py check is broken with download-rustc lol, I think that should hopefully be fixed with https://github.com/rust-lang/rust/pull/82739

matklad (Mar 08 2021 at 15:34, on Zulip):

@Joshua Nelson we could do a quick screen share session right now, if that's helpful

Joshua Nelson (Mar 08 2021 at 15:34, on Zulip):

sure! give me a second

matklad (Mar 08 2021 at 15:34, on Zulip):

x.py check shouldn't be relevant I think

Joshua Nelson (Mar 08 2021 at 15:34, on Zulip):

Click to join video call

Joshua Nelson (Mar 08 2021 at 15:44, on Zulip):

ok cool we got rustcSources working and I'm going to write up docs for it this afternoon (either for rust-analyzer or rustc-dev-guide)

matklad (Mar 08 2021 at 15:44, on Zulip):

Ok, @Joshua Nelson now knows how to set this up, so you can ask them ^^

Joshua Nelson (Mar 08 2021 at 15:45, on Zulip):

matklad said:

It's indeed not compatible with how rustc_private. So there's a tradeoff between "sane compilation model" and "supporting rustc_private correnctly". I think we should pick sane compilation model here

so to my understanding this is mostly "rustc_private won't be supported automatically and you'll need to do some setup work" which seems fine

Joshua Nelson (Mar 08 2021 at 15:45, on Zulip):

as long as it's possible to get it to work

Daniel Mcnab (Mar 08 2021 at 16:27, on Zulip):

@Joshua Nelson Your final comment does kind of make sense, but the behaviour of running cargo check (for build scripts) was already in the code
I just added an opt-out

Joshua Nelson (Mar 08 2021 at 16:27, on Zulip):

Your final comment does kind of make sense, but the behaviour of running cargo check (for build scripts) was already in the code

well sure, but you can always change it :P

Joshua Nelson (Mar 08 2021 at 16:27, on Zulip):

it seems silly to require everyone who uses this feature to add the opt out themselves

Daniel Mcnab (Mar 08 2021 at 16:28, on Zulip):

Well for example, I wouldn't opt out
Since it's possible that it does improve code analysis somewhere in the rustc source.

Joshua Nelson (Mar 08 2021 at 16:29, on Zulip):

Daniel Mcnab said:

Well for example, I wouldn't opt out
Since it's possible that it does improve code analysis somewhere in the rustc source.

but this is never the case

Joshua Nelson (Mar 08 2021 at 16:29, on Zulip):

cargo check will never work out of the box

Daniel Mcnab (Mar 08 2021 at 16:29, on Zulip):

It certainly appears to do something. The point is that it only runs cargo check to run build scripts.

Joshua Nelson (Mar 08 2021 at 16:30, on Zulip):

It certainly appears to do something.

can you describe what you mean by "do something"?

Daniel Mcnab (Mar 08 2021 at 16:31, on Zulip):

Sure. It shows up in the bottom bar as metadata rustc_mir and friends
And it doesn't hurt, at least on my machine.
It must have been added for a reason.

Joshua Nelson (Mar 08 2021 at 16:32, on Zulip):

It must have been added for a reason.

image.png

Daniel Mcnab (Mar 08 2021 at 16:34, on Zulip):

@matklad if that is so, then would moving to rustcPrivate.src still make sense?
In this world we'd remove the other setting and the lines of code which get disabled by it

Daniel Mcnab (Mar 08 2021 at 16:34, on Zulip):

I'm still learning how to Zulip, sorry

matklad (Mar 08 2021 at 16:35, on Zulip):

if we don't need to touch settings, it's better not to touch them indeed

Daniel Mcnab (Mar 08 2021 at 16:43, on Zulip):

Right, I'm updated then

Daniel Mcnab (Mar 08 2021 at 16:48, on Zulip):

Would it make sense for [package.metadata.rust-analyzer]rustc_private=true to imply rust-analyzer.rustcSource="discover" if it is otherwise unset.

matklad (Mar 08 2021 at 16:49, on Zulip):

I don't think so -- this is "nightly" stuff, so I'd rather user opt-into settings.json somewhere

Joshua Nelson (Mar 08 2021 at 16:50, on Zulip):

matklad said:

I don't think so -- this is "nightly" stuff, so I'd rather user opt-into settings.json somewhere

isn't the package.metadata already an opt-in?

Daniel Mcnab (Mar 08 2021 at 16:51, on Zulip):

Sorry for the bors spam, I know I had permission before, wasn't aware it had been removed
(Not that it shouldn't have been removed, it being removed is definitely right)

Daniel Mcnab (Mar 08 2021 at 16:54, on Zulip):

It's a good thing this is landing today (because of the release). I can go around the respositories I know of and add this metadata so it doesn't break for people.

matklad (Mar 08 2021 at 16:57, on Zulip):

No worries about bors, we pruned the perms a while ago indeed. Would be happy to grant r+ back if I get tired of r+ PRs myself :)

Daniel Mcnab (Mar 08 2021 at 17:01, on Zulip):

I don't need it - I don't expect to be actively participating too much. It's the good thing of Rust, can just jump into almost any project and being sure that your contributions are at least 'correct', for some definition thereof

Joshua Nelson (Mar 08 2021 at 22:22, on Zulip):

Daniel Mcnab said:

Would it make sense for [package.metadata.rust-analyzer]rustc_private=true to imply rust-analyzer.rustcSource="discover" if it is otherwise unset.

I'm interested in adding this, but I'm not sure how - I got as far as

diff --git a/crates/project_model/src/workspace.rs b/crates/project_model/src/workspace.rs
index 1b53fcc30..47b332a32 100644
--- a/crates/project_model/src/workspace.rs
+++ b/crates/project_model/src/workspace.rs
@@ -443,6 +443,7 @@ fn cargo_to_crate_graph(
         // If the user provided a path to rustc sources, we add all the rustc_private crates
         // and create dependencies on them for the crates which opt-in to that
         if let Some(rustc_workspace) = rustc {
+            rustc_workspace.build_data_config.cargo_features.rustc_source = Some(crate::RustcSource::Discover);
             handle_rustc_crates(
                 rustc_workspace,
                 load,

and gave up when it said the fields are private:

error[E0616]: field `build_data_config` of struct `CargoWorkspace` is private
error[E0616]: field `cargo_features` of struct `BuildDataConfig` is private

Also I think this is wrong in the first place because it only runs if rustc_source was already found :/

Daniel Mcnab (Mar 09 2021 at 07:14, on Zulip):

What you'd want is to add an else branch to the if has_private {if let ... {} else {/*launch discovery*/ }}
I'm not certain what I called the variable

Daniel Mcnab (Mar 09 2021 at 07:51, on Zulip):

It also turns out that this code was very useful for clippy, since clippy doesn't actually use a workspace
A happy accident of improvement

RalfJ (Mar 13 2021 at 10:37, on Zulip):

So I guess there is some reason that rust-analyzer.rustcSource="discover" cant be the default always, irrespective of what the package settings say?

matklad (Mar 13 2021 at 10:42, on Zulip):

We don't want to load rustc crates for everyone who isn't working on the compiler and related tooling. This is essentially a nightly opt-in

RalfJ (Mar 13 2021 at 11:10, on Zulip):

Ah so this loads things eagerly, not just when needed? I understand, makes sense.

Daniel Mcnab (Mar 13 2021 at 12:25, on Zulip):

@RalfJ Basically, it would run cargo metadata on the rustc crates, but not analyse them if rustc_dev is installed and no crates in the current workspace (and dependency tree) use the metadata
Personally, I think that could be a reasonable tradeoff, since I suspect the number of people who should have rustc_dev installed on their normal nightly should be small

Daniel Mcnab (Mar 13 2021 at 12:25, on Zulip):

That would also get all crates from crates.io which rustc depends on

Last update: Jul 26 2021 at 12:15UTC