Relink don't rebuild: add a baseline, sound implementation that can be incrementally improved#155871
Relink don't rebuild: add a baseline, sound implementation that can be incrementally improved#155871susitsm wants to merge 19 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
rustbot has assigned @jdonszelmann. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
0e63248 to
4b676bd
Compare
|
This is a large change, might take me multiple review passes. I'll see if I can do the first today or tomorrow :) |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
1682f38 to
ab0ab17
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot review |
|
Just realized that there is already a soundness hole: the |
| // should be added here as | ||
| // ``` | ||
| // // FIXME do we need this // or a comment about why we need this | ||
| // let _ = my_new_field; |
There was a problem hiding this comment.
this part is outdated now, about the let _ =
| // `hash_crate_root_public_api`" into `encode_my_new_field` | ||
| // 3. Only remove/change what is hashed in a separate PR. Removing items just from the hash | ||
| // should be done with extreme scrutiny. A better way might be to sort the query result | ||
| // in its provider, or filter which values we encode. That also helps with rmeta size. |
There was a problem hiding this comment.
this should maybe be a fixme at the end
| } | ||
| } | ||
|
|
||
| // When changed, make sure to update the hashing in `hash_crate_root_public_api` |
There was a problem hiding this comment.
ideally, we of course made it so the hashing of that and the encoding here share some of the same source. Maybe by implementing a trait, or calling a method here that adds to the RDR hash, or returning a closure, idk. That way you don't have to think about all these comments and nonlocal subtleties
There was a problem hiding this comment.
that, or even a rustc lint at some point that automatically prompts you if you've made a change here and didn't update the other file. not 100% sure yet what the logic there would be, but this, though better, still seems brittle
There was a problem hiding this comment.
Reworked this part in a way that the compiler can help a bit more with not forgetting it and doing it correctly. Introduced a new HashableCrateRoot struct that derives StableHash. The public api hash is just the stable hash of this struct. When hashing is done, CrateRoot is constructed from the calculated public and private hashes and the values moved from HashableCrateRoot.
|
@rustbot author |
This comment has been minimized.
This comment has been minimized.
ab0ab17 to
981dee4
Compare
981dee4 to
f07198b
Compare
|
These commits modify compiler targets. |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The last commit also fixes the issue I described with the resolver in #155871 (comment). It looks up dependencies based on their public api hash, but the resolver considers both public and private hashes when looking for duplicate candidates. It will return an error if there are two crates with |
This comment has been minimized.
This comment has been minimized.
f07198b to
cd53915
Compare
… rmeta without parents)
…end on public_api_hash instead of crate_hash
…tributes for testing
…stc_public_hash_unchanged attributes
…lic hashes in the resolver When the resolver resolved transitive dependecies, it used the `crate_hash` of dependecies saved inside the rmeta of downstream crates to locate them. With public api hashing enabled, it is not sound to save that hash into downstream crates. Only the public hash can be saved. This modifies the locator to find all crates with the given public hash, but look for conflicing crates using the (public, private) hash pair. So if there are multiple crates with the same public hash, but different private hash (which should only happen if there was some kind of hash collision while making the public hashes or the StableCrateId which is included in it), it will be reported as having multiple candidates for the crate.
cd53915 to
b9829db
Compare
b9829db to
ff5f988
Compare
This comment has been minimized.
This comment has been minimized.
ff5f988 to
d292f0b
Compare
|
☔ The latest upstream changes (presumably #156361) made this pull request unmergeable. Please resolve the merge conflicts. |
View all comments
This PR implemenets a sound but not too useful implementation of relink don't rebuild with the unstable
-Z public-api-hashflag. It currently uses the stable hash of all items in the metadata.The goal is to give a base implementation that can be used for experimentation. It can be incrementally improved over time by removing or stable sorting items. The PR also adds new test attributes
rustc_public_hash_changedandrustc_public_hash_unchangedand an example test using them.What are non-goals for this PR: a useful, optimized implementation of RDR and public api hashing. That has many more challenges which will each require careful review.
A non exhaustive list of the challenges for the feature I ran into while trying to make the hash useful (this should probably go in a tracking issue, but I'm not aware of one)
my_crate::funcprintsprivate functionas the errorcargo check), could use a much simpler hash than codegen, it does not need private types from mir (or any mir at all?)