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

Improve handling of rustc_private #7891

Merged
merged 12 commits into from
Mar 8, 2021

Conversation

DJMcNab
Copy link
Contributor

@DJMcNab DJMcNab commented Mar 6, 2021

This PR changes how rust-analyzer handles rustc_private. In particular, packages now must opt-in to using rustc_private in Cargo.toml, by adding:

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

This means that depending on crates which also use rustc_private will be significantly improved, since their dependencies on the rustc_private crates will be resolved properly.

A similar approach could be used in #6714 to allow annotating that your package uses the test crate, although I have not yet handled that in this PR.

Additionally, we now only index the crates which are transitive dependencies of rustc_driver in the rustcSource directory. This should not cause any change in behaviour when using rustcSource: "discover", as the source used then will only be a partial clone. However, if rustcSource pointing at a local checkout of rustc, this should significantly improve the memory usage and lower indexing time. This is because we avoids indexing all crates in src/tools/, which includes rust-analyzer itself.

Furthermore, we also prefer named dependencies over dependencies from rustcSource. This ensures that feature resolution for crates which are depended on by both rustc and your crate uses the correct set for analysing your crate.

See also introductory zulip stream

I have tested this in priroda, and it provides a significant improvement to the development experience (once I give miri the required data in Cargo.toml)

Todo:

  • Documentation

This is ready to review, and I will add documentation if this would be accepted (or if I get time to do so anyway)

DJMcNab added 2 commits March 6, 2021 12:17
This is a hack to work around miri being included in
our analysis of rustc-dev
Really, we should probably use an include set of the actual root libraries

I'm not sure how those are determined however
@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 6, 2021

I have just discovered that (it appears that) any crate can use extern crate regex_syntax; (for example) if rustc-dev is installed, and just use it directly, without depending on it through cargo. This seems like a non-ideal status quo.

Never mind, we have the rustc_private feature enabled. See rust-lang/rust#50373

DJMcNab added 4 commits March 7, 2021 10:18
This gives the advantage that

A future extension would be to check for `feature(rustc_private)` instead
Also add some more detailed comments
Extract into function deleted the previous comments
@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 7, 2021

Packages which probably currently use rustc_private and rust-analyzer:

It's worse than I thought...
@DJMcNab DJMcNab changed the title Implement opt-in (and opt-out) rustc_private Improve handling of rustc_private Mar 7, 2021
@bjorn3
Copy link
Member

bjorn3 commented Mar 7, 2021

In rustc_codegen_cranelift I don't use rust-analyzer.rustcDev as it uses too much memory. Instead I just disabled the errors for unresolved extern crate.

@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 7, 2021

This PR should also significantly reduce memory usage from using rustcSource @bjorn3.

(This is because we now only index the crates brought in by rustc-dev, instead of all crates in rustcSource).

@bjorn3
Copy link
Member

bjorn3 commented Mar 7, 2021

I just tried it out. The memory usage is indeed lower. However it tried to run cargo check on the rustc source.

@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 8, 2021

@bjorn3 I've added a setting which could help - rust-analyzer.cargo.disableRustcBuildScripts.

Comment on lines 52 to 53
/// Disable running build scripts (`build.rs`) for the `rustc_private` crates in `rustcSource`.
cargo_disableRustcBuildScripts: bool = "false",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this and rustcSource into rustc_private section, so that we have

"rustcPrivate.src": ...,
"rustcPrivate.runBuildScripts": ...,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would cargo.rustcPrivate.runBuildScripts and cargo.rustcPrivate.src make sense? Since we only have special behaviour for the rustcSource for cargo, afaik.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rathe keep it top level. The fact that it is rustcPrivate is more important than that it is cargo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but no strong opinion here!

@matklad
Copy link
Member

matklad commented Mar 8, 2021

This looks good to me!

I am a bit hesitant about putting rust-analyzer string in Cargo.toml, I think that generally would be a mistake. However, this is rustc_private stuff, so it doesn't really matter.

Hm, now that I've spelled this out, I wonder if we can do somthing worse :D Can we just read lib.rs and see if it contains rustc_private string?

@matklad
Copy link
Member

matklad commented Mar 8, 2021

Can we just read lib.rs and see if it contains rustc_private string?

I think we can just bypass VFS here and use just std::fs::read_to_string. That would be bad as a "proper" way to do this, but for rustc private it's better than extending the inteface to allow loading peek into sources.

EDIT: seems to fancy, and without a clear place to report an error, lets keep this as a key in metadata.

@jyn514
Copy link
Member

jyn514 commented Mar 8, 2021

Hm, now that I've spelled this out, I wonder if we can do somthing worse :D Can we just read lib.rs and see if it contains rustc_private string?

This will break if only one crate in the workspace uses rustc_private (unless you mean to do this for each crate?)

@jyn514
Copy link
Member

jyn514 commented Mar 8, 2021

@bjorn3 I've added a setting which could help - rust-analyzer.cargo.disableRustcBuildScripts.

A setting seems kind of weird, cargo check will never work on rustc sources. Could you turn it on unconditionally when package.metadata.rust-analyzer is enabled instead?

@matklad
Copy link
Member

matklad commented Mar 8, 2021

bors r+

bors bot added a commit that referenced this pull request Mar 8, 2021
7891: Improve handling of rustc_private r=matklad a=DJMcNab

This PR changes how `rust-analyzer` handles `rustc_private`. In particular, packages now must opt-in to using `rustc_private` in `Cargo.toml`, by adding:
```toml
[package.metadata.rust-analyzer]
rustc_private=true
```

This means that depending on crates which also use `rustc_private` will be significantly improved, since their dependencies on the `rustc_private` crates will be resolved properly.

A similar approach could be used in #6714 to allow annotating that your package uses the `test` crate, although I have not yet handled that in this PR.

Additionally, we now only index the crates which are transitive dependencies of `rustc_driver` in the `rustcSource` directory. This should not cause any change in behaviour when using `rustcSource: "discover"`, as the source used then will only be a partial clone. However, if `rustcSource` pointing at a local checkout of rustc, this should significantly improve the memory usage and lower indexing time. This is because we avoids indexing all crates in `src/tools/`, which includes `rust-analyzer` itself.

Furthermore, we also prefer named dependencies over dependencies from `rustcSource`. This ensures that feature resolution for crates which are depended on by both `rustc` and your crate uses the correct set for analysing your crate.

See also [introductory zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Fixed.20crate.20graphs.20and.20optional.20builtin.20crates/near/229086673)

I have tested this in [priroda](https://github.com/oli-obk/priroda/), and it provides a significant improvement to the development experience (once I give `miri` the required data in `Cargo.toml`)

Todo:
- [ ] Documentation

This is ready to review, and I will add documentation if this would be accepted (or if I get time to do so anyway)

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Mar 8, 2021

Canceled.

@DJMcNab

This comment has been minimized.

@DJMcNab

This comment has been minimized.

@bors
Copy link
Contributor

bors bot commented Mar 8, 2021

🔒 Permission denied

Existing reviewers: click here to make DJMcNab a reviewer

@matklad
Copy link
Member

matklad commented Mar 8, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 8, 2021

@bors bors bot merged commit d57c9f7 into rust-lang:master Mar 8, 2021
@DJMcNab DJMcNab deleted the rustc_private_metadata branch March 8, 2021 17:00
DJMcNab added a commit to DJMcNab/rust-gpu that referenced this pull request Mar 8, 2021
DJMcNab added a commit to DJMcNab/miri that referenced this pull request Mar 8, 2021
DJMcNab added a commit to DJMcNab/rust-clippy that referenced this pull request Mar 8, 2021
DJMcNab added a commit to DJMcNab/rust-semverver that referenced this pull request Mar 8, 2021
dario23 added a commit to dario23/prusti-dev that referenced this pull request Mar 8, 2021
This is suggested to make the dependency on rustc internals explicit in
the crate manifest; see
rust-lang/rust-analyzer#7891 for some more
details.

This change adds this hint to all crates that use
`#![feature(rustc_private)]`.
XAMPPRocky pushed a commit to EmbarkStudios/rust-gpu that referenced this pull request Mar 9, 2021
bors added a commit to rust-lang/rust-clippy that referenced this pull request Mar 9, 2021
Opt-in to rustc_private for `rust-analyzer`

rust-lang/rust-analyzer#7891

changelog: none

This will also help priroda and any other package which depends on the `miri` library crate.
vakaras pushed a commit to viperproject/prusti-dev that referenced this pull request Mar 9, 2021
This is suggested to make the dependency on rustc internals explicit in
the crate manifest; see
rust-lang/rust-analyzer#7891 for some more
details.

This change adds this hint to all crates that use
`#![feature(rustc_private)]`.
DJMcNab added a commit to DJMcNab/priroda that referenced this pull request Mar 9, 2021
bjorn3 pushed a commit to oli-obk/priroda that referenced this pull request Mar 9, 2021
bors added a commit to rust-lang/miri that referenced this pull request Mar 9, 2021
bors added a commit to rust-lang/rust-semverver that referenced this pull request Mar 10, 2021
bors added a commit to rust-lang/rust-semverver that referenced this pull request Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants