search

rust-lang / rust-analyzer

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

overrideCommand with a relative path (`./miri`/`./x.py`) does not work very well, and that is very hard to debug #10793

Closed RalfJung closed 1 month ago

RalfJung commented 2 years ago

I am using the following workspace settings for working on Miri with vscode:

// Place your settings in this file to overwrite default and user settings.
{
    "rust-analyzer.rustfmt.extraArgs": [
        "+nightly"
    ],
    "rust-analyzer.checkOnSave.overrideCommand": [
        "./miri",
        "check",
        "--message-format=json",
    ],
    "rust-analyzer.linkedProjects": [
        "./Cargo.toml",
        "./cargo-miri/Cargo.toml"
    ],
    //"rust-analyzer.rustcSource": "discover"
}

Whenever I save a file, an error pops up saying

cargo check failed: No such file or directory (os error 2)

That's all it says. I have no idea which file it is talking about, or what is even going wrong -- the cargo check seems to be working fine, I am getting the errors I am expected highlighted as they should. The only sign that anything going wrong is this error message, and the error is not giving any details that would be helpful in determining what is happening.

Veykril commented 2 years ago

This looks like a cargo problem to me(as in it doesn't emit the bad file's path), as we show the entire error message we receive from cargo here https://github.com/rust-analyzer/rust-analyzer/blob/cfa26c3ac3d85d502f75c2f6b760aadcc997a1a8/crates/rust-analyzer/src/main_loop.rs#L396-L401

bjorn3 commented 2 years ago

The stderr of cargo is read but never presented to the user: https://github.com/rust-analyzer/rust-analyzer/blob/d1e756e05aab5410f6176fce26bf021453708b9b/crates/flycheck/src/lib.rs#L292

RalfJung commented 2 years ago

Also, running ./miri check --message-format=json does not show any such error. So if this error is coming from cargo, the question is what RA is doing that it invokes cargo in a way that causes that error.

This is also a regression, the same configuration used to work fine a few months ago.

RalfJung commented 2 years ago

This excerpt from the log might be interesting though it does not tell me much

[INFO rust_analyzer::main_loop] handle_event(FetchWorkspace(Report("metadata")))
[INFO rust_analyzer::reload] did fetch workspaces [Ok(Cargo { root: Some("miri"), n_packages: 57, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 }), Ok(Cargo { root: Some("cargo-miri"), n_packages: 53, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 })]
[INFO rust_analyzer::main_loop] handle_event(FetchWorkspace(End([Ok(Cargo { root: Some("miri"), n_packages: 57, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 }), Ok(Cargo { root: Some("cargo-miri"), n_packages: 53, sysroot: true, n_rustc_compiler_crates: 0, n_rustc_cfg: 67, n_cfg_overrides: 1 })])))
[INFO rust_analyzer::reload] will switch workspaces
[INFO rust_analyzer::main_loop] handle_event(PrimeCaches(Begin))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(12)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(13)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(14)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Request(Request { id: RequestId(I32(9)), method: "textDocument/semanticTokens/full", params: Object({"textDocument": Object({"uri": String("file:///home/r/src/rust/miri/tests/run-pass/portable-simd.rs")})}) }))
[INFO flycheck] restart flycheck "./miri" "check" "--message-format=json"
[INFO flycheck] restart flycheck "./miri" "check" "--message-format=json"
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 1, progress: DidStart })
[ERROR flycheck] Flycheck failed to run the following command: "./miri" "check" "--message-format=json"
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 1, progress: DidFinish(Err(Os { code: 2, kind: NotFound, message: "No such file or directory" })) })
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(15)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Response(Response { id: RequestId(I32(16)), result: None, error: None }))
[INFO rust_analyzer::main_loop] handle_event(Progress { id: 0, progress: DidFinish(Ok(())) })
[INFO rust_analyzer::main_loop] handle_event(Request(Request { id: RequestId(I32(10)), method: "textDocument/codeLens", params: Object({"textDocument": Object({"uri": String("file:///home/r/src/rust/miri/tests/run-pass/portable-simd.rs")})}) }))
[INFO rust_analyzer::main_loop] handle_event(Response { id: RequestId(I32(10)), error: None })
[INFO rust_analyzer::global_state] handled textDocument/codeLens - (10) in 104.40µs

Could it be the case that it starts ./miri ... twice, one of them works and gives me the in-editor warnings I expect, and the other fails with the error it shows?

bjorn3 commented 2 years ago

It sees two workspaces: One for miri (at .) and one for cargo-miri (at ./cargo-miri) and then tries to run ./miri check --message-format=json within the directory of both workspaces. The first succeeds, but the second fails as it needs to be ../miri in that case.

RalfJung commented 2 years ago

That is new behavior though I think? We had two workspaces for a while. I don't remember exactly why but I do remember that making them one workspace did not work the way I wanted it to.

./miri check --message-format=json actually builds both of them.

RalfJung commented 2 years ago

I confirmed @bjorn3's theory by creating a dummy script cargo-miri/miri that does nothing -- then the error goes away.

Unfortunately, vscode still does not show errors or warnings for the cargo-miri crate -- it used to be enough to add it to linkedProjects to achieve this, but it seems that things changed over the last few weeks/months. So instead I made cargo-miri/miri a symlink to miri, and changed the script to adjust its working dir correctly even when being called through a symlink -- now things seem to work again.

But it looks like overrideCommand and linkedProjects currently do not behave well when used together.

RalfJung commented 2 years ago

Current status: pretty much unchanged.

Part of the problem here is that I expected the overrideCommand to be a single global setting, not something that RA uses separately for each project. https://github.com/rust-lang/rust-analyzer/pull/13065 should help a bit with that, at least.

For Miri, we have a pretty decent work-around with https://github.com/rust-lang/miri/pull/2495. But for x.py we don't, and it'd be great to be able to get RA to work inside bootstrap there.

RalfJung commented 2 years ago

But for x.py we don't, and it'd be great to be able to get RA to work inside bootstrap there.

So here's a possible plan:

I think together this should mean that the existing recommended override command for rustc will actually work as intended even when adding src/bootstrap/Cargo.toml to linkedProjects, and native diagnostics (as well as all the RA magic) should then work in bootstrap. It does mean that RA calls ./x.py multiple times (once for each linked project), but after the first run completes all the others should be instantaneous so that's fine, I think. (To improve this we'd need x.py to actually honor the --manifest-path argument.)

Cc @jyn514 for x.py/bootstrap involvement.

Veykril commented 2 years ago

So I've been brain storming a bit. First up, this also affects build-scripts (and proc-macro building as well therefore), not just checkOnSave. I believe there are 3 variants of what people might want from this:

Does this seem right?

RalfJung commented 2 years ago

Yeah sounds good -- though if the per-workspace command is needed, then also having a global command does not seem that useful. I was thinking of global commands mostly because I was sure the command would be global (an implicit assumption I did not even realize I was making), and the documentation did not indicate anything else, so I was taken by surprise when I realized that it is per-workspace.

But doing this per-workspace makes sense, it'd just be good to find a way to do that that does not involve changing the working directory, while also working around https://github.com/rust-lang/cargo/issues/11007.

Veykril commented 2 years ago

The working dir switching was done for cargo I imagine since that just works, but it might break for custom commands. I think offering both version, switching working dir or passing the project path offers the most flexibility for users in regards to override commands, so I don't think offering both hurts. The main problem is really implementing the first point, running it once for the entire project in regards to diagnostic paths as you've said with rust-lang/cargo#11007

Veykril commented 2 years ago

Also https://github.com/rust-lang/rust-analyzer/issues/8937 is probably related to this

RalfJung commented 2 years ago

Yeah, I don't think there even is a reliable way to take the file paths that come out of ./miri check or ./x.py check and figure out where they point -- different parts of the build use paths relative to different base directories.

But I think a per-workspace command that stays in the project root is 'good enough'. The command might ignore the --manifest-path and do some unnecessary work, but assuming reasonable caching that should usually stabilize quickly.

jyn514 commented 2 years ago
  • Change x.py to simply ignore --manifest-path, so that using ./x.py check ... still works as an override command even when there are multiple linked projects.

Why not do the same workaround you had for miri, have a symlink in src/bootstrap/x.py that links to x.py? That seems less hacky.

It does mean that RA calls ./x.py multiple times (once for each linked project), but after the first run completes all the others should be instantaneous so that's fine, I think. (To improve this we'd need x.py to actually honor the --manifest-path argument.)

I don't understand why that would help; the least work we can do after a full x.py check is "none", which is ~basically what we do today. Are you imagining x.py check --manifest-path src/bootstrap/Cargo.toml to behave the same as x.py check src/bootstrap? Why would that be faster?

RalfJung commented 2 years ago

Why not do the same workaround you had for miri, have a symlink in src/bootstrap/x.py that links to x.py? That seems less hacky.

Dunno, that symlink seems like a pretty big hack to me.

Also that won't work for x.py. x.py is pwd-dependent, it will put the build artifacts into a build folder relative to where it is invoked. I tried to change this many years ago but the PR got rejected -- this is by design.

Are you imagining x.py check --manifest-path src/bootstrap/Cargo.toml to behave the same as x.py check src/bootstrap? Why would that be faster?

Yes, and similar for the cranelift workspace -- "only check that workspace".

It would do less work, wouldn't it? No need to recheck all of rustc if we just changed bootstrap.

(Though what I'd really want is that when I work on library, it doesn't rebuild all of compiler on each change... but there's not really a good way for RA to know this I think.)

jyn514 commented 2 years ago

(Though what I'd really want is that when I work on library, it doesn't rebuild all of compiler on each change... but there's not really a good way for RA to know this I think.)

You can tell RA that by changing the command to x check library bootstrap (instead of an unconditional check).

It would do less work, wouldn't it? No need to recheck all of rustc if we just changed bootstrap.

x.py won't rebuild rustc if you've only modified bootstrap. So an unconditionally x.py check isn't doing more work after the very first run.

Also that won't work for x.py. x.py is pwd-dependent, it will put the build artifacts into a build folder relative to where it is invoked. I tried to change this many years ago but the PR got rejected -- this is by design.

ah, well, fair enough. I guess ignoring --manifest-path is fine - I see the reason it's necessary for RA is for workspaces that aren't using a custom cargo wrapper, like you said above.

RalfJung commented 2 years ago

You can tell RA that by changing the command to x check library bootstrap (instead of an unconditional check).

I know. But I also often work on the compiler and then I of course want it to check the compiler. Right now I just change the configuration each time I change from working on the compiler to working on the library, But ideally, the file I hit Ctrl-S in would determine which things it checks... just dreaming here. ;)

x.py won't rebuild rustc if you've only modified bootstrap. So an unconditionally x.py check isn't doing more work after the very first run.

Yeah, that first run can take minutes though.

But I agree it's probably not a big deal in practice. Changing things in the compiler will also rebuild cranelift, but that's probably a good thing -- RA seems to assume that changes to one workspace cannot cause errors in other workspaces (so it is enough to run the check-build for the workspace that changed), which is just a wrong assumption in general.

Veykril commented 2 years ago

I know. But I also often work on the compiler and then I of course want it to check the compiler. Right now I just change the configuration each time I change from working on the compiler to working on the library, But ideally, the file I hit Ctrl-S in would determine which things it checks... just dreaming here. ;)

Maybe we can have the server interpolate into the override command, so that you could just pass --manifest-path {manifestPath} or {packageName} etc. Fixing this on the r-a side shouldn't be too much trouble (apart from only running one command for the entire project, but that isn't a hard requirement here right now), so this is more of a design issue right now. Though proper interpolation is probably way more than is really needed, unless the --manifest-path option isn't an option for rustc?

RA seems to assume that changes to one workspace cannot cause errors in other workspaces (so it is enough to run the check-build for the workspace that changed), which is just a wrong assumption in general.

This should not be the case, basically if you save a file, we fetch all crates that make use of this file, then collect all transwitive reverse dependency crates of those crates and finally we kick off the checkOnSave logic for all workspaces that contain at least on of those crates. Only if the file does not belong to some workspace we unconditionally trigger checkOnSave for all workspaces. At least that's how this is supposed to work.

RalfJung commented 2 years ago

Yeah, interpolation makes sense, though there also needs to be some way to control the working directory behavior (or else existing cargo clippy overrides will break on multi-workspace projects). Then we don't even need x.py to ignore an extra argument, we can just decide to not pass the argument.

That won't help with me having to edit the vscode config when doing library vs compiler work, as those are in the same workspace. But that's just a separate issue, sorry for bringing it up here.

This should not be the case, basically if you save a file, we fetch all crates that make use of this file, then collect all transwitive reverse dependency crates of those crates and finally we kick off the checkOnSave logic for all workspaces that contain at least on of those crates. Only if the file does not belong to some workspace we unconditionally trigger checkOnSave for all workspaces. At least that's how this is supposed to work.

Oh nice, that is a lot more clever than I expected. So we'll have to do some tests once the working directory part lands on the RA side.

RalfJung commented 1 year ago

The current status her as far as I know:

RalfJung commented 1 month ago

With recent developments (https://github.com/rust-lang/miri/pull/3798, https://github.com/rust-lang/rust/pull/128904), I think the options RA provides are entirely sufficient, and documentation has also been improved to explain the working directory situation. So this can be closed; what's left is possibly simplifying the implementation (https://github.com/rust-lang/rust-analyzer/issues/17848).