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

Change src/etc/vscode_settings.json to always treat ./library as the sysroot source #108188

Merged
merged 1 commit into from
Feb 19, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 17, 2023

See
https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/False.20error.20report.20for.20.60rust-analyzer.28private-field.29.60 for further discussion; previously this had various bugs.

I tested go-to-definition on:

  • use std::io::Write in src/bootstrap/setup.rs
  • use std::cell::RefCell in src/librustdoc/core.rs
  • use rustc_span::symbol::sym in src/librustdoc/core.rs
  • use std::fmt in compiler/rustc_span/src/symbol.rs
  • Global in library/alloc/src/alloc/tests.rs

The following things still don't work:

  • Global.deallocate in alloc/tests.rs. This function is under cfg(not(test)), so it can't be enabled without disabling RA in tests.rs altogether. I think this might be fixable by moving library/alloc/src/alloc/tests.rs to library/alloc/tests/alloc/lib.rs, so it's in a different crate, but I'd like to avoid blocking this improvement on that change.

cc @thomcc @BoxyUwU @spastorino - you've had issues with RA in the past, does this fix them? Are there any other use cases I should test? You can try these changes out by running cp src/etc/vscode_settings.json .vscode/settings.json, or running x setup and picking a random profile (it won't overwrite config.toml if it already exists). See #108135 for plans to make updating the config easier.

r? @Veykril

…sysroot source

See
https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/False.20error.20report.20for.20.60rust-analyzer.28private-field.29.60
for further discussion; previously this had various bugs.

I tested go-to-definition on:
- `use std::io::Write` in `src/bootstrap/setup.rs`
- `use std::cell::RefCell` in `src/librustdoc/core.rs`
- `use rustc_span::symbol::sym` in `src/librustdoc/core.rs`
- `use std::fmt` in `compiler/rustc_span/src/symbol.rs`
- `Global` in `library/alloc/src/alloc/tests.rs`

The following things still don't work:
- `Global.deallocate` in alloc/tests.rs. This function is under
  `cfg(not(test))`, so it can't be enabled without disabling RA in
  `tests.rs` altogether. I think this might be fixable by moving
  `library/alloc/src/alloc/tests.rs` to
  `library/alloc/tests/alloc/lib.rs`, so it's in a different crate, but
  I'd like to avoid blocking this improvement on that change.
@rustbot
Copy link
Collaborator

rustbot commented Feb 17, 2023

Failed to set assignee to Veykril: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 17, 2023
@jyn514
Copy link
Member Author

jyn514 commented Feb 17, 2023

r? bootstrap - I feel pretty confident this is better than the status quo, but happy to make further improvements.

@jyn514
Copy link
Member Author

jyn514 commented Feb 17, 2023

cc @ozkanonur

@onur-ozkan
Copy link
Member

I can't properly test this since I am not using Vscode.

Are you using vscode? @albertlarsan68

@albertlarsan68
Copy link
Member

Yes I do.
r? @albertlarsan68

@rustbot rustbot assigned albertlarsan68 and unassigned onur-ozkan Feb 18, 2023
@jyn514
Copy link
Member Author

jyn514 commented Feb 18, 2023

@ozkanonur what editor are you using? There's instructions for how to use this with neovim on https://rustc-dev-guide.rust-lang.org/building/suggested.html#neovim

@onur-ozkan
Copy link
Member

I am using neovim, but without the neoconf plugin that is mentioned in dev-guide. When I saw vscode in the PR title I thought PR is more related with vscode users. But it's actually all about lsp configuration, which means I can also test this. I will test the configuration and share the results here probably tomorrow morning.

@jyn514
Copy link
Member Author

jyn514 commented Feb 18, 2023

Ah, gotcha. Maybe we should rename that file to rust-analzyer-settings.json so it's more clear.

@onur-ozkan
Copy link
Member

Sure, I think it's better naming for that file

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

All the notes you mentioned works like charm(I also tested go to reference) with this change. Plus, before this change, when I open compiler/rustc_attr/src/session_diagnostics.rs I had the following error from nvim lsp:

LSP[rust_analyzer] rust-analyzer failed to load workspace: Failed to find sysroot for Cargo.toml file /home/nimda/devspace/personal/rust/src/bootstrap/Cargo.toml.: can't load standard library from sysroot /ho
me/nimda/devspace/personal/rust/./build/host/stage0-sysroot
rust-analyzer failed to load workspace: Failed to find sysroot for Cargo.toml file /home/nimda/devspace/personal/rust/Cargo.toml.: can't load standard library from sysroot /home/nimda/devspace/personal/rust/.
/build/host/stage0-sysroot

And also Diagnostic was showing error proc macro `Diagnostic` not expanded: crate has not (yet) been built

This change fixes it. The error no longer exists, and Diagnostic doesn't show error. I can use signature help to get information about it, and also use go to definition on it.

@albertlarsan68
Copy link
Member

Do you want to rename the file in this PR, or in another?
r=me if the rename is made as a follow-up.

@jyn514
Copy link
Member Author

jyn514 commented Feb 19, 2023

Let's do a follow-up, it ended up being slightly more complicated than I expected and I want to land this change since it works well :)

@bors r=albertlarsan68

@bors
Copy link
Contributor

bors commented Feb 19, 2023

📌 Commit 6209e6c has been approved by albertlarsan68

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2023
@jyn514
Copy link
Member Author

jyn514 commented Feb 19, 2023

@bors rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2023
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#107766 (Fix json reexports of different items with same name)
 - rust-lang#108129 (Correctly handle links starting with whitespace)
 - rust-lang#108188 (Change src/etc/vscode_settings.json to always treat ./library as the sysroot source)
 - rust-lang#108203 (Fix RPITITs in default trait methods (by assuming projection predicates in param-env))
 - rust-lang#108212 (Download rustfmt regardless of rustc being set in config.toml)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 243dcd0 into rust-lang:master Feb 19, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 19, 2023
@jyn514 jyn514 deleted the ra-sysroot branch February 20, 2023 22:46
@onur-ozkan
Copy link
Member

Do you want to rename the file in this PR, or in another? r=me if the rename is made as a follow-up.

Be aware that when the filename changes, some of the editors won't be able to detect this file and they will require some additional configurations.

@albertlarsan68
Copy link
Member

The rename is about the checked in file (src/etc/vscode_settings.json to src/etc/rust_analyzer_settings.json or something like that), not the VS Code settings file.
The checked in file is not detected by any editor AFAIK.

@onur-ozkan
Copy link
Member

Ah right, I confused it with .vscode/settings.json, my bad.

@albertlarsan68
Copy link
Member

This rename means that the file may be used with others editors than VS Code.

@jyn514
Copy link
Member Author

jyn514 commented Feb 24, 2023

The file can already be used with other editors. This is just about making the name less confusing in review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants