Don't canonicalize crate paths

[ChrisDenton]

When printing paths in diagnostic we should favour printing the paths that were passed in rather than resolving all symlinks.

This PR changes the form of the crate path but it should only really affect diagnostics as filesystem functions won't care which path is used. The uncanonicalized path was already used as a fallback for when canonicalization failed.

This is a partial alternative to #139823.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[lcnr]

r? compiler

[fee1-dead]

This PR changes the form of the crate path but it should only really affect diagnostics as filesystem functions won't care which path is used. The uncanonicalized path was already used as a fallback for when canonicalization failed.

Any test that can show us how the diagnostics get changed?

[ChrisDenton]

I can add a run-make test. I'm finding it hard to do in a UI test.

But before when running rustc main.rs -L ./ with multiple candidate crates in the current dir:

error[E0464]: multiple candidates for `rlib` dependency `resolver1` found
 --> main.rs:1:1
  |
1 | extern crate resolver1;
  | ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: candidate #1: \\?\F:\Temp\rust\crateresolver\libresolver1-1.rlib
  = note: candidate #2: \\?\F:\Temp\rust\crateresolver\libresolver1-2.rlib

error: aborting due to 1 previous error

After:

error[E0464]: multiple candidates for `rlib` dependency `resolver1` found
 --> main.rs:1:1
  |
1 | extern crate resolver1;
  | ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: candidate #1: ./libresolver1-1.rlib
  = note: candidate #2: ./libresolver1-2.rlib

error: aborting due to 1 previous error

I think the latter is nicer. Another alternative would be to make paths absolute without resolving symlinks but I think it worth preserving relative paths if that's what the user gave us. If nothing else it's a bit more reproducible between machines.

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

[fee1-dead]

Not too familiar with the specific part of the code base to review beyond that, I think..

r? compiler

[jieyouxu]

r? jieyouxu

[jieyouxu]

Note for myself: see if with this PR we can remove the extra normalization from Waffle's PR and not have tests explode (89b4eba)

[WaffleLapkin]

Looks reasonable to me, r=me with the comments added.

[WaffleLapkin]

minor: Can you add comments here? I envision something like this:

                        {
                            // Use canonical path to check if we already processed it.
                            // Going through different symlinks does not change the result.
                            let path =
                                try_canonicalize(&spf.path).unwrap_or_else(|_| spf.path.to_path_buf());
                            if seen_paths.contains(&path) {
                                continue;
                            };
                            seen_paths.insert(path);
                        }
                        // Use the original path (potentially with unresolved symlinks),
                        // filesystem code should not care, but this is nicer for diagnostics.
                        let path = spf.path.to_path_buf();

[WaffleLapkin]

question: Does this also make the error not reproducible if case there are 2 symlinks? (i.e. you could go from either of them first, the second is ignored)

[ChrisDenton]

It's as deterministic as our Vec<SearchPath> of library paths, so it should always pick the first library path passed in that resolves to the same directory. Changing the library order passed in would change the diagnostic. Though I don't think that's too different from other diagnostics that depend on the order of things passed in?

Aside: I am slightly uncertain if we need to use canonicalise at all. If two rlibs, for example, are byte-for-byte equal then we should treat them as not conflicting, whether that's due to being a symlink, hard link, copy or a deterministic rebuild. So skipping directories is just a potential performance optimisation. Though I don't know if it has that much impact in practice or if using same-file (thus skipping potentially more expensive checks) would recover most performance loss, if any. I've not looked into it so didn't want to touch that here.

[WaffleLapkin]

Makes sense

[ChrisDenton]

@bors r=WaffleLapkin

[bors]

📌 Commit 52f35d0 has been approved by WaffleLapkin

It is now in the queue for this repository.