From 4dae09ecff7fc8bcb05ad6bd5f831e89d8933615 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Fri, 21 Feb 2025 09:16:25 -0500 Subject: [PATCH] [red-knot] Better handling of visibility constraint copies (#16276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related changes. For context: 1. We were maintaining two separate arenas of `Constraint`s in each use-def map. One was used for narrowing constraints, and the other for visibility constraints. The visibility constraint arena was interned, ensuring that we always used the same ID for any particular `Constraint`. The narrowing constraint arena was not interned. 2. The TDD code relies on _all_ TDD nodes being interned and reduced. This is an important requirement for TDDs to be a canonical form, which allows us to use a single int comparison to test for "always true/false" and to compare two TDDs for equivalence. But we also need to support an individual `Constraint` having multiple values in a TDD evaluation (e.g. to handle a `while` condition having different values the first time it's evaluated vs later times). Previously, we handled that by introducing a "copy" number, which was only there as a disambiguator, to allow an interned, deduplicated constraint ID to appear in the TDD formula multiple times. A better way to handle (2) is to not intern the constraints in the visibility constraint arena! The caller now gets to decide: if they add a `Constraint` to the arena more than once, they get distinct `ScopedConstraintId`s — which the TDD code will treat as distinct variables, allowing them to take on different values in the ternary function. With that in place, we can then consolidate on a single (non-interned) arena, which is shared for both narrowing and visibility constraints. --------- Co-authored-by: Carl Meyer --- .../src/semantic_index/builder.rs | 31 +++-- .../src/semantic_index/constraint.rs | 30 +++++ .../src/semantic_index/use_def.rs | 55 ++++----- .../semantic_index/use_def/symbol_state.rs | 5 +- crates/red_knot_python_semantic/src/symbol.rs | 18 +-- .../src/visibility_constraints.rs | 113 +++++------------- 6 files changed, 113 insertions(+), 139 deletions(-) diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index 1074cbceb6..3f77df38cd 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -15,7 +15,7 @@ use crate::module_name::ModuleName; use crate::semantic_index::ast_ids::node_key::ExpressionNodeKey; use crate::semantic_index::ast_ids::AstIdsBuilder; use crate::semantic_index::attribute_assignment::{AttributeAssignment, AttributeAssignments}; -use crate::semantic_index::constraint::PatternConstraintKind; +use crate::semantic_index::constraint::{PatternConstraintKind, ScopedConstraintId}; use crate::semantic_index::definition::{ AssignmentDefinitionNodeRef, ComprehensionDefinitionNodeRef, Definition, DefinitionNodeKey, DefinitionNodeRef, ForStmtDefinitionNodeRef, ImportFromDefinitionNodeRef, @@ -26,7 +26,7 @@ use crate::semantic_index::symbol::{ SymbolTableBuilder, }; use crate::semantic_index::use_def::{ - EagerBindingsKey, FlowSnapshot, ScopedConstraintId, ScopedEagerBindingsId, UseDefMapBuilder, + EagerBindingsKey, FlowSnapshot, ScopedEagerBindingsId, UseDefMapBuilder, }; use crate::semantic_index::SemanticIndex; use crate::unpack::{Unpack, UnpackValue}; @@ -294,7 +294,7 @@ impl<'db> SemanticIndexBuilder<'db> { &self.use_def_maps[scope_id] } - fn current_visibility_constraints_mut(&mut self) -> &mut VisibilityConstraintsBuilder<'db> { + fn current_visibility_constraints_mut(&mut self) -> &mut VisibilityConstraintsBuilder { let scope_id = self.current_scope(); &mut self.use_def_maps[scope_id].visibility_constraints } @@ -406,16 +406,12 @@ impl<'db> SemanticIndexBuilder<'db> { } /// Negates a constraint and adds it to the list of all constraints, does not record it. - fn add_negated_constraint( - &mut self, - constraint: Constraint<'db>, - ) -> (Constraint<'db>, ScopedConstraintId) { + fn add_negated_constraint(&mut self, constraint: Constraint<'db>) -> ScopedConstraintId { let negated = Constraint { node: constraint.node, is_positive: false, }; - let id = self.current_use_def_map_mut().add_constraint(negated); - (negated, id) + self.current_use_def_map_mut().add_constraint(negated) } /// Records a previously added constraint by adding it to all live bindings. @@ -431,7 +427,7 @@ impl<'db> SemanticIndexBuilder<'db> { /// Negates the given constraint and then adds it to all live bindings. fn record_negated_constraint(&mut self, constraint: Constraint<'db>) -> ScopedConstraintId { - let (_, id) = self.add_negated_constraint(constraint); + let id = self.add_negated_constraint(constraint); self.record_constraint_id(id); id } @@ -460,9 +456,10 @@ impl<'db> SemanticIndexBuilder<'db> { &mut self, constraint: Constraint<'db>, ) -> ScopedVisibilityConstraintId { + let constraint_id = self.current_use_def_map_mut().add_constraint(constraint); let id = self .current_visibility_constraints_mut() - .add_atom(constraint, 0); + .add_atom(constraint_id); self.record_visibility_constraint_id(id); id } @@ -1192,12 +1189,14 @@ where // We need multiple copies of the visibility constraint for the while condition, // since we need to model situations where the first evaluation of the condition // returns True, but a later evaluation returns False. + let first_constraint_id = self.current_use_def_map_mut().add_constraint(constraint); + let later_constraint_id = self.current_use_def_map_mut().add_constraint(constraint); let first_vis_constraint_id = self .current_visibility_constraints_mut() - .add_atom(constraint, 0); + .add_atom(first_constraint_id); let later_vis_constraint_id = self .current_visibility_constraints_mut() - .add_atom(constraint, 1); + .add_atom(later_constraint_id); // Save aside any break states from an outer loop let saved_break_states = std::mem::take(&mut self.loop_break_states); @@ -1778,13 +1777,13 @@ where // anymore. if index < values.len() - 1 { let constraint = self.build_constraint(value); - let (constraint, constraint_id) = match op { - ast::BoolOp::And => (constraint, self.add_constraint(constraint)), + let constraint_id = match op { + ast::BoolOp::And => self.add_constraint(constraint), ast::BoolOp::Or => self.add_negated_constraint(constraint), }; let visibility_constraint = self .current_visibility_constraints_mut() - .add_atom(constraint, 0); + .add_atom(constraint_id); let after_expr = self.flow_snapshot(); diff --git a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs index cee7e472ed..0249f6385b 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs @@ -1,10 +1,40 @@ use ruff_db::files::File; +use ruff_index::{newtype_index, IndexVec}; use ruff_python_ast::Singleton; use crate::db::Db; use crate::semantic_index::expression::Expression; use crate::semantic_index::symbol::{FileScopeId, ScopeId}; +// A scoped identifier for each `Constraint` in a scope. +#[newtype_index] +#[derive(Ord, PartialOrd)] +pub(crate) struct ScopedConstraintId; + +// A collection of constraints. This is currently stored in `UseDefMap`, which means we maintain a +// separate set of constraints for each scope in a file. +pub(crate) type Constraints<'db> = IndexVec>; + +#[derive(Debug, Default)] +pub(crate) struct ConstraintsBuilder<'db> { + constraints: IndexVec>, +} + +impl<'db> ConstraintsBuilder<'db> { + /// Adds a constraint. Note that we do not deduplicate constraints. If you add a `Constraint` + /// more than once, you will get distinct `ScopedConstraintId`s for each one. (This lets you + /// model constraint expressions that might evaluate to different values at different points of + /// execution.) + pub(crate) fn add_constraint(&mut self, constraint: Constraint<'db>) -> ScopedConstraintId { + self.constraints.push(constraint) + } + + pub(crate) fn build(mut self) -> Constraints<'db> { + self.constraints.shrink_to_fit(); + self.constraints + } +} + #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, salsa::Update)] pub(crate) struct Constraint<'db> { pub(crate) node: ConstraintNode<'db>, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index 99ea469c4d..bc58c79a74 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -165,7 +165,7 @@ //! don't actually store these "list of visible definitions" as a vector of [`Definition`]. //! Instead, [`SymbolBindings`] and [`SymbolDeclarations`] are structs which use bit-sets to track //! definitions (and constraints, in the case of bindings) in terms of [`ScopedDefinitionId`] and -//! [`ScopedConstraintId`], which are indices into the `all_definitions` and `all_constraints` +//! [`ScopedConstraintId`], which are indices into the `all_definitions` and `constraints` //! indexvecs in the [`UseDefMap`]. //! //! There is another special kind of possible "definition" for a symbol: there might be a path from @@ -255,27 +255,27 @@ //! snapshot, and merging a snapshot into the current state. The logic using these methods lives in //! [`SemanticIndexBuilder`](crate::semantic_index::builder::SemanticIndexBuilder), e.g. where it //! visits a `StmtIf` node. -pub(crate) use self::symbol_state::ScopedConstraintId; + +use ruff_index::{newtype_index, IndexVec}; +use rustc_hash::FxHashMap; + use self::symbol_state::{ ConstraintIndexIterator, LiveBindingsIterator, LiveDeclaration, LiveDeclarationsIterator, ScopedDefinitionId, SymbolBindings, SymbolDeclarations, SymbolState, }; use crate::semantic_index::ast_ids::ScopedUseId; +use crate::semantic_index::constraint::{ + Constraint, Constraints, ConstraintsBuilder, ScopedConstraintId, +}; use crate::semantic_index::definition::Definition; use crate::semantic_index::symbol::{FileScopeId, ScopedSymbolId}; use crate::visibility_constraints::{ ScopedVisibilityConstraintId, VisibilityConstraints, VisibilityConstraintsBuilder, }; -use ruff_index::{newtype_index, IndexVec}; -use rustc_hash::FxHashMap; - -use super::constraint::Constraint; mod bitset; mod symbol_state; -type AllConstraints<'db> = IndexVec>; - /// Applicable definitions and constraints for every use of a name. #[derive(Debug, PartialEq, Eq, salsa::Update)] pub(crate) struct UseDefMap<'db> { @@ -284,10 +284,10 @@ pub(crate) struct UseDefMap<'db> { all_definitions: IndexVec>>, /// Array of [`Constraint`] in this scope. - all_constraints: AllConstraints<'db>, + constraints: Constraints<'db>, /// Array of visibility constraints in this scope. - visibility_constraints: VisibilityConstraints<'db>, + visibility_constraints: VisibilityConstraints, /// [`SymbolBindings`] reaching a [`ScopedUseId`]. bindings_by_use: IndexVec, @@ -369,7 +369,7 @@ impl<'db> UseDefMap<'db> { ) -> BindingWithConstraintsIterator<'map, 'db> { BindingWithConstraintsIterator { all_definitions: &self.all_definitions, - all_constraints: &self.all_constraints, + constraints: &self.constraints, visibility_constraints: &self.visibility_constraints, inner: bindings.iter(), } @@ -381,6 +381,7 @@ impl<'db> UseDefMap<'db> { ) -> DeclarationsIterator<'map, 'db> { DeclarationsIterator { all_definitions: &self.all_definitions, + constraints: &self.constraints, visibility_constraints: &self.visibility_constraints, inner: declarations.iter(), } @@ -414,8 +415,8 @@ type EagerBindings = IndexVec; #[derive(Debug)] pub(crate) struct BindingWithConstraintsIterator<'map, 'db> { all_definitions: &'map IndexVec>>, - all_constraints: &'map AllConstraints<'db>, - pub(crate) visibility_constraints: &'map VisibilityConstraints<'db>, + pub(crate) constraints: &'map Constraints<'db>, + pub(crate) visibility_constraints: &'map VisibilityConstraints, inner: LiveBindingsIterator<'map>, } @@ -423,14 +424,14 @@ impl<'map, 'db> Iterator for BindingWithConstraintsIterator<'map, 'db> { type Item = BindingWithConstraints<'map, 'db>; fn next(&mut self) -> Option { - let all_constraints = self.all_constraints; + let constraints = self.constraints; self.inner .next() .map(|live_binding| BindingWithConstraints { binding: self.all_definitions[live_binding.binding], - constraints: ConstraintsIterator { - all_constraints, + narrowing_constraints: ConstraintsIterator { + constraints, constraint_ids: live_binding.narrowing_constraints.iter(), }, visibility_constraint: live_binding.visibility_constraint, @@ -442,12 +443,12 @@ impl std::iter::FusedIterator for BindingWithConstraintsIterator<'_, '_> {} pub(crate) struct BindingWithConstraints<'map, 'db> { pub(crate) binding: Option>, - pub(crate) constraints: ConstraintsIterator<'map, 'db>, + pub(crate) narrowing_constraints: ConstraintsIterator<'map, 'db>, pub(crate) visibility_constraint: ScopedVisibilityConstraintId, } pub(crate) struct ConstraintsIterator<'map, 'db> { - all_constraints: &'map AllConstraints<'db>, + constraints: &'map Constraints<'db>, constraint_ids: ConstraintIndexIterator<'map>, } @@ -457,7 +458,7 @@ impl<'db> Iterator for ConstraintsIterator<'_, 'db> { fn next(&mut self) -> Option { self.constraint_ids .next() - .map(|constraint_id| self.all_constraints[ScopedConstraintId::from_u32(constraint_id)]) + .map(|constraint_id| self.constraints[ScopedConstraintId::from_u32(constraint_id)]) } } @@ -465,7 +466,8 @@ impl std::iter::FusedIterator for ConstraintsIterator<'_, '_> {} pub(crate) struct DeclarationsIterator<'map, 'db> { all_definitions: &'map IndexVec>>, - pub(crate) visibility_constraints: &'map VisibilityConstraints<'db>, + pub(crate) constraints: &'map Constraints<'db>, + pub(crate) visibility_constraints: &'map VisibilityConstraints, inner: LiveDeclarationsIterator<'map>, } @@ -506,11 +508,11 @@ pub(super) struct UseDefMapBuilder<'db> { /// Append-only array of [`Definition`]. all_definitions: IndexVec>>, - /// Append-only array of [`Constraint`]. - all_constraints: AllConstraints<'db>, + /// Builder of constraints. + constraints: ConstraintsBuilder<'db>, /// Builder of visibility constraints. - pub(super) visibility_constraints: VisibilityConstraintsBuilder<'db>, + pub(super) visibility_constraints: VisibilityConstraintsBuilder, /// A constraint which describes the visibility of the unbound/undeclared state, i.e. /// whether or not the start of the scope is visible. This is important for cases like @@ -539,7 +541,7 @@ impl Default for UseDefMapBuilder<'_> { fn default() -> Self { Self { all_definitions: IndexVec::from_iter([None]), - all_constraints: IndexVec::new(), + constraints: ConstraintsBuilder::default(), visibility_constraints: VisibilityConstraintsBuilder::default(), scope_start_visibility: ScopedVisibilityConstraintId::ALWAYS_TRUE, bindings_by_use: IndexVec::new(), @@ -572,7 +574,7 @@ impl<'db> UseDefMapBuilder<'db> { } pub(super) fn add_constraint(&mut self, constraint: Constraint<'db>) -> ScopedConstraintId { - self.all_constraints.push(constraint) + self.constraints.add_constraint(constraint) } pub(super) fn record_constraint_id(&mut self, constraint: ScopedConstraintId) { @@ -752,7 +754,6 @@ impl<'db> UseDefMapBuilder<'db> { pub(super) fn finish(mut self) -> UseDefMap<'db> { self.all_definitions.shrink_to_fit(); - self.all_constraints.shrink_to_fit(); self.symbol_states.shrink_to_fit(); self.bindings_by_use.shrink_to_fit(); self.declarations_by_binding.shrink_to_fit(); @@ -761,7 +762,7 @@ impl<'db> UseDefMapBuilder<'db> { UseDefMap { all_definitions: self.all_definitions, - all_constraints: self.all_constraints, + constraints: self.constraints.build(), visibility_constraints: self.visibility_constraints.build(), bindings_by_use: self.bindings_by_use, public_symbols: self.symbol_states, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs index 6eccbd0535..0dabb54902 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs @@ -48,6 +48,7 @@ use itertools::{EitherOrBoth, Itertools}; use ruff_index::newtype_index; use smallvec::{smallvec, SmallVec}; +use crate::semantic_index::constraint::ScopedConstraintId; use crate::semantic_index::use_def::bitset::{BitSet, BitSetIterator}; use crate::semantic_index::use_def::VisibilityConstraintsBuilder; use crate::visibility_constraints::ScopedVisibilityConstraintId; @@ -66,10 +67,6 @@ impl ScopedDefinitionId { pub(super) const UNBOUND: ScopedDefinitionId = ScopedDefinitionId::from_u32(0); } -/// A newtype-index for a constraint expression in a particular scope. -#[newtype_index] -pub(crate) struct ScopedConstraintId; - /// Can reference this * 64 total constraints inline; more will fall back to the heap. const INLINE_CONSTRAINT_BLOCKS: usize = 2; diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 1fc45dc7c1..f3c2a33fc8 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/crates/red_knot_python_semantic/src/symbol.rs @@ -503,6 +503,7 @@ fn symbol_from_bindings_impl<'db>( bindings_with_constraints: BindingWithConstraintsIterator<'_, 'db>, requires_explicit_reexport: RequiresExplicitReExport, ) -> Symbol<'db> { + let constraints = bindings_with_constraints.constraints; let visibility_constraints = bindings_with_constraints.visibility_constraints; let mut bindings_with_constraints = bindings_with_constraints.peekable(); @@ -514,9 +515,9 @@ fn symbol_from_bindings_impl<'db>( Some(BindingWithConstraints { binding, visibility_constraint, - constraints: _, + narrowing_constraints: _, }) if binding.map_or(true, is_non_exported) => { - visibility_constraints.evaluate(db, *visibility_constraint) + visibility_constraints.evaluate(db, constraints, *visibility_constraint) } _ => Truthiness::AlwaysFalse, }; @@ -524,7 +525,7 @@ fn symbol_from_bindings_impl<'db>( let mut types = bindings_with_constraints.filter_map( |BindingWithConstraints { binding, - constraints, + narrowing_constraints, visibility_constraint, }| { let binding = binding?; @@ -533,13 +534,14 @@ fn symbol_from_bindings_impl<'db>( return None; } - let static_visibility = visibility_constraints.evaluate(db, visibility_constraint); + let static_visibility = + visibility_constraints.evaluate(db, constraints, visibility_constraint); if static_visibility.is_always_false() { return None; } - let mut constraint_tys = constraints + let mut constraint_tys = narrowing_constraints .filter_map(|constraint| narrowing_constraint(db, constraint, binding)) .peekable(); @@ -590,6 +592,7 @@ fn symbol_from_declarations_impl<'db>( declarations: DeclarationsIterator<'_, 'db>, requires_explicit_reexport: RequiresExplicitReExport, ) -> SymbolFromDeclarationsResult<'db> { + let constraints = declarations.constraints; let visibility_constraints = declarations.visibility_constraints; let mut declarations = declarations.peekable(); @@ -602,7 +605,7 @@ fn symbol_from_declarations_impl<'db>( declaration, visibility_constraint, }) if declaration.map_or(true, is_non_exported) => { - visibility_constraints.evaluate(db, *visibility_constraint) + visibility_constraints.evaluate(db, constraints, *visibility_constraint) } _ => Truthiness::AlwaysFalse, }; @@ -618,7 +621,8 @@ fn symbol_from_declarations_impl<'db>( return None; } - let static_visibility = visibility_constraints.evaluate(db, visibility_constraint); + let static_visibility = + visibility_constraints.evaluate(db, constraints, visibility_constraint); if static_visibility.is_always_false() { None diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 35355db6f1..97cfe06369 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -178,7 +178,9 @@ use std::cmp::Ordering; use ruff_index::{Idx, IndexVec}; use rustc_hash::FxHashMap; -use crate::semantic_index::constraint::{Constraint, ConstraintNode, PatternConstraintKind}; +use crate::semantic_index::constraint::{ + Constraint, ConstraintNode, Constraints, PatternConstraintKind, ScopedConstraintId, +}; use crate::types::{infer_expression_type, Truthiness}; use crate::Db; @@ -231,69 +233,15 @@ impl std::fmt::Debug for ScopedVisibilityConstraintId { #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] struct InteriorNode { - atom: Atom, + /// A "variable" that is evaluated as part of a TDD ternary function. For visibility + /// constraints, this is a `Constraint` that represents some runtime property of the Python + /// code that we are evaluating. + atom: ScopedConstraintId, if_true: ScopedVisibilityConstraintId, if_ambiguous: ScopedVisibilityConstraintId, if_false: ScopedVisibilityConstraintId, } -/// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, -/// this is a `Constraint` that represents some runtime property of the Python code that we are -/// evaluating. We intern these constraints in an arena ([`VisibilityConstraints::constraints`]). -/// An atom is then an index into this arena. -/// -/// By using a 32-bit index, we would typically allow 4 billion distinct constraints within a -/// scope. However, we sometimes have to model how a `Constraint` can have a different runtime -/// value at different points in the execution of the program. To handle this, we reserve the top -/// byte of an atom to represent a "copy number". This is just an opaque value that allows -/// different `Atom`s to evaluate the same `Constraint`. This yields a maximum of 16 million -/// distinct `Constraint`s in a scope, and 256 possible copies of each of those constraints. -#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] -struct Atom(u32); - -impl Atom { - /// Deconstruct an atom into a constraint index and a copy number. - #[inline] - fn into_index_and_copy(self) -> (u32, u8) { - let copy = self.0 >> 24; - let index = self.0 & 0x00ff_ffff; - (index, copy as u8) - } - - #[inline] - fn copy_of(mut self, copy: u8) -> Self { - // Clear out the previous copy number - self.0 &= 0x00ff_ffff; - // OR in the new one - self.0 |= u32::from(copy) << 24; - self - } -} - -// A custom Debug implementation that prints out the constraint index and copy number as distinct -// fields. -impl std::fmt::Debug for Atom { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let (index, copy) = self.into_index_and_copy(); - f.debug_tuple("Atom").field(&index).field(©).finish() - } -} - -impl Idx for Atom { - #[inline] - fn new(value: usize) -> Self { - assert!(value <= 0x00ff_ffff); - #[allow(clippy::cast_possible_truncation)] - Self(value as u32) - } - - #[inline] - fn index(self) -> usize { - let (index, _) = self.into_index_and_copy(); - index as usize - } -} - impl ScopedVisibilityConstraintId { /// A special ID that is used for an "always true" / "always visible" constraint. pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = @@ -336,16 +284,13 @@ const SMALLEST_TERMINAL: ScopedVisibilityConstraintId = ALWAYS_FALSE; /// A collection of visibility constraints. This is currently stored in `UseDefMap`, which means we /// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq, salsa::Update)] -pub(crate) struct VisibilityConstraints<'db> { - constraints: IndexVec>, +pub(crate) struct VisibilityConstraints { interiors: IndexVec, } #[derive(Debug, Default, PartialEq, Eq)] -pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: IndexVec>, +pub(crate) struct VisibilityConstraintsBuilder { interiors: IndexVec, - constraint_cache: FxHashMap, Atom>, interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< @@ -358,10 +303,9 @@ pub(crate) struct VisibilityConstraintsBuilder<'db> { >, } -impl<'db> VisibilityConstraintsBuilder<'db> { - pub(crate) fn build(self) -> VisibilityConstraints<'db> { +impl VisibilityConstraintsBuilder { + pub(crate) fn build(self) -> VisibilityConstraints { VisibilityConstraints { - constraints: self.constraints, interiors: self.interiors, } } @@ -385,14 +329,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } - /// Adds a constraint, ensuring that we only store any particular constraint once. - fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - self.constraint_cache - .entry(constraint) - .or_insert_with(|| self.constraints.push(constraint)) - .copy_of(copy) - } - /// Adds an interior node, ensuring that we always use the same visibility constraint ID for /// equal nodes. fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { @@ -408,17 +344,23 @@ impl<'db> VisibilityConstraintsBuilder<'db> { .or_insert_with(|| self.interiors.push(node)) } - /// Adds a new visibility constraint that checks a single [`Constraint`]. Provide different - /// values for `copy` if you need to model that the constraint can evaluate to different - /// results at different points in the execution of the program being modeled. + /// Adds a new visibility constraint that checks a single [`Constraint`]. + /// + /// [`ScopedConstraintId`]s are the “variables” that are evaluated by a TDD. A TDD variable has + /// the same value no matter how many times it appears in the ternary formula that the TDD + /// represents. + /// + /// However, we sometimes have to model how a `Constraint` can have a different runtime + /// value at different points in the execution of the program. To handle this, you can take + /// advantage of the fact that the [`Constraints`] arena does not deduplicate `Constraint`s. + /// You can add a `Constraint` multiple times, yielding different `ScopedConstraintId`s, which + /// you can then create separate TDD atoms for. pub(crate) fn add_atom( &mut self, - constraint: Constraint<'db>, - copy: u8, + constraint: ScopedConstraintId, ) -> ScopedVisibilityConstraintId { - let atom = self.add_constraint(constraint, copy); self.add_interior(InteriorNode { - atom, + atom: constraint, if_true: ALWAYS_TRUE, if_ambiguous: AMBIGUOUS, if_false: ALWAYS_FALSE, @@ -588,11 +530,12 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } -impl<'db> VisibilityConstraints<'db> { +impl VisibilityConstraints { /// Analyze the statically known visibility for a given visibility constraint. - pub(crate) fn evaluate( + pub(crate) fn evaluate<'db>( &self, db: &'db dyn Db, + constraints: &Constraints<'db>, mut id: ScopedVisibilityConstraintId, ) -> Truthiness { loop { @@ -602,7 +545,7 @@ impl<'db> VisibilityConstraints<'db> { ALWAYS_FALSE => return Truthiness::AlwaysFalse, _ => self.interiors[id], }; - let constraint = &self.constraints[node.atom]; + let constraint = &constraints[node.atom]; match Self::analyze_single(db, constraint) { Truthiness::AlwaysTrue => id = node.if_true, Truthiness::Ambiguous => id = node.if_ambiguous,