Skip to content

Remove global next_disambiguator state and handle it with a DisambiguatorState type #140453

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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 1 addition & 2 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,8 @@ impl<'hir> LoweringContext<'_, 'hir> {
let mut generic_args = ThinVec::new();
for (idx, arg) in args.iter().cloned().enumerate() {
if legacy_args_idx.contains(&idx) {
let parent_def_id = self.current_hir_id_owner.def_id;
let node_id = self.next_node_id();
self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, f.span);
self.create_def(node_id, None, DefKind::AnonConst, f.span);
let mut visitor = WillCreateDefIdsVisitor {};
let const_value = if let ControlFlow::Break(span) = visitor.visit_expr(&arg) {
AstP(Expr {
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_ast_lowering/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,12 +494,12 @@ enum GenericArgsMode {
impl<'a, 'hir> LoweringContext<'a, 'hir> {
fn create_def(
&mut self,
parent: LocalDefId,
node_id: ast::NodeId,
name: Option<Symbol>,
def_kind: DefKind,
span: Span,
) -> LocalDefId {
let parent = self.current_hir_id_owner.def_id;
debug_assert_ne!(node_id, ast::DUMMY_NODE_ID);
assert!(
self.opt_local_def_id(node_id).is_none(),
Expand All @@ -509,7 +509,11 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
self.tcx.hir_def_key(self.local_def_id(node_id)),
);

let def_id = self.tcx.at(span).create_def(parent, name, def_kind).def_id();
let def_id = self
.tcx
.at(span)
.create_def(parent, name, def_kind, None, &mut self.resolver.disambiguator)
.def_id();

debug!("create_def: def_id_to_node_id[{:?}] <-> {:?}", def_id, node_id);
self.resolver.node_id_to_def_id.insert(node_id, def_id);
Expand Down Expand Up @@ -781,7 +785,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
LifetimeRes::Fresh { param, kind, .. } => {
// Late resolution delegates to us the creation of the `LocalDefId`.
let _def_id = self.create_def(
self.current_hir_id_owner.def_id,
param,
Some(kw::UnderscoreLifetime),
DefKind::LifetimeParam,
Expand Down Expand Up @@ -2107,16 +2110,14 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
hir::ConstArgKind::Path(qpath)
} else {
// Construct an AnonConst where the expr is the "ty"'s path.

let parent_def_id = self.current_hir_id_owner.def_id;
let node_id = self.next_node_id();
let span = self.lower_span(span);

// Add a definition for the in-band const def.
// We're lowering a const argument that was originally thought to be a type argument,
// so the def collector didn't create the def ahead of time. That's why we have to do
// it here.
let def_id = self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, span);
let def_id = self.create_def(node_id, None, DefKind::AnonConst, span);
let hir_id = self.lower_node_id(node_id);

let path_expr = Expr {
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_ast_lowering/src/pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,14 +517,13 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
span: Span,
base_type: Span,
) -> &'hir hir::ConstArg<'hir> {
let parent_def_id = self.current_hir_id_owner.def_id;
let node_id = self.next_node_id();

// Add a definition for the in-band const def.
// We're generating a range end that didn't exist in the AST,
// so the def collector didn't create the def ahead of time. That's why we have to do
// it here.
let def_id = self.create_def(parent_def_id, node_id, None, DefKind::AnonConst, span);
let def_id = self.create_def(node_id, None, DefKind::AnonConst, span);
let hir_id = self.lower_node_id(node_id);

let unstable_span = self.mark_span_with_reason(
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_const_eval/src/const_eval/dummy_machine.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use rustc_hir::def_id::LocalDefId;
use rustc_hir::definitions::DisambiguatorState;
use rustc_middle::mir::interpret::{AllocId, ConstAllocation, InterpResult};
use rustc_middle::mir::*;
use rustc_middle::query::TyCtxtAt;
Expand Down Expand Up @@ -42,7 +44,7 @@ pub macro throw_machine_stop_str($($tt:tt)*) {{
pub struct DummyMachine;

impl HasStaticRootDefId for DummyMachine {
fn static_def_id(&self) -> Option<rustc_hir::def_id::LocalDefId> {
fn static_parent_and_disambiguator(&mut self) -> Option<(LocalDefId, &mut DisambiguatorState)> {
None
}
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use rustc_abi::{Align, Size};
use rustc_ast::Mutability;
use rustc_data_structures::fx::{FxHashMap, FxIndexMap, IndexEntry};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::definitions::DisambiguatorState;
use rustc_hir::{self as hir, CRATE_HIR_ID, LangItem};
use rustc_middle::mir::AssertMessage;
use rustc_middle::mir::interpret::ReportedErrorInfo;
Expand Down Expand Up @@ -63,7 +64,7 @@ pub struct CompileTimeMachine<'tcx> {
/// If `Some`, we are evaluating the initializer of the static with the given `LocalDefId`,
/// storing the result in the given `AllocId`.
/// Used to prevent reads from a static's base allocation, as that may allow for self-initialization loops.
Copy link
Member

Choose a reason for hiding this comment

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

This doc comment should be updated to explain the role of this DisambiguatorState component.

Looking at the const_eval diff in this PR, I am completely clueless about what is happening.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Const eval is generating statics (not quite sure why, that's should probably be part of the comment) and DisambiguatorState is used to ensure these have unique names (def paths).

Copy link
Member

Choose a reason for hiding this comment

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

Const eval is generating statics (not quite sure why, that's should probably be part of the comment)

That part I can explain. :D Though it's way outside the scope for this comment I think -- this field is not directly related to generating those statics.

A static can require more than one allocation for its result, e.g. if you do static S = &Vec::new();. We generate anonymous statics for those inner allocations. (Note that this example does not involve any const promotion.)

It seems like what you are doing is funneling some state through to the allocation interner. That is the place where we should have comments explaining why and how we generate anonymous statics.

DisambiguatorState is used to ensure these have unique names (def paths).

Seems like ideally this would be managed by the interner. It's not relevant during const-eval, only after the end of const-eval, so ideally it should not be present during const-eval.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea that one is my fault. It was just the minimal diff allowing this, doing it solely in the interner... I guess would work unconditionally, not even basing it off on whether we are interning a static item, but just generally. Most code won't use it, but initializing an empty map is not more expensive than wrapping it in an option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which interner are you talking about? It could also just be a counter, if it's per-source static.

I also noticed it's using a nested symbol. That's should probably be a DefPathData variant to avoid collisions with source items.

Copy link
Member

Choose a reason for hiding this comment

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

Which interner are you talking about?

compiler/rustc_const_eval/src/interpret/intern.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't look like it has any query local state to put DisambiguatorState in?

Copy link
Member

Choose a reason for hiding this comment

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

It's not a query, so I don't know what you mean.

intern_const_alloc_recursive would take an &mut DisambiguatorState and forward it to where it is needed; the other entry points should never hit intern_as_new_static.

pub(crate) static_root_ids: Option<(AllocId, LocalDefId)>,
pub(crate) static_root_ids: Option<(AllocId, LocalDefId, DisambiguatorState)>,

/// A cache of "data range" computations for unions (i.e., the offsets of non-padding bytes).
union_data_ranges: FxHashMap<Ty<'tcx>, RangeSet>,
Expand Down Expand Up @@ -706,7 +707,7 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {

fn before_alloc_read(ecx: &InterpCx<'tcx, Self>, alloc_id: AllocId) -> InterpResult<'tcx> {
// Check if this is the currently evaluated static.
if Some(alloc_id) == ecx.machine.static_root_ids.map(|(id, _)| id) {
if Some(alloc_id) == ecx.machine.static_root_ids.as_ref().map(|(id, ..)| *id) {
return Err(ConstEvalErrKind::RecursiveStatic).into();
}
// If this is another static, make sure we fire off the query to detect cycles.
Expand Down
15 changes: 10 additions & 5 deletions compiler/rustc_const_eval/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use hir::def::DefKind;
use rustc_ast::Mutability;
use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
use rustc_hir as hir;
use rustc_hir::definitions::DisambiguatorState;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
use rustc_middle::query::TyCtxtAt;
Expand Down Expand Up @@ -46,12 +47,13 @@ pub trait CompileTimeMachine<'tcx, T> = Machine<
pub trait HasStaticRootDefId {
/// Returns the `DefId` of the static item that is currently being evaluated.
/// Used for interning to be able to handle nested allocations.
fn static_def_id(&self) -> Option<LocalDefId>;
fn static_parent_and_disambiguator(&mut self) -> Option<(LocalDefId, &mut DisambiguatorState)>;
}

impl HasStaticRootDefId for const_eval::CompileTimeMachine<'_> {
fn static_def_id(&self) -> Option<LocalDefId> {
Some(self.static_root_ids?.1)
fn static_parent_and_disambiguator(&mut self) -> Option<(LocalDefId, &mut DisambiguatorState)> {
let (_, static_id, d) = self.static_root_ids.as_mut()?;
Some((*static_id, d))
}
}

Expand Down Expand Up @@ -87,8 +89,8 @@ fn intern_shallow<'tcx, T, M: CompileTimeMachine<'tcx, T>>(
}
// link the alloc id to the actual allocation
let alloc = ecx.tcx.mk_const_alloc(alloc);
if let Some(static_id) = ecx.machine.static_def_id() {
intern_as_new_static(ecx.tcx, static_id, alloc_id, alloc);
if let Some((static_id, disambiguator)) = ecx.machine.static_parent_and_disambiguator() {
intern_as_new_static(ecx.tcx, static_id, alloc_id, alloc, disambiguator);
} else {
ecx.tcx.set_alloc_id_memory(alloc_id, alloc);
}
Expand All @@ -102,11 +104,14 @@ fn intern_as_new_static<'tcx>(
static_id: LocalDefId,
alloc_id: AllocId,
alloc: ConstAllocation<'tcx>,
disambiguator: &mut DisambiguatorState,
) {
let feed = tcx.create_def(
static_id,
Some(sym::nested),
DefKind::Static { safety: hir::Safety::Safe, mutability: alloc.0.mutability, nested: true },
None,
disambiguator,
);
tcx.set_nested_alloc_id_static(alloc_id, feed.def_id());

Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_const_eval/src/interpret/util.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use rustc_hir::def_id::LocalDefId;
use rustc_hir::definitions::DisambiguatorState;
use rustc_middle::mir;
use rustc_middle::mir::interpret::{AllocInit, Allocation, InterpResult, Pointer};
use rustc_middle::ty::layout::TyAndLayout;
Expand Down Expand Up @@ -40,8 +41,8 @@ pub(crate) fn create_static_alloc<'tcx>(
) -> InterpResult<'tcx, MPlaceTy<'tcx>> {
let alloc = Allocation::try_new(layout.size, layout.align.abi, AllocInit::Uninit)?;
let alloc_id = ecx.tcx.reserve_and_set_static_alloc(static_def_id.into());
assert_eq!(ecx.machine.static_root_ids, None);
ecx.machine.static_root_ids = Some((alloc_id, static_def_id));
assert!(ecx.machine.static_root_ids.is_none());
ecx.machine.static_root_ids = Some((alloc_id, static_def_id, DisambiguatorState::new()));
assert!(ecx.memory.alloc_map.insert(alloc_id, (MemoryKind::Stack, alloc)).is_none());
interp_ok(ecx.ptr_to_mplace(Pointer::from(alloc_id).into(), layout))
}
11 changes: 3 additions & 8 deletions compiler/rustc_hir/src/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,14 +269,9 @@ impl DefKind {
| DefKind::TyParam
| DefKind::ExternCrate => DefPathData::TypeNs(name.unwrap()),

// An associated type name will be missing for an RPITIT.
DefKind::AssocTy => {
if let Some(name) = name {
DefPathData::TypeNs(name)
} else {
DefPathData::AnonAssocTy
}
}
// An associated type name will be missing for an RPITIT (DefPathData::AnonAssocTy),
// but those provide their own DefPathData.
DefKind::AssocTy => DefPathData::TypeNs(name.unwrap()),

// It's not exactly an anon const, but wrt DefPathData, there
// is no difference.
Expand Down
56 changes: 44 additions & 12 deletions compiler/rustc_hir/src/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl DefPathTable {
//
// See the documentation for DefPathHash for more information.
panic!(
"found DefPathHash collision between {def_path1:?} and {def_path2:?}. \
"found DefPathHash collision between {def_path1:#?} and {def_path2:#?}. \
Compilation cannot continue."
);
}
Expand Down Expand Up @@ -97,13 +97,31 @@ impl DefPathTable {
}
}

#[derive(Debug)]
pub struct DisambiguatorState {
next: UnordMap<(LocalDefId, DefPathData), u32>,
}

impl DisambiguatorState {
pub fn new() -> Self {
Self { next: Default::default() }
Copy link
Contributor

Choose a reason for hiding this comment

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

#[derive(Default)] can be used instead of this.

}

/// Creates a `DisambiguatorState` where the next allocated `(LocalDefId, DefPathData)` pair
/// will have `index` as the disambiguator.
pub fn with(def_id: LocalDefId, data: DefPathData, index: u32) -> Self {
let mut this = Self::new();
this.next.insert((def_id, data), index);
this
}
}

/// The definition table containing node definitions.
/// It holds the `DefPathTable` for `LocalDefId`s/`DefPath`s.
/// It also stores mappings to convert `LocalDefId`s to/from `HirId`s.
#[derive(Debug)]
pub struct Definitions {
table: DefPathTable,
next_disambiguator: UnordMap<(LocalDefId, DefPathData), u32>,
}

/// A unique identifier that we can use to lookup a definition
Expand Down Expand Up @@ -173,7 +191,11 @@ impl DisambiguatedDefPathData {
}
}
DefPathDataName::Anon { namespace } => {
write!(writer, "{{{}#{}}}", namespace, self.disambiguator)
if let DefPathData::AnonAssocTy(method) = self.data {
write!(writer, "{}::{{{}#{}}}", method, namespace, self.disambiguator)
} else {
write!(writer, "{{{}#{}}}", namespace, self.disambiguator)
}
}
}
}
Expand Down Expand Up @@ -288,7 +310,7 @@ pub enum DefPathData {
/// Argument position `impl Trait` have a `TypeNs` with their pretty-printed name.
OpaqueTy,
/// An anonymous associated type from an RPITIT.
AnonAssocTy,
AnonAssocTy(Symbol),
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a comment telling what the symbol is.

/// A synthetic body for a coroutine's by-move body.
SyntheticCoroutineBody,
}
Expand Down Expand Up @@ -342,24 +364,33 @@ impl Definitions {
let root = LocalDefId { local_def_index: table.allocate(key, def_path_hash) };
assert_eq!(root.local_def_index, CRATE_DEF_INDEX);

Definitions { table, next_disambiguator: Default::default() }
Definitions { table }
}

/// Adds a definition with a parent definition.
pub fn create_def(&mut self, parent: LocalDefId, data: DefPathData) -> LocalDefId {
/// Creates a definition with a parent definition.
/// If there are multiple definitions with the same DefPathData and the same parent, use
/// `disambiguator` to differentiate them. Distinct `DisambiguatorState` instances are not
/// guaranteed to generate unique disambiguators and should instead ensure that the `parent`
/// and `data` pair is distinct from other instances.
pub fn create_def(
&mut self,
parent: LocalDefId,
data: DefPathData,
disambiguator: &mut DisambiguatorState,
) -> LocalDefId {
// We can't use `Debug` implementation for `LocalDefId` here, since it tries to acquire a
// reference to `Definitions` and we're already holding a mutable reference.
debug!(
"create_def(parent={}, data={data:?})",
self.def_path(parent).to_string_no_crate_verbose(),
);

// The root node must be created with `create_root_def()`.
// The root node must be created in `new()`.
assert!(data != DefPathData::CrateRoot);

// Find the next free disambiguator for this key.
let disambiguator = {
let next_disamb = self.next_disambiguator.entry((parent, data)).or_insert(0);
let next_disamb = disambiguator.next.entry((parent, data)).or_insert(0);
let disambiguator = *next_disamb;
*next_disamb = next_disamb.checked_add(1).expect("disambiguator overflow");
disambiguator
Expand Down Expand Up @@ -411,7 +442,9 @@ impl DefPathData {
pub fn get_opt_name(&self) -> Option<Symbol> {
use self::DefPathData::*;
match *self {
TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) => Some(name),
TypeNs(name) | ValueNs(name) | MacroNs(name) | LifetimeNs(name) | AnonAssocTy(name) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The AnonAssocTy case here doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does seem a little bit odd. I could duplicate get_opt_name to have a variant that's used for stable hashing.

Some(name)
}

Impl
| ForeignMod
Expand All @@ -422,7 +455,6 @@ impl DefPathData {
| Ctor
| AnonConst
| OpaqueTy
| AnonAssocTy
| SyntheticCoroutineBody => None,
}
}
Expand All @@ -443,7 +475,7 @@ impl DefPathData {
Ctor => DefPathDataName::Anon { namespace: sym::constructor },
AnonConst => DefPathDataName::Anon { namespace: sym::constant },
OpaqueTy => DefPathDataName::Anon { namespace: sym::opaque },
AnonAssocTy => DefPathDataName::Anon { namespace: sym::anon_assoc },
AnonAssocTy(..) => DefPathDataName::Anon { namespace: sym::anon_assoc },
SyntheticCoroutineBody => DefPathDataName::Anon { namespace: sym::synthetic },
}
}
Expand Down
Loading
Loading