Rust: incr.comp.: Crate Metadata not corresponding to a DefId is untracked.

[michaelwoerister]

Mentoring update: Mentoring instructions can be found here

---

There are many things in crate metadata that are not covered by dependency tracking (basically everything below this line). Those are things that are unlikely to change in an upstream crate, which is why this has not caused problems yet, but we'll not want to rely on that.

The best way to solve this is probably to create separate "input" DepNodes for these.

cc @nikomatsakis

[nikomatsakis]

Yes, I want to refactor how this stuff is handled. Ultimately, I think that metadata should just be a bunch of (key, value) pairs where the key is some query, and there should be no more "crate store" trait (except for the "connect providers" method).

However, it occurs to me that once we get the red-green system, we could just have a single "metadata" node corresponding to the entire crate, and then let the red/green caching remember what the values of specific metadata queries were.

[nikomatsakis]

After some discussion on IRC, @michaelwoerister and I were thinking that the best immediate way to make progress here is going to involve converting all CrateStore methods into queries. The main sticking point will be resolve, which we will leave 'as is' for now.

To that end, here is a checklist of CrateStore methods. Let's see if we can convert them all to queries. I've checked some off that probably don't make sense at this juncture (or ever).

Remaining

• [x] fn crates(&self) -> Vec<CrateNum>;

Resolve and elsewhere in rustc?

Rename these methods to {name}_untracked in CrateStore, add queries to TyCtxt, convert the compiler to use the query and change resolve to using {name}_untracked

• [x] fn dep_kind(&self, cnum: CrateNum) -> DepKind; - #44142
• [x] fn visibility(&self, def: DefId) -> ty::Visibility; - #44142
• [x] fn crate_name(&self, cnum: CrateNum) -> Symbol; - #44142
• [x] fn item_children(&self, did: DefId) -> Vec<def::Export>; - #44142
• [x] fn extern_mod_stmt_cnum(&self, emod_id: ast::NodeId) -> Option<CrateNum>; - #44142

Transitioned

• [x] fn describe_def(&self, def: DefId) -> Option<Def>; -- #41534
• [x] fn def_span(&self, sess: &Session, def: DefId) -> Span; -- #41593
• [x] fn stability(&self, def: DefId) -> Option<attr::Stability>; -- #41653
• [x] fn deprecation(&self, def: DefId) -> Option<attr::Deprecation>; -- #41653
• [x] fn item_attrs(&self, def_id: DefId) -> Vec<ast::Attribute>; -- #41724
• [x] fn fn_arg_names(&self, did: DefId) -> Vec<ast::Name>; -- #41724
• [x] fn impl_polarity(&self, def: DefId) -> hir::ImplPolarity; -- #41469
• [x] fn impl_parent(&self, impl_def_id: DefId) -> Option<DefId>; -- #41724
• [x] fn trait_of_item(&self, def_id: DefId) -> Option<DefId>; -- #41724
• [x] fn is_const_fn(&self, did: DefId) -> bool; -- #42598
• [x] fn is_default_impl(&self, impl_did: DefId) -> bool; -- #41917
• [x] fn is_foreign_item(&self, did: DefId) -> bool; -- #41724
• [x] fn is_staged_api(&self, cnum: CrateNum) -> bool; -- #41847
• [x] fn is_allocator(&self, cnum: CrateNum) -> bool; -- #42598
• [x] fn is_panic_runtime(&self, cnum: CrateNum) -> bool; -- #42598
• [x] fn extern_crate(&self, cnum: CrateNum) -> Option<ExternCrate>; -- #42598
• [x] fn retrace_path(&self, cnum: CrateNum, path_data: &[DisambiguatedDefPathData]) -> Option<DefId>; -- #43361
• [x] fn item_body_nested_bodies(&self, def: DefId) -> BTreeMap<hir::BodyId, hir::Body>; -- #41611
• [x] fn const_is_rvalue_promotable_to_static(&self, def: DefId) -> bool; -- #41611
• [x] fn is_item_mir_available(&self, def: DefId) -> bool; -- #41611
• [x] fn metadata_filename(&self) -> &str; - #41565
• [x] fn metadata_section_name(&self, target: &Target) -> &str; - #41565
• [x] fn is_exported_symbol(&self, def_id: DefId) -> bool; - #41724
• [x] fn dylib_dependency_formats(&self, cnum: CrateNum) -> Vec<(CrateNum, LinkagePreference)>; - #42598
• [x] fn original_crate_name(&self, cnum: CrateNum) -> Symbol; - #44142
• [x] fn crate_hash(&self, cnum: CrateNum) -> Svh; - #44142
• [x] fn crate_disambiguator(&self, cnum: CrateNum) -> Symbol; - #44142
• [x] fn plugin_registrar_fn(&self, cnum: CrateNum) -> Option<DefId>; - #44142
• [x] fn derive_registrar_fn(&self, cnum: CrateNum) -> Option<DefId>; - #44142
• [x] fn native_libraries(&self, cnum: CrateNum) -> Vec<NativeLibrary>; - #44142
• [x] fn exported_symbols(&self, cnum: CrateNum) -> Vec<DefId>; - #44142
• [x] fn impl_defaultness(&self, def: DefId) -> hir::Defaultness; - #44142
• [x] fn is_compiler_builtins(&self, cnum: CrateNum) -> bool; - #44142
• [x] fn is_sanitizer_runtime(&self, cnum: CrateNum) -> bool; - #44142
• [x] fn panic_strategy(&self, cnum: CrateNum) -> PanicStrategy; - #44142
• [x] fn is_no_builtins(&self, cnum: CrateNum) -> bool; - #44142
• [x] fn implementations_of_trait(&self, filter: Option<DefId>) -> Vec<DefId>; -#44142
• [x] fn is_dllimport_foreign_item(&self, def: DefId) -> bool; - #44142
• [x] fn is_statically_included_foreign_item(&self, def_id: DefId) -> bool; - #44142
• [x] fn used_libraries(&self) -> Vec<NativeLibrary>; - #44142
• [x] fn used_link_args(&self) -> Vec<String>; - #44142
• [x] fn item_body<'a, 'tcx>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> &'tcx hir::Body; - #44142
• [x] fn visible_parent_map<'a>(&'a self) -> ::std::cell::Ref<'a, DefIdMap<DefId>>; - #44142
• [x] fn used_crates(&self, prefer: LinkagePreference) -> Vec<(CrateNum, LibSource)>; - #44142
• [x] fn used_crate_source(&self, cnum: CrateNum) -> CrateSource; - #44142

??

• [x] fn encode_metadata<'a, 'tcx>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, link_meta: &LinkMeta, reachable: &NodeSet) -> EncodedMetadata;
• [x] fn metadata_encoding_version(&self) -> &[u8];
• [x] fn def_key(&self, def: DefId) -> DefKey;
  
  
  
  • def-id is just a canonicalized def-key and def-path, so this is an immutable property or else def-id would be different
  
  
• [x] fn def_path(&self, def: DefId) -> hir_map::DefPath;
  
  
  
  • see above
  
  
• [x] fn def_path_hash(&self, def: DefId) -> u64;
  
  
  
  • see above
  
  
• [x] fn def_path_table(&self, cnum: CrateNum) -> Rc<DefPathTable>;
  
  
  
  • see above
  
  
• [x] fn struct_field_names(&self, def: DefId) -> Vec<ast::Name>; -- used in resolve
• [x] fn load_macro(&self, did: DefId, sess: &Session) -> LoadedMacro; -- used in resolve
• [x] fn export_macros(&self, cnum: CrateNum); -- used in resolve
• [x] fn associated_item_cloned(&self, def: DefId) -> ty::AssociatedItem; -- used in resolve
• [x] fn item_generics_cloned(&self, def: DefId) -> ty::Generics;
• [x] fn lang_items(&self, cnum: CrateNum) -> Vec<(DefIndex, usize)>; - deferred to #44190
• [x] fn missing_lang_items(&self, cnum: CrateNum) -> Vec<lang_items::LangItem>; - deferred to #44190

[nikomatsakis]

cc @cramertj @aochagavia -- you interested in tackling some of these? In some cases, queries actually exist already, and we just have to convert code to use them exclusively (e.g., is_foreign_item)

[nikomatsakis]

Mentoring instructions: The current step in resolving this is to move methods from the CrateStore trait and convert them into queries. The CrateStore trait serves as the "abstract interface" to the metadata -- it is defined in librustc, but implemented in librustc_metadata (exactly once, though there exists a second, dummy impl).

The idea then for a PR is to:

• pick a method in the list
• find uses of it:
  
  
  
  • if it is used in resolve, you can skip it for now and move on to another
  
  
• otherwise:
  
  
  
  • add a query to the src/librustc/ty/maps.rs file for this method
  
  
  • convert the method into a crate metadata provider
  
  
  • convert all calls to the method to use the query (typically by doing ty::queries::QUERY::get(tcx, DUMMY_SP, def_id)
  
  

[hackeryarn]

I would love to get started on this. I will try to have an implementation for one of the items up in a couple days to align on direction.

[cramertj]

Yeah, I can take a look at some of these tonight or tomorrow.

[hackeryarn]

@cramertj I was planning on taking a look at the couple of the first ones today or tomorrow morning if you want to start a little further down the list.

Wouldn't want us stepping on each other's toes 😄

[cramertj]

@achernyak Sounds great! Thanks.

[nikomatsakis]

@cramertj @achernyak great! =)

[michaelwoerister]

However, it occurs to me that once we get the red-green system, we could just have a single "metadata" node corresponding to the entire crate, and then let the red/green caching remember what the values of specific metadata queries were.

One downside of this approach, that came up during discussion, is that this would mean that we would have to load and deserialize each value just to find out if it has changed or not. Currently we directly load fingerprints to do this check.

[arielb1]

One downside of this approach, that came up during discussion, is that this would mean that we would have to load and deserialize each value just to find out if it has changed or not. Currently we directly load fingerprints to do this check.

Can't we save fingerprints in the parent crate metadata and copy the expected fingerprints into the child crate IC-info?

[nikomatsakis]

@arielb1

Can't we save fingerprints in the parent crate metadata and copy the expected fingerprints into the child crate IC-info?

Yes, we could presumably special-case this code somewhat, though I think that -- for this to really work well -- we would want to align the metadata and queries so that they have a 1:1 relationship (refactoring the CrateStore methods away gets us closer to this vision). That said, I've been wondering if we will ever want more stability in our metadata format, in which case tying it directly to the current set of queries may or may not be wise. Probably not worth worrying about right now.

[cramertj]

Many of these methods don't take DefId as an argument, but the provide macro at the top of src/librustc_metadata/cstore_impl.rs assumes that they do-- should I change the macro to allow specifying the input type, or is there another way you think I should provide those methods?

[nikomatsakis]

@cramertj good point. I'd prefer to change the macro, but I'm not sure how easy that would be to do. I guess you can leverage the Key trait (not public, but it could be -- and in fact on one of my upcoming branches I made it public) in the maps crate to extract the krate number from any kind of legal key.

[hackeryarn]

@cramertj in the short term, I think most of these can be handled but using a tuple like const_eval_dep_node does, or by pulling the needed information off the TyCtxt itself. For example, for def_span the Session is available on TyCtxt and that's actually how it was passed in &tcx.sess.

Also CrateNumber, which is the other major one, is supported by the query already i.e. typecheck_item_body takes a CrateNumber already.

[hackeryarn]

I am making this comment as a way to track the conversions that proved problematic.

Used in resolve:

• fn visibility(&self, def: DefId) -> ty::Visibility;
• fn associated_item_cloned(&self, def: DefId) -> ty::AssociatedItem;

Unreachable tcx in calls. Call sights should be converted on-demand computations:

• fn item_generics_cloned(&self, def: DefId) -> ty::Generics;

[hackeryarn]

@nikomatsakis I was just looking at visible_parent_map and has a fairly substantial implementation inside the cstore::CStore. Moving that into a provider would require modifying the CrateStore trait to expose new methods. Is this something we want to take on as part of this PR?

[nikomatsakis]

@achernyak

I was just looking at visible_parent_map and has a fairly substantial implementation inside the cstore::CStore. Moving that into a provider would require modifying the CrateStore trait to expose new methods. Is this something we want to take on as part of this PR?

wowsers, that is complex. And it even returns a Ref. Yeah, I don't really think we want to put that into a query just "as is". I wonder if we can break it down somehow.

[cramertj]

@nikomatsakis If you get a chance, can you update the checklist above? I'm not entirely sure which sections have been finished since we've both been working on it and there are a couple of PRs open rn. If you'd like, I'd be happy to post the list of all the ones I think we've done so we can double-check.

[cramertj]

It would also be useful to mark off or otherwise indicate which ones were skipped because they're used in librustc_resolve. I'll put together a list.

[cramertj]

Used in librustc_resolve:
-visibility
-associated_item_cloned
-dep_kind
-export_macros
-crate_name
-struct_field_names
-item_children
-load_macro
-extern_mod_stmt_cnum

[nikomatsakis]

@cramertj will do! I'll do a sweep with what I can find, and you can compare.

[nikomatsakis]

Updated -- I tagged with all the PRs.

[cramertj]

@nikomatsakis Great, thanks! It looks like almost all the rest are ones I put off because they don't have a single DefId argument. I'll plan to have a PR out tonight updating the provide macro to allow for other arguments.

[hackeryarn]

@cramertj from looking into this a bit. It looks like the windows error in your other pr was thrown by the implementation for is_dllimport_foreign_item. Have you tried removing that and seeing if everything else works?

I think the last of this issue is blocked by #41780

[nikomatsakis]

@achernyak

I think the last of this issue is blocked by #41780

that's a closed PR? Is that what you meant to link to?

[hackeryarn]

@nikomatsakis, that PR was closed due to a Windows build issue in #41766 which was taking too long to resolved. I believe @cramertj closed both of these to not keep the PRs open for an extended period of time.

[cramertj]

Finally getting back to this-- I'm opening a PR with a few new ones. I also looked at lang_items and missing_lang_items. I discovered those are currently calculated before tcx is made and then stored in a field. They're accessed all over the place, so that'll require a large-scale refactor. I'm not sure how often the list of lang items changes, but if we want incremental results to be valid across lang-item-list changes, the lang-item list will need to be broken up similar to what @nikomatsakis did for the region maps (https://github.com/rust-lang/rust/pull/41662).

[michaelwoerister]

@cramertj Anything that is wrapped in a Tracked (like lang_items) you don't need to worry about. Those things can only be accessed when specifying a DepNode (or at least one has to explicitly work around tracking if it's really necessary).

[cramertj]

@michaelwoerister Oh, good to know. We should check those ones off on the list, then. I'll put together a list of them when I get a chance later today.

[michaelwoerister]

@cramertj Excellent, thank you!

[cramertj]

Out of the list above, I see these included as Tracked:

• lang_items
• missing_lang_items
• native_libraries
• implementations_of_trait above is backed by the get_implementations_for_trait method on cdata, which appears to just be a filter on top of CrateRoot.impls, which is Tracked<LazySeq<TraitImpls>>. I'm not sure whether or not to count this-- I guess it depends on whether or not we want to directly track the filtered result of get_implementations_for_trait separately from the overall list of trait impls. I'd expect we probably would want to. If that's the case, it should _not_ be checked off until this has been done.
• exported symbols
• dllimport_foreign_items is Tracked, but like implementations_of_trait I'd expect we probably still want to track is_dllimport_foreign_item (which should be unchecked, BTW, as the linked PR for it was closed, not merged).
• panic_strategy
• dylib_dependency_formats

[cramertj]

item_generics_cloned should also be checked off -- it's used in librustc/middle/resolve_lifetime.rs, and only has access to a Session.

[kennytm]

Could the list in https://github.com/rust-lang/rust/issues/41417#issuecomment-296678068 be updated? #41766 was closed, and some are done in later PRs. Specifically,

• [x] is_const_fn is done in #42598
• [x] is_default_impl is done in #41917
• [ ] is_dllimport_foreign_item should be unchecked.

Also,

• [x] impl_polarity is done in #41469
• [ ] impl_defaultness is missing
• [x] is_staged_api has been deleted in #41847
• [x] is_allocator is done in #42598
• [x] is_panic_runtime is done in #42598
• [ ] is_profiler_runtime is missing
• [x] extern_crate is done in #42598
• [x] retrace_path has been deleted in #43361
• [ ] maybe_get_item_body is renamed to item_body in #41408
• [x] metadata_filename has been deleted in #41565, replaced by metadata_loader.
• [x] metadata_section_name has been deleted in #41565, replaced by metadata_loader.
• [ ] metadata_loader is missing