improve type safety

Author: AlexWaygood

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -519,6 +519,7 @@ impl<'db> SemanticIndexBuilder<'db> {
     }
 
     /// Records a visibility constraint by applying it to all live bindings and declarations.
+    #[must_use = "A visibility constraint must always be negated after it is added"]
     fn record_visibility_constraint(
         &mut self,
         predicate: Predicate<'db>,

crates/red_knot_python_semantic/src/semantic_index/use_def.rs

@@ -701,24 +701,27 @@ impl<'db> UseDefMapBuilder<'db> {
             .add_and_constraint(self.scope_start_visibility, constraint);
     }
 
+    #[must_use = "A `*`-import visibility constraint must always be negated after it is added"]
     pub(super) fn record_star_import_visibility_constraint(
         &mut self,
         star_import: StarImportPlaceholderPredicate<'db>,
         symbol: ScopedSymbolId,
-    ) -> ScopedVisibilityConstraintId {
+    ) -> StarImportVisibilityConstraintId {
         let predicate_id = self.add_predicate(star_import.into());
         let visibility_id = self.visibility_constraints.add_atom(predicate_id);
         self.symbol_states[symbol]
             .record_visibility_constraint(&mut self.visibility_constraints, visibility_id);
-        visibility_id
+        StarImportVisibilityConstraintId(visibility_id)
     }
 
     pub(super) fn negate_star_import_visibility_constraint(
         &mut self,
         symbol_id: ScopedSymbolId,
-        constraint: ScopedVisibilityConstraintId,
+        constraint: StarImportVisibilityConstraintId,
     ) {
-        let negated_constraint = self.visibility_constraints.add_not_constraint(constraint);
+        let negated_constraint = self
+            .visibility_constraints
+            .add_not_constraint(constraint.into_scoped_constraint_id());
         self.symbol_states[symbol_id]
             .record_visibility_constraint(&mut self.visibility_constraints, negated_constraint);
         self.scope_start_visibility = self
@@ -925,3 +928,24 @@ impl<'db> UseDefMapBuilder<'db> {
         }
     }
 }
+
+/// Newtype wrapper over [`ScopedVisibilityConstraintId`] to improve type safety.
+///
+/// By returning this type from [`UseDefMapBuilder::record_star_import_visibility_constraint`]
+/// rather than [`ScopedVisibilityConstraintId`] directly, we ensure that
+/// [`UseDefMapBuilder::negate_star_import_visibility_constraint`] must be called after the
+/// visibility constraint has been added, and we ensure that
+/// [`super::SemanticIndexBuilder::record_negated_visibility_constraint`] *cannot* be called with
+/// the narrowing constraint (which would lead to incorrect behaviour).
+///
+/// This type is defined here rather than in the [`super::visibility_constraints`] module
+/// because it should only ever be constructed and deconstructed from methods in the
+/// [`UseDefMapBuilder`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(super) struct StarImportVisibilityConstraintId(ScopedVisibilityConstraintId);
+
+impl StarImportVisibilityConstraintId {
+    fn into_scoped_constraint_id(self) -> ScopedVisibilityConstraintId {
+        self.0
+    }
+}