[red-knot] Internal refactoring of visibility constraints API (#15913)

This extracts some pure refactoring noise from
#15861. This changes the API for
creating and evaluating visibility constraints, but does not change how
they are respresented internally. There should be no behavioral or
performance changes in this PR.

Changes:

- Hide the internal representation isn't changed, so that we can make
changes to it in #15861.
- Add a separate builder type for visibility constraints. (With TDDs, we
will have some additional builder state that we can throw away once
we're done constructing.)
- Remove a layer of helper methods from `UseDefMapBuilder`, making
`SemanticIndexBuilder` responsible for constructing whatever visibility
constraints it needs.

crates/red_knot_python_semantic/src/semantic_index.rs

@@ -32,7 +32,7 @@ mod use_def;
 
 pub(crate) use self::use_def::{
     BindingWithConstraints, BindingWithConstraintsIterator, DeclarationWithConstraint,
-    DeclarationsIterator, ScopedVisibilityConstraintId,
+    DeclarationsIterator,
 };
 
 type SymbolMap = hashbrown::HashMap<ScopedSymbolId, (), FxBuildHasher>;

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -25,12 +25,10 @@ use crate::semantic_index::symbol::{
     FileScopeId, NodeWithScopeKey, NodeWithScopeRef, Scope, ScopeId, ScopeKind, ScopedSymbolId,
     SymbolTableBuilder,
 };
-use crate::semantic_index::use_def::{
-    FlowSnapshot, ScopedConstraintId, ScopedVisibilityConstraintId, UseDefMapBuilder,
-};
+use crate::semantic_index::use_def::{FlowSnapshot, ScopedConstraintId, UseDefMapBuilder};
 use crate::semantic_index::SemanticIndex;
 use crate::unpack::{Unpack, UnpackValue};
-use crate::visibility_constraints::VisibilityConstraint;
+use crate::visibility_constraints::{ScopedVisibilityConstraintId, VisibilityConstraintsBuilder};
 use crate::Db;
 
 use super::constraint::{Constraint, ConstraintNode, PatternConstraint};
@@ -232,6 +230,11 @@ impl<'db> SemanticIndexBuilder<'db> {
         &self.use_def_maps[scope_id]
     }
 
+    fn current_visibility_constraints_mut(&mut self) -> &mut VisibilityConstraintsBuilder<'db> {
+        let scope_id = self.current_scope();
+        &mut self.use_def_maps[scope_id].visibility_constraints
+    }
+
     fn current_ast_ids(&mut self) -> &mut AstIdsBuilder {
         let scope_id = self.current_scope();
         &mut self.ast_ids[scope_id]
@@ -367,39 +370,35 @@ impl<'db> SemanticIndexBuilder<'db> {
         id
     }
 
-    /// Adds a new visibility constraint, but does not record it. Returns the constraint ID
-    /// for later recording using [`SemanticIndexBuilder::record_visibility_constraint_id`].
-    fn add_visibility_constraint(
-        &mut self,
-        constraint: VisibilityConstraint<'db>,
-    ) -> ScopedVisibilityConstraintId {
-        self.current_use_def_map_mut()
-            .add_visibility_constraint(constraint)
-    }
-
     /// Records a previously added visibility constraint by applying it to all live bindings
     /// and declarations.
     fn record_visibility_constraint_id(&mut self, constraint: ScopedVisibilityConstraintId) {
         self.current_use_def_map_mut()
-            .record_visibility_constraint_id(constraint);
+            .record_visibility_constraint(constraint);
     }
 
     /// Negates the given visibility constraint and then adds it to all live bindings and declarations.
     fn record_negated_visibility_constraint(
         &mut self,
         constraint: ScopedVisibilityConstraintId,
     ) -> ScopedVisibilityConstraintId {
-        self.current_use_def_map_mut()
-            .record_visibility_constraint(VisibilityConstraint::VisibleIfNot(constraint))
+        let id = self
+            .current_visibility_constraints_mut()
+            .add_not_constraint(constraint);
+        self.record_visibility_constraint_id(id);
+        id
     }
 
     /// Records a visibility constraint by applying it to all live bindings and declarations.
     fn record_visibility_constraint(
         &mut self,
         constraint: Constraint<'db>,
     ) -> ScopedVisibilityConstraintId {
-        self.current_use_def_map_mut()
-            .record_visibility_constraint(VisibilityConstraint::VisibleIf(constraint, 0))
+        let id = self
+            .current_visibility_constraints_mut()
+            .add_atom(constraint, 0);
+        self.record_visibility_constraint_id(id);
+        id
     }
 
     /// Records that all remaining statements in the current block are unreachable, and therefore
@@ -408,10 +407,10 @@ impl<'db> SemanticIndexBuilder<'db> {
         self.current_use_def_map_mut().mark_unreachable();
     }
 
-    /// Records a [`VisibilityConstraint::Ambiguous`] constraint.
-    fn record_ambiguous_visibility(&mut self) -> ScopedVisibilityConstraintId {
+    /// Records a visibility constraint that always evaluates to "ambiguous".
+    fn record_ambiguous_visibility(&mut self) {
         self.current_use_def_map_mut()
-            .record_visibility_constraint(VisibilityConstraint::Ambiguous)
+            .record_visibility_constraint(ScopedVisibilityConstraintId::AMBIGUOUS);
     }
 
     /// Simplifies (resets) visibility constraints on all live bindings and declarations that did
@@ -1091,10 +1090,12 @@ 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_vis_constraint_id =
-                    self.add_visibility_constraint(VisibilityConstraint::VisibleIf(constraint, 0));
-                let later_vis_constraint_id =
-                    self.add_visibility_constraint(VisibilityConstraint::VisibleIf(constraint, 1));
+                let first_vis_constraint_id = self
+                    .current_visibility_constraints_mut()
+                    .add_atom(constraint, 0);
+                let later_vis_constraint_id = self
+                    .current_visibility_constraints_mut()
+                    .add_atom(constraint, 1);
 
                 // Save aside any break states from an outer loop
                 let saved_break_states = std::mem::take(&mut self.loop_break_states);
@@ -1665,9 +1666,9 @@ where
                             ast::BoolOp::And => (constraint, self.add_constraint(constraint)),
                             ast::BoolOp::Or => self.add_negated_constraint(constraint),
                         };
-                        let visibility_constraint = self.add_visibility_constraint(
-                            VisibilityConstraint::VisibleIf(constraint, 0),
-                        );
+                        let visibility_constraint = self
+                            .current_visibility_constraints_mut()
+                            .add_atom(constraint, 0);
 
                         let after_expr = self.flow_snapshot();
 

crates/red_knot_python_semantic/src/semantic_index/use_def.rs

@@ -255,16 +255,18 @@
 //! 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 self::symbol_state::{
     BindingIdWithConstraintsIterator, ConstraintIdIterator, DeclarationIdIterator,
     ScopedDefinitionId, SymbolBindings, SymbolDeclarations, SymbolState,
 };
-pub(crate) use self::symbol_state::{ScopedConstraintId, ScopedVisibilityConstraintId};
 use crate::semantic_index::ast_ids::ScopedUseId;
 use crate::semantic_index::definition::Definition;
 use crate::semantic_index::symbol::ScopedSymbolId;
 use crate::semantic_index::use_def::symbol_state::DeclarationIdWithConstraint;
-use crate::visibility_constraints::{VisibilityConstraint, VisibilityConstraints};
+use crate::visibility_constraints::{
+    ScopedVisibilityConstraintId, VisibilityConstraints, VisibilityConstraintsBuilder,
+};
 use ruff_index::IndexVec;
 use rustc_hash::FxHashMap;
 
@@ -285,7 +287,7 @@ pub(crate) struct UseDefMap<'db> {
     /// Array of [`Constraint`] in this scope.
     all_constraints: AllConstraints<'db>,
 
-    /// Array of [`VisibilityConstraint`]s in this scope.
+    /// Array of visibility constraints in this scope.
     visibility_constraints: VisibilityConstraints<'db>,
 
     /// [`SymbolBindings`] reaching a [`ScopedUseId`].
@@ -487,8 +489,8 @@ pub(super) struct UseDefMapBuilder<'db> {
     /// Append-only array of [`Constraint`].
     all_constraints: AllConstraints<'db>,
 
-    /// Append-only array of [`VisibilityConstraint`].
-    visibility_constraints: VisibilityConstraints<'db>,
+    /// Builder of visibility constraints.
+    pub(super) visibility_constraints: VisibilityConstraintsBuilder<'db>,
 
     /// 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
@@ -513,7 +515,7 @@ impl Default for UseDefMapBuilder<'_> {
         Self {
             all_definitions: IndexVec::from_iter([None]),
             all_constraints: IndexVec::new(),
-            visibility_constraints: VisibilityConstraints::default(),
+            visibility_constraints: VisibilityConstraintsBuilder::default(),
             scope_start_visibility: ScopedVisibilityConstraintId::ALWAYS_TRUE,
             bindings_by_use: IndexVec::new(),
             definitions_by_definition: FxHashMap::default(),
@@ -561,35 +563,18 @@ impl<'db> UseDefMapBuilder<'db> {
         new_constraint_id
     }
 
-    pub(super) fn add_visibility_constraint(
-        &mut self,
-        constraint: VisibilityConstraint<'db>,
-    ) -> ScopedVisibilityConstraintId {
-        self.visibility_constraints.add(constraint)
-    }
-
-    pub(super) fn record_visibility_constraint_id(
+    pub(super) fn record_visibility_constraint(
         &mut self,
         constraint: ScopedVisibilityConstraintId,
     ) {
         for state in &mut self.symbol_states {
             state.record_visibility_constraint(&mut self.visibility_constraints, constraint);
         }
-
         self.scope_start_visibility = self
             .visibility_constraints
             .add_and_constraint(self.scope_start_visibility, constraint);
     }
 
-    pub(super) fn record_visibility_constraint(
-        &mut self,
-        constraint: VisibilityConstraint<'db>,
-    ) -> ScopedVisibilityConstraintId {
-        let new_constraint_id = self.add_visibility_constraint(constraint);
-        self.record_visibility_constraint_id(new_constraint_id);
-        new_constraint_id
-    }
-
     /// This method resets the visibility constraints for all symbols to a previous state
     /// *if* there have been no new declarations or bindings since then. Consider the
     /// following example:
@@ -742,7 +727,7 @@ impl<'db> UseDefMapBuilder<'db> {
         UseDefMap {
             all_definitions: self.all_definitions,
             all_constraints: self.all_constraints,
-            visibility_constraints: self.visibility_constraints,
+            visibility_constraints: self.visibility_constraints.build(),
             bindings_by_use: self.bindings_by_use,
             public_symbols: self.symbol_states,
             definitions_by_definition: self.definitions_by_definition,

crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs

@@ -49,7 +49,8 @@ use ruff_index::newtype_index;
 use smallvec::SmallVec;
 
 use crate::semantic_index::use_def::bitset::{BitSet, BitSetIterator};
-use crate::semantic_index::use_def::VisibilityConstraints;
+use crate::semantic_index::use_def::VisibilityConstraintsBuilder;
+use crate::visibility_constraints::ScopedVisibilityConstraintId;
 
 /// A newtype-index for a definition in a particular scope.
 #[newtype_index]
@@ -99,18 +100,6 @@ type ConstraintsPerBinding = SmallVec<InlineConstraintArray>;
 /// Iterate over all constraints for a single binding.
 type ConstraintsIterator<'a> = std::slice::Iter<'a, Constraints>;
 
-/// A newtype-index for a visibility constraint in a particular scope.
-#[newtype_index]
-pub(crate) struct ScopedVisibilityConstraintId;
-
-impl ScopedVisibilityConstraintId {
-    /// A special ID that is used for an "always true" / "always visible" constraint.
-    /// When we create a new [`VisibilityConstraints`] object, this constraint is always
-    /// present at index 0.
-    pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId =
-        ScopedVisibilityConstraintId::from_u32(0);
-}
-
 const INLINE_VISIBILITY_CONSTRAINTS: usize = 4;
 type InlineVisibilityConstraintsArray =
     [ScopedVisibilityConstraintId; INLINE_VISIBILITY_CONSTRAINTS];
@@ -164,7 +153,7 @@ impl SymbolDeclarations {
     /// Add given visibility constraint to all live declarations.
     pub(super) fn record_visibility_constraint(
         &mut self,
-        visibility_constraints: &mut VisibilityConstraints,
+        visibility_constraints: &mut VisibilityConstraintsBuilder,
         constraint: ScopedVisibilityConstraintId,
     ) {
         for existing in &mut self.visibility_constraints {
@@ -180,7 +169,7 @@ impl SymbolDeclarations {
         }
     }
 
-    fn merge(&mut self, b: Self, visibility_constraints: &mut VisibilityConstraints) {
+    fn merge(&mut self, b: Self, visibility_constraints: &mut VisibilityConstraintsBuilder) {
         let a = std::mem::take(self);
         self.live_declarations = a.live_declarations.clone();
         self.live_declarations.union(&b.live_declarations);
@@ -270,7 +259,7 @@ impl SymbolBindings {
     /// Add given visibility constraint to all live bindings.
     pub(super) fn record_visibility_constraint(
         &mut self,
-        visibility_constraints: &mut VisibilityConstraints,
+        visibility_constraints: &mut VisibilityConstraintsBuilder,
         constraint: ScopedVisibilityConstraintId,
     ) {
         for existing in &mut self.visibility_constraints {
@@ -287,7 +276,7 @@ impl SymbolBindings {
         }
     }
 
-    fn merge(&mut self, mut b: Self, visibility_constraints: &mut VisibilityConstraints) {
+    fn merge(&mut self, mut b: Self, visibility_constraints: &mut VisibilityConstraintsBuilder) {
         let mut a = std::mem::take(self);
         self.live_bindings = a.live_bindings.clone();
         self.live_bindings.union(&b.live_bindings);
@@ -373,7 +362,7 @@ impl SymbolState {
     /// Add given visibility constraint to all live bindings.
     pub(super) fn record_visibility_constraint(
         &mut self,
-        visibility_constraints: &mut VisibilityConstraints,
+        visibility_constraints: &mut VisibilityConstraintsBuilder,
         constraint: ScopedVisibilityConstraintId,
     ) {
         self.bindings
@@ -401,7 +390,7 @@ impl SymbolState {
     pub(super) fn merge(
         &mut self,
         b: SymbolState,
-        visibility_constraints: &mut VisibilityConstraints,
+        visibility_constraints: &mut VisibilityConstraintsBuilder,
     ) {
         self.bindings.merge(b.bindings, visibility_constraints);
         self.declarations
@@ -584,7 +573,7 @@ mod tests {
 
     #[test]
     fn merge() {
-        let mut visibility_constraints = VisibilityConstraints::default();
+        let mut visibility_constraints = VisibilityConstraintsBuilder::default();
 
         // merging the same definition with the same constraint keeps the constraint
         let mut sym1a = SymbolState::undefined(ScopedVisibilityConstraintId::ALWAYS_TRUE);
@@ -655,7 +644,7 @@ mod tests {
 
     #[test]
     fn record_declaration_merge() {
-        let mut visibility_constraints = VisibilityConstraints::default();
+        let mut visibility_constraints = VisibilityConstraintsBuilder::default();
         let mut sym = SymbolState::undefined(ScopedVisibilityConstraintId::ALWAYS_TRUE);
         sym.record_declaration(ScopedDefinitionId::from_u32(1));
 
@@ -669,7 +658,7 @@ mod tests {
 
     #[test]
     fn record_declaration_merge_partial_undeclared() {
-        let mut visibility_constraints = VisibilityConstraints::default();
+        let mut visibility_constraints = VisibilityConstraintsBuilder::default();
         let mut sym = SymbolState::undefined(ScopedVisibilityConstraintId::ALWAYS_TRUE);
         sym.record_declaration(ScopedDefinitionId::from_u32(1));