Skip to content

Remove all uses of NodeId in ResolverOutputs #72402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 24, 2020
Merged

Remove all uses of NodeId in ResolverOutputs #72402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5728c53
Use `HirId` as key for `ResolverOutputs::trait_map` instead of `NodeId`
marmeladema May 20, 2020
3c5dba7
Use `HirId` in value of `ResolverOutputs::trait_map` instead of `NodeId`
marmeladema May 20, 2020
52359f7
Use `HirId` in `ResolverOutputs::export_map` instead of `NodeId`
marmeladema May 20, 2020
13c86f2
Use `LocalDefId` in `ResolverOutputs::maybe_unused_trait_imports` ins…
marmeladema May 20, 2020
25f575b
Use `DefId` in `ResolverOutputs::maybe_unused_extern_crates` instead …
marmeladema May 20, 2020
21f65ae
Use `DefId` in `ResolverOutputs::glob_map` instead of `NodeId`
marmeladema May 20, 2020
8ff6ffd
Use `DefId` in `ResolverOutputs::extern_crate_map` instead of `NodeId`
marmeladema May 20, 2020
4c4cb46
Use `collect()` instead of manually inserting elements into maps
marmeladema May 21, 2020
f31e076
Replace unecessary calls to `.clone()` by argument binding pattern fo…
marmeladema May 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/librustc_hir/definitions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,9 @@ impl Definitions {

#[inline]
pub fn local_def_id(&self, node: ast::NodeId) -> LocalDefId {
self.opt_local_def_id(node).unwrap()
self.opt_local_def_id(node).unwrap_or_else(|| {
panic!("no entry for node id: `{:?}` / `{:?}`", node, self.opt_node_id_to_hir_id(node))
})
}

#[inline]
Expand Down
45 changes: 7 additions & 38 deletions src/librustc_middle/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use crate::ty::{InferConst, ParamConst};
use crate::ty::{List, TyKind, TyS};
use rustc_ast::ast;
use rustc_ast::expand::allocator::AllocatorKind;
use rustc_ast::node_id::NodeMap;
use rustc_attr as attr;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::profiling::SelfProfilerRef;
Expand Down Expand Up @@ -923,7 +922,7 @@ pub struct GlobalCtxt<'tcx> {
pub consts: CommonConsts<'tcx>,

/// Resolutions of `extern crate` items produced by resolver.
extern_crate_map: NodeMap<CrateNum>,
extern_crate_map: FxHashMap<DefId, CrateNum>,

/// Map indicating what traits are in scope for places where this
/// is relevant; generated by resolve.
Expand Down Expand Up @@ -1113,13 +1112,8 @@ impl<'tcx> TyCtxt<'tcx> {
};

let mut trait_map: FxHashMap<_, FxHashMap<_, _>> = FxHashMap::default();
for (k, v) in resolutions.trait_map {
let hir_id = definitions.node_id_to_hir_id(k);
for (hir_id, v) in resolutions.trait_map.into_iter() {
let map = trait_map.entry(hir_id.owner).or_default();
let v = v
.into_iter()
.map(|tc| tc.map_import_ids(|id| definitions.node_id_to_hir_id(id)))
.collect();
map.insert(hir_id.local_id, StableVec::new(v));
}

Expand All @@ -1136,32 +1130,10 @@ impl<'tcx> TyCtxt<'tcx> {
consts: common_consts,
extern_crate_map: resolutions.extern_crate_map,
trait_map,
export_map: resolutions
.export_map
.into_iter()
.map(|(k, v)| {
let exports: Vec<_> = v
.into_iter()
.map(|e| e.map_id(|id| definitions.node_id_to_hir_id(id)))
.collect();
(k, exports)
})
.collect(),
maybe_unused_trait_imports: resolutions
.maybe_unused_trait_imports
.into_iter()
.map(|id| definitions.local_def_id(id))
.collect(),
maybe_unused_extern_crates: resolutions
.maybe_unused_extern_crates
.into_iter()
.map(|(id, sp)| (definitions.local_def_id(id).to_def_id(), sp))
.collect(),
glob_map: resolutions
.glob_map
.into_iter()
.map(|(id, names)| (definitions.local_def_id(id), names))
.collect(),
export_map: resolutions.export_map,
maybe_unused_trait_imports: resolutions.maybe_unused_trait_imports,
maybe_unused_extern_crates: resolutions.maybe_unused_extern_crates,
glob_map: resolutions.glob_map,
extern_prelude: resolutions.extern_prelude,
untracked_crate: krate,
definitions,
Expand Down Expand Up @@ -2709,10 +2681,7 @@ pub fn provide(providers: &mut ty::query::Providers<'_>) {
let id = tcx.hir().local_def_id_to_hir_id(id.expect_local());
tcx.stability().local_deprecation_entry(id)
};
providers.extern_mod_stmt_cnum = |tcx, id| {
let id = tcx.hir().as_local_node_id(id).unwrap();
tcx.extern_crate_map.get(&id).cloned()
};
providers.extern_mod_stmt_cnum = |tcx, id| tcx.extern_crate_map.get(&id).cloned();
providers.all_crate_nums = |tcx, cnum| {
assert_eq!(cnum, LOCAL_CRATE);
tcx.arena.alloc_slice(&tcx.cstore.crates_untracked())
Expand Down
16 changes: 8 additions & 8 deletions src/librustc_middle/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ use crate::ty;
use crate::ty::subst::{InternalSubsts, Subst, SubstsRef};
use crate::ty::util::{Discr, IntTypeExt};
use rustc_ast::ast;
use rustc_ast::node_id::{NodeId, NodeMap, NodeSet};
use rustc_attr as attr;
use rustc_data_structures::captures::Captures;
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::sorted_map::SortedIndexMultiMap;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
Expand All @@ -31,7 +31,7 @@ use rustc_hir as hir;
use rustc_hir::def::{CtorKind, CtorOf, DefKind, Namespace, Res};
use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LocalDefId, CRATE_DEF_INDEX};
use rustc_hir::lang_items::{FnMutTraitLangItem, FnOnceTraitLangItem, FnTraitLangItem};
use rustc_hir::{Constness, GlobMap, Node, TraitMap};
use rustc_hir::{Constness, Node};
use rustc_index::vec::{Idx, IndexVec};
use rustc_macros::HashStable;
use rustc_serialize::{self, Encodable, Encoder};
Expand Down Expand Up @@ -120,12 +120,12 @@ mod sty;
pub struct ResolverOutputs {
pub definitions: rustc_hir::definitions::Definitions,
pub cstore: Box<CrateStoreDyn>,
pub extern_crate_map: NodeMap<CrateNum>,
pub trait_map: TraitMap<NodeId>,
pub maybe_unused_trait_imports: NodeSet,
pub maybe_unused_extern_crates: Vec<(NodeId, Span)>,
pub export_map: ExportMap<NodeId>,
pub glob_map: GlobMap,
pub extern_crate_map: FxHashMap<DefId, CrateNum>,
pub trait_map: FxHashMap<hir::HirId, Vec<hir::TraitCandidate<hir::HirId>>>,
pub maybe_unused_trait_imports: FxHashSet<LocalDefId>,
pub maybe_unused_extern_crates: Vec<(DefId, Span)>,
pub export_map: ExportMap<hir::HirId>,
pub glob_map: FxHashMap<LocalDefId, FxHashSet<Symbol>>,
Comment on lines +123 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem like all of these could plausibly be DefId. Did you find something during your experiments that wasn't a definition? If not, we should open a cleanup issue once this gets merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only tried for trait_map but yes, for this field, there is no def id, only hir id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petrochenkov I see you've assigned yourself. Any recommendations here? Is there a reason that trait_map and export_map cannot use DefIds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also because the global context uses HirId, using DefId here would mean another map clone/conversion in librustc_middle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zoo of DefIds, LocalDefIds and HirIds here is a bit surprising (all of these could be LocalDefIds (or HirIds)), but it looks like the choices are made to minimize conversions at use sites.

I guess the right thing to do here would be changing DefIds into LocalDefIds in GlobalCtxt which is the end destination of these tables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can definitely take a look at that in a follow up PR!

/// Extern prelude entries. The value is `true` if the entry was introduced
/// via `extern crate` item and not `--extern` option or compiler built-in.
pub extern_prelude: FxHashMap<Symbol, bool>,
Expand Down
114 changes: 101 additions & 13 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// ignore-tidy-filelength

//! This crate is responsible for the part of name resolution that doesn't require type checker.
//!
//! Module structure of the crate is built here.
Expand Down Expand Up @@ -1271,15 +1273,60 @@ impl<'a> Resolver<'a> {
}

pub fn into_outputs(self) -> ResolverOutputs {
let definitions = self.definitions;
let extern_crate_map = self
.extern_crate_map
.into_iter()
.map(|(k, v)| (definitions.local_def_id(k).to_def_id(), v))
.collect();
let export_map = self
.export_map
.into_iter()
.map(|(k, v)| {
(
k,
v.into_iter()
.map(|e| e.map_id(|id| definitions.node_id_to_hir_id(id)))
.collect(),
)
})
.collect();
let trait_map = self
.trait_map
.into_iter()
.map(|(k, v)| {
(
definitions.node_id_to_hir_id(k),
v.into_iter()
.map(|tc| tc.map_import_ids(|id| definitions.node_id_to_hir_id(id)))
.collect(),
)
})
.collect();
let maybe_unused_trait_imports = self
.maybe_unused_trait_imports
.into_iter()
.map(|id| definitions.local_def_id(id))
.collect();
let maybe_unused_extern_crates = self
.maybe_unused_extern_crates
.into_iter()
.map(|(id, sp)| (definitions.local_def_id(id).to_def_id(), sp))
.collect();
let glob_map = self
.glob_map
.into_iter()
.map(|(id, names)| (definitions.local_def_id(id), names))
.collect();
ResolverOutputs {
definitions: self.definitions,
definitions: definitions,
cstore: Box::new(self.crate_loader.into_cstore()),
extern_crate_map: self.extern_crate_map,
export_map: self.export_map,
trait_map: self.trait_map,
glob_map: self.glob_map,
maybe_unused_trait_imports: self.maybe_unused_trait_imports,
maybe_unused_extern_crates: self.maybe_unused_extern_crates,
extern_crate_map,
export_map,
trait_map,
glob_map,
maybe_unused_trait_imports,
maybe_unused_extern_crates,
extern_prelude: self
.extern_prelude
.iter()
Expand All @@ -1292,12 +1339,53 @@ impl<'a> Resolver<'a> {
ResolverOutputs {
definitions: self.definitions.clone(),
cstore: Box::new(self.cstore().clone()),
extern_crate_map: self.extern_crate_map.clone(),
export_map: self.export_map.clone(),
trait_map: self.trait_map.clone(),
glob_map: self.glob_map.clone(),
maybe_unused_trait_imports: self.maybe_unused_trait_imports.clone(),
maybe_unused_extern_crates: self.maybe_unused_extern_crates.clone(),
extern_crate_map: self
.extern_crate_map
.iter()
.map(|(&k, &v)| (self.definitions.local_def_id(k).to_def_id(), v))
.collect(),
export_map: self
.export_map
.iter()
.map(|(&k, v)| {
(
k,
v.iter()
.map(|e| e.map_id(|id| self.definitions.node_id_to_hir_id(id)))
.collect(),
)
})
.collect(),
trait_map: self
.trait_map
.iter()
.map(|(&k, v)| {
(
self.definitions.node_id_to_hir_id(k),
v.iter()
.cloned()
.map(|tc| {
tc.map_import_ids(|id| self.definitions.node_id_to_hir_id(id))
})
.collect(),
)
})
.collect(),
glob_map: self
.glob_map
.iter()
.map(|(&id, names)| (self.definitions.local_def_id(id), names.clone()))
.collect(),
maybe_unused_trait_imports: self
.maybe_unused_trait_imports
.iter()
.map(|&id| self.definitions.local_def_id(id))
.collect(),
maybe_unused_extern_crates: self
.maybe_unused_extern_crates
.iter()
.map(|&(id, sp)| (self.definitions.local_def_id(id).to_def_id(), sp))
.collect(),
extern_prelude: self
.extern_prelude
.iter()
Expand Down