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

Opt-in to rustc_private for rust-analyzer #6869

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

DJMcNab
Copy link
Contributor

@DJMcNab DJMcNab commented Mar 8, 2021

rust-lang/rust-analyzer#7891

changelog: none

This will also help priroda and any other package which depends on the miri library crate.
This isn't Miri :p

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @phansch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 8, 2021
@matthiaskrgr
Copy link
Member

What does this do?
Is it related to #5803 ?
Does it change the way rust-analyzer handles the way clippy depends on rustc crates?
How does it interact with the subrepo nested inside the rustc repo vs the standalone repo that we have here?

@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 8, 2021

This tells rust-analyzer to use the rustc_private crates located at rust-analyzer.rustcSource for development of the crates within clippy (This setting can be "discover" to automatically detect it based on your active toolchain, which works because rustc-dev ships the source now). This is now opt-in and allows rust-analyzer to resolve the rustc crates for non-workspace member crates - previously it was implicitely active for every crate in the current workspace.

This should mean that so long as every user has:

{ "rust-analyzer.rustcSource": "discover" }

set, rust-analyzer should just work in this repository.

That being said, if you have rustcSource set and are working within the rust repository itself, then you might still have problems. I'm not sure how developing within rust currently works using rust-analyzer so some clarification on that would be helpful. However, in any case this change does not change behaviour (i.e. this change is not a regression, since the dependency on the rustc_private crates would still have been added automatically, this just makes it implcit).

That is, previous to this change I believe rust-analyzer would not correctly work on the version of clippy within the rust repo, and it still won't.

This should obsolete #5803 (and/or the other upstream changes already have done that, and this PR only brings it to your attention).

@phansch
Copy link
Member

phansch commented Mar 9, 2021

This is great 🎉

Unfortunately I have some trouble to get this to work locally. Even though it indexes the rustc crates, it still marks the rustc crates with unresolved extern crates and I can't jump to definition, etc.

Here's what I currently have configured in my local Clippy checkout:

$ cat .vscode/settings.json
{
    "rust-analyzer.rustcSource": "/home/phansch/code/rust/Cargo.toml"
}
  • My VSCode plugin version is 0.2.513
  • rust-analyzer --version is d54e115
  • The rust checkout is up-to-date as well
  • rustc-nightly version: 2021-03-08
  • It also didn't work when setting rustcSource to discover; The rustc crates weren't being indexed at all in this case

@phansch
Copy link
Member

phansch commented Mar 9, 2021

Nevermind, it worked after setting rust-analyzer.updates.channel to nightly:

Screenshot_2021-03-09_07-41-15

@phansch
Copy link
Member

phansch commented Mar 9, 2021

Let's get this in for now 🎉 We can add some more documentation once a new non-nightly version of rust-analyzer is released.

@matthiaskrgr I believe this also means the ra-setup machinery is no longer needed. At least I didn't run that locally and it still resolved the crates correctly.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 9, 2021

📌 Commit c4e2cf9 has been approved by phansch

@bors
Copy link
Contributor

bors commented Mar 9, 2021

⌛ Testing commit c4e2cf9 with merge 727e5f1...

@bors
Copy link
Contributor

bors commented Mar 9, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 727e5f1 to master...

@bors bors merged commit 727e5f1 into rust-lang:master Mar 9, 2021
@DJMcNab DJMcNab deleted the rust-analyzer-private branch March 9, 2021 07:09
@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 9, 2021

Oh that's cool

So possibly the old heuristic was not sufficient for clippy, because the sub crates are not actually in the same workspace and instead are used as path dependencies

I didn't even consider that as a possibility, but I'm glad it's worked out like that for you <3

@flip1995
Copy link
Member

flip1995 commented Mar 9, 2021

Thanks a lot for this! This also works great in (neo)vim with coc.nvim 🎉

(I didn't have to set the update channel in vim, but I guess my setup is a bit different: I use the rustup version of rust-analyzer-preview with a symlink from ~/.cargo/bin to .rustup/toolchain/nightly/bin/rust-analyzer and have

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

in my config.)

My complete rust-analyzer config
{
    "rust-analyzer.server.path": "rust-analyzer",
    "rust-analyzer.checkOnSave.extraArgs": ["--target-dir", "target/rust-analyzer"],
    "rust-analyzer.cargo.allFeatures": true,
    "rust-analyzer.diagnostics.disabled": ["macro-error", "unresolved-proc-macro"],
    "rust-analyzer.rustcSource": "discover",
}

The disabled diagnostics made problems when hacking on Clippy. The first appeared on our declare_clippy_lint! macro (maybe this is resolved now with rustcSource: discover). The second showed me warnings for Serialize and Deserialize derives in every crate. This was annoying when jumping between diagnostics.

@matthiaskrgr
Copy link
Member

Yeah, this looks like it obsoletes cargo dev ra-setup, rip. 😄

@DJMcNab
Copy link
Contributor Author

DJMcNab commented Mar 9, 2021

My apologies. I can send a reverting PR so you can keep using that if you want 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants