checksum-freshness: Fix invalid checksum calculation for binary files

[osiewicz]

Admittedly this is not the cleanest way to achieve this, but SourceMap is quite intertwined with source files being represented as Strings.
Tracking issue: rust-lang/cargo#14136
Closes: #151090

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
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

[jieyouxu]

@rustbot reroll

[mati865]

Looks good to me, but I don't feel confident enough approving it.

@rustbot reroll

CCing @bjorn3 who I think should be able to give useful feedback here.

[bjorn3]

Maybe make this IndexMap<String, Option<(u64, Option<SourceFileHash>)>> where None indicates that the length and hash haven't been filled in yet?

[osiewicz]

Does that get us anything? This PR makes it so that files with binary contents are inserted into both source_map.files() (with null hash unnormalized_source_len equal to 0) and file_depinfo; with the use of IndexMap, we can override those coming from source_map.files() with hashes coming from file_depinfo. Thus, the value in that IndexMap would never be None.

Now is that the cleanest way? I'm sure not.. but I honestly don't understand implications of removing these "null" binary entries from the sourcemap well enough to make that cut.

[jdonszelmann]

I think it makes some amount of sense to keep them like this, as null entries? Especially if the has can never really be None as Piotr says. @bjorn3 does Piotr's comment convince you? Or d'ya have a good reason why making it an Option makes sense

[bjorn3]

You are first inserting into files with a dummy value of hash 0 + len 0 and later overwriting it with the real value, right? My suggestion is to first insert None and later overwrite it with the real value. This way you can unwrap the entry at use and be guaranteed that you didn't accidentally get a dummy value.

[osiewicz]

I see your point. What's tricky about it is that we have no way of knowing whether the items in files are "bugged" or not.
That is to say, in the context of that function, we have no way to tell if an entry with hash 0 + len 0 originated in a include_bytes!("my_1024mb_file.bin"); call (bugged one, that this PR tries to address) or include_str!("empty_file.txt") (legit one). Both of them have a hash 0 + len 0, and it is the correct value for the include_str one.

[bjorn3]

You can change line 639 to return None instead of (0, None), right? And adapt the return type of hash_iter_files accordingly. Or am I missing something?

[osiewicz]

I believe that branch (on line 639) is not relevant at all and that's not where we were getting our fake-nulls ((hash 0, len 0)) from.
Before this PR, the file path of a binary file (with non-UTF8 contents) was injected into depfile through source_map().files(). For non-utf8 files the values were not correct, since SourceMap only accepts Strings as the contents of a source file. See the following snippet, as that's where we "fake out" contents of a binary file when inserting it into a SourceMap:

rust/compiler/rustc_span/src/source_map.rs

Line 279 in 2850ca8

let text = std::str::from_utf8(&bytes).unwrap_or("").to_string();

Crucially, we did not/do not call hash_iter_files on these, as we assume that SourceMap calculated the length and a hash correctly.. Which it did not, in some cases (non-utf8 binary files). This then led to binary files having a hash of empty string and a length of 0.

With this PR, that's still the case - binary files are still present in source_map().files() result, but they're also injected into file_depinfo and that lets us call hash_iter_files on them and get the correct hash/file length. The change to files type let's us override the "faulty" value with a good one. However, as outlined in https://github.com/rust-lang/rust/pull/151137/files#r2726164490 , there's no straightforward way (IMHO) to use an option here, because if we knew which entries from the SourceMap were "faulty", we could just call hash_iter_files on them directly without jumping through the hoops.. And line 639 is not really at fault for what we're struggling with.

[bjorn3]

I see. In that case I'm fine with leaving the code as is.

[fee1-dead]

@rustbot reroll

[jdonszelmann]

This looks good to me, if @bjorn3 's concern is lifted, r=me

View changes since this review

[jdonszelmann]

I think it makes some amount of sense to keep them like this, as null entries? Especially if the has can never really be None as Piotr says. @bjorn3 does Piotr's comment convince you? Or d'ya have a good reason why making it an Option makes sense

[osiewicz]

I have addressed the outstanding review comments and am happy with the code as is; unless anybody objects, I think this PR is good to go (from my side).

View changes since this review

[jdonszelmann]

@bors r+

[rust-bors]

📌 Commit 0df94dd has been approved by jdonszelmann

It is now in the queue for this repository.

[rust-bors]

💔 Test for 14ee0d1 failed: CI. Failed job:

• auto - dist-i686-mingw (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors r-
Failed ci

[rust-bors]

Commit 0df94dd has been unapproved.

[JonathanBrouwer]

@bors r=jdonszelmann
Was spurious, github's problem

[rust-bors]

📌 Commit 0df94dd has been approved by jdonszelmann

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

##[error]Could not find a part of the path 'D:\a'.