Disambiguate between SourceFiles from different crates even if they have the same path

[michaelwoerister]

This PR fixes an ICE that can occur when the compiler encounters a source file that is part of both the local crate and an upstream crate:

1. While importing source files from an upstream crate the compiler creates a SourceFile entry for foo.rs in the SourceMap. Since this is an imported source file its src field is None.
2. At a later point the parser encounters foo.rs again. It tells the SourceMap to load the file but because we already have an entry for foo.rs the SourceMap will return the existing version with src == None.
3. The parser proceeds under the assumption that src.is_some() and panics when actually trying to use the file's contents.

This PR fixes the issue by adding the source file's associated CrateNum to the SourceMap's interning key. As a consequence the two instances of the file will each have a separate entry in the SourceMap. They just happen to share the same file path. This approach seemed less problematic to me than trying to mutate the SourceFile after it had already been created.

Another, more involved, approach might be to merge the src and the external_src field.

Fixes #85955

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[davidtwco]

Looks good to me!

[davidtwco]

@bors r+

[bors]

📌 Commit 2942d18145b8f5e878da7f3adc33ff370f320624 has been approved by davidtwco

[michaelwoerister]

Thanks for the review, @davidtwco!

[wesleywiser]

Since this issue was introduced in #83813 which landed in 1.54, I'm nominating for backport to beta (1.54).

[gilescope]

I've built this branch and confirm this solves the ICE for me. Thank you so much @michaelwoerister !

[bors]

⌛ Testing commit 2942d18145b8f5e878da7f3adc33ff370f320624 with merge edadf7cd25a7bbac01eb7438bf7b913cab443d65...

[bors]

💔 Test failed - checks-actions

[davidtwco]

Looks like a legitimate failure.

[michaelwoerister]

I'll look into it.
@bors rollup=never

[michaelwoerister]

It looks like #85834 might be the culprit: CrateNum now gets encoded as a StableCrateId but the decoder in question still decodes it as a u32.

[michaelwoerister]

I fixed the issue by encoding the CrateNum part as a StableCrateId and then mapping that back to CrateNum on-demand. I also tried to remove the default encoding and decoding impls for CrateNum in order to make it more difficult to run into an encoder/decoder mismatch like this in the future but it turns out the default impls are still used somewhere. I opened #86540 regarding that.

Do you want to take another look, @davidtwco?

[davidtwco]

@bors r+

[bors]

📌 Commit c3c4ab5 has been approved by davidtwco

[bors]

⌛ Testing commit c3c4ab5 with merge 80926fc...

[bors]

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 80926fc to master...

[gilescope]

Thanks again @michaelwoerister for fixing this - much obliged!

[michaelwoerister]

You're welcome, @gilescope!

[apiraino]

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

[pnkfelix]

This seems to have caused a mild (approx. -1%) regression on ctfe-stress-4 for the incr-unchanged case.

My intuition is that it may just be an unavoidable cost of the change in representation that was introduced here.

(Should a perf run have been done on this PR before it landed? Not an easy call to make; only one benchmark significantly regressed, but from the code itself here, this was a key data structure being modified, so maybe a perf run is always warranted in such cases.)

[cuviper]

@michaelwoerister I found this non-trivial to backport to beta, particularly with errors in on_disk_cache.rs. Would you be able to prepare that backport yourself?

[michaelwoerister]

Sorry, @cuviper, I totally overlooked your message. A backport PR is up at #87393.