Rust-analyzer + rustc repo: build failures in the standard library are not shown in the editor any more

[RalfJung]

This is a consequence of https://github.com/rust-lang/rust/pull/128534 plus https://github.com/rust-lang/cargo/issues/9887: on a build failure in the sysroot, x.py check library now prints

error[E0425]: cannot find function `handle_errors` in this scope
   --> alloc/src/raw_vec.rs:177:25
    |
177 |             Err(err) => handle_errors(err),
    |                         ^^^^^^^^^^^^^ help: a function with a similar name exists: `handle_error`
...
607 | fn handle_error(e: TryReserveError) -> ! {
    | ---------------------------------------- similarly named function `handle_error` defined here

Note that the file name no longer includes the leading library/. As a consequence, RA cannot locate the error, so it is not shown in the IDE at all.

This will make working on the standard library a lot more painful, unfortunately. Cc @bjorn3 @epage @Veykril

[weihanglo]

Hmm... so Rust Analyser assumes a diagnostic item with a leading library/ path is from the standard library?

[Veykril]

No, r-a runs x.py in the root to fetch diagnostics, yet not all diagnostics reported by x.py have their paths be relative to the root of the workspace due to https://github.com/rust-lang/cargo/issues/9887. So r-a is unable to associate diagnostics with files that are not part of the main ./Cargo.toml workspace in the rust repo (as only those are actually relative to the working directory of where x.py is invoked in)

[RalfJung]

@weihanglo This is a problem that affects all projects where one does not invoke cargo directly, and instead a wrapper script coordinates the build across a bunch of workspaces -- the directories in the output are relative to the workspace root where the problem occurred, but the workspace root is somewhere inside the repo.

It's not just about some assumption RA makes, it is also confusing for users. E.g. this is output I get from x.py check regularly:

error[E0425]: cannot find value `bx` in this scope
   --> src/intrinsics/simd.rs:196:51
    |
196 |                         idx_ty.simd_size_and_type(bx.cx.tcx).1.kind(),
    |                                                   ^^

Can you tell which file the error occurs in? It's in the cranelift backend, but given just the error message, that's impossible to tell. Cargo got invoked as cargo --manifest-path=..., so it knows that the workspace root is not equal to the working directory, but unfortunately it nevertheless invokes rustc in a way that paths are printed relative to the workspace rather than the working directory.

So far this only affected the cranelift and GCC backends, so only a few contributors were affected. Now it affects the entire standard library, making it a much bigger issue for rustc contributors.

[Veykril]

Likewise it also makes the paths semi useless in integrated terminals for editors where usually clicking it opens the file (as long as the path is relative to the opened workspace root)

[pnkfelix]

it nevertheless invokes rustc in a way that paths are printed relative to the workspace rather than the working directory

hmm. should rustc consider adding a (potentially unstable) option to change that? I.e. an option that will make it render the emitted paths as relative to the current working directory?

(Or potentially, emit the paths as relative to a directory supplied to that command line option, so that even if a build tool like cargo changes the current working directory itself for some reason, we could still thread through the original directory and then have rustc compute paths as relative to that)

My memory from other contexts is that this can be a hard problem to solve optimally, but I would imagine there are some at least half baked solutions that could serve us well here...

[bjorn3]

I.e. an option that will make it render the emitted paths as relative to the current working directory?

That is exactly what rustc does already. Cargo however sets the current working directory of the rustc invocations it spawns to the root of the workspace (or the package dir in case of crates.io deps) to fix https://github.com/rust-lang/cargo/pull/4788. In https://github.com/rust-lang/cargo/issues/9887#issuecomment-2271216002 I suggested having an option to make cargo set a parent directory of the workspace as current dir for the rustc invocation.

[pnkfelix]

Okay it sounds like I was basically suggesting that we work on addressing https://github.com/rust-lang/cargo/issues/9887 . (In particular, when I included the generalization of an option that tells rustc "do your reporting relative to this directory I'm giving to you." For example, that generalization yields effectively absolute paths when one asks for directories relative to the root directory.)

You (bjorn3) made an interesting point over there about that potentially interfering with diagnostics caching; it doesn't sound insurmountable, but it definitely complicates the most naive approach I was envisioning...

[pnkfelix]

heh, apparently I was complaining about this quite a long time ago: https://github.com/rust-lang/rust/issues/47355

[RalfJung]

Yeah this is arguably a regression introduced by https://github.com/rust-lang/cargo/pull/4788, but it essentially only affects projects that have "multi workspace repositories" with a custom build script (like rustc or Miri). It's been known for years but there is no easy fix that does not regress the "move build cache to different folder" case and it's not been anyone's priority to come up with a non-easy fix :(

[estebank]

I just discussed this with Felix, and I'm still catching up on the conversation in this thread, but there's an option that might be minimally disruptive to the two different use-cases mentioned: We could extend the json output to include both relative and absolute paths in different fields (or keep the current relative path field and add a field holding the absolute path to where the relative path starts from and let tools concatenate them themselves). (We also need to make a determination, I believe around the distinction between absolute paths and canonical paths, to account for symlinks.)

[RalfJung]

I just discussed this with Felix, and I'm still catching up on the conversation in this thread, but there's an option that might be minimally disruptive to the two different use-cases mentioned: We could extend the json output to include both relative and absolute paths in different fields (or keep the current relative path field and add a field holding the absolute path to where the relative path starts from and let tools concatenate them themselves). (We also need to make a determination, I believe around the distinction between absolute paths and canonical paths, to account for symlinks.)

Given that the issue affects tools and users reading rustc output (as I keep repeating), I don't see how this would help. x.py would still produce pretty useless errors like this

error[E0425]: cannot find value `bx` in this scope
   --> src/intrinsics/simd.rs:196:51

when there is an error in the cranelift backend.

[RalfJung]

https://github.com/rust-lang/cargo/pull/14752 plus https://github.com/rust-lang/rust/pull/132390 are fixing this issue. :)

[Veykril]

Neat, that doesn't even require any changes on our side I take it? (given this is internal to bootstrap's invocations)

[RalfJung]

Nope, it's all working on my machine with the existing RA. :) Even build errors in bootstrap itself are shown properly.