Repository navigation
Make untracked incr. comp. information inaccessible #90317
Description
When iterating over a collection, be it a Vec, a HashMap or an IndexMap, the order of items influences the value of the resulting hash: [a, b] and [b, a] have different hashes.
Meanwhile, there is some information we do not want to track. This is the case of the value of LocalDefId. Inserting a function in a file will change other functions LocalDefId, but it will not change their DefPathHash.
My concern is about controlling this information flow. In order to do that ToStableHashKey trait replaces the iteration order of the collection (which for hash maps is based on the key and the allocated memory capacity and should be irrelevant to the compilation), by the order of a stable hash key (the DefPathHash when the key is LocalDefId). By sorting the vectors by stable key, we manage the information flow.
Using IndexMap, the iteration order is the insertion order. Normally, this insertion order should only depend on tracked information obtained by depending on another query. For instance, a HIR visitor will create a query dependency on hir_owner_nodes, which hashes the in-code declaration order of functions. However, and this is my concern, the order of LocalDefId is freely available without using a query and is purposely untracked.
In order to make IndexMaps safe for incr. comp., we need to ensure untracked information is inaccessible.
Known issues:
- Stop implementing
PartialOrdandOrdonDefId; - Stop implementing
PartialOrd,OrdandIdxforLocalDefId; - Stop implementing
PartialOrd,OrdandIdxforLocalExpnId; - Stop implementing
PartialOrdandOrdforSyntaxContext; - Enforce that UNTRACKED options are not accessed within a query.
Originally posted by @cjgillot in #90253 (comment)
Activity
pierwill commentedon Oct 29, 2021
Can these items be done one at a time?
cjgillot commentedon Oct 29, 2021
Of course!
pierwill commentedon Oct 29, 2021
Will give this a go and see what happens. @rustbot claim
pierwill commentedon Oct 30, 2021
I started experimenting with
LocalDefId. I wonder if theDefIdwork might end up being slightly less complex. 🤔pierwill commentedon Nov 2, 2021
Update: No. 😄
pierwill commentedon Nov 3, 2021
Looks like removing
Ord,PartialOrdforExpnIdandLocalExpnIdmight require changes to the macrorustc_index::newtype_indexbecause of this line:rust/compiler/rustc_index/src/vec.rs
Line 107 in 473eaa4
bjorn3 commentedon Nov 5, 2021
Most of them are already "tracked" by discarding the entire incr comp cache if they change. The ones that aren't tracked are explicitly not tracked. See the
UNTRACKEDmentions in https://github.com/rust-lang/rust/blob/d22dd65835190278f315e06442614142653ec98f/compiler/rustc_session/src/options.rs A couple of them should be marked asTRACKEDthough I think.pierwill commentedon Nov 8, 2021
pierwill commentedon Nov 11, 2021
@cjgillot It looks like
rustc_span::hygiene::ExpnIddoes not implementOrdorPartialOrd.rustc_span::hygiene::LocalExpnIddoes implement ordering.(So does
rustc_middle::thir::ExprId. Is that related to this issue?)66 remaining items
Rollup merge of rust-lang#122820 - oli-obk:no_ord_def_id, r=estebank
Unrolled build for rust-lang#122820
Auto merge of rust-lang#122832 - oli-obk:no_ord_def_id3, r=michaelwoe…
Auto merge of rust-lang#123165 - oli-obk:no_ord_def_id3, r=cjgillot
Auto merge of #123165 - oli-obk:no_ord_def_id3, r=cjgillot
Auto merge of #123165 - oli-obk:no_ord_def_id3, r=cjgillot
Auto merge of #123165 - oli-obk:no_ord_def_id3, r=cjgillot
Rollup merge of rust-lang#138610 - oli-obk:no-sort-hir-ids, r=compile…
Rollup merge of rust-lang#138610 - oli-obk:no-sort-hir-ids, r=compile…
Rollup merge of rust-lang#138610 - oli-obk:no-sort-hir-ids, r=compile…
Unrolled build for rust-lang#138610
Rollup merge of #138610 - oli-obk:no-sort-hir-ids, r=compiler-errors
Auto merge of #90842 - pierwill:localdefid-indexmap, r=wesleywiser
Auto merge of #90842 - pierwill:localdefid-indexmap, r=wesleywiser
Auto merge of #123165 - oli-obk:no_ord_def_id3, r=cjgillot