[red-knot] Improve handling of visibility constraints in external modules when resolving * imports

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -1185,6 +1185,20 @@ where
 
                         let referenced_module = module.file();
 
+                        // In order to understand the visibility of definitions created by a `*` import,
+                        // we need to know the visibility of the global-scope definitions in the
+                        // `referenced_module` the symbols imported from. Much like predicates for `if`
+                        // statements can only have their visibility constraints resolved at type-inference
+                        // time, the visibility of these global-scope definitions in the external module
+                        // cannot be resolved at this point. As such, we essentially model each definition
+                        // stemming from a `from exporter *` import as something like:
+                        //
+                        // ```py
+                        // if <external_definition_is_visible>:
+                        //     from exporter import name
+                        // ```
+                        //
+                        // For more details, see the doc-comment on `StarImportPlaceholderPredicate`.
                         for export in exported_names(self.db, referenced_module) {
                             let symbol_id = self.add_symbol(export.clone());
                             let node_ref = StarImportDefinitionNodeRef { node, symbol_id };
@@ -1194,13 +1208,10 @@ where
                                 symbol_id,
                                 referenced_module,
                             );
-                            let predicate = Predicate {
-                                node: PredicateNode::StarImportPlaceholder(star_import),
-                                is_positive: true,
-                            };
                             let pre_definition = self.flow_snapshot();
                             self.push_additional_definition(symbol_id, node_ref);
-                            let constraint_id = self.record_visibility_constraint(predicate);
+                            let constraint_id =
+                                self.record_visibility_constraint(star_import.into());
                             let post_definition = self.flow_snapshot();
                             self.flow_restore(pre_definition.clone());
                             self.record_negated_visibility_constraint(constraint_id);

crates/red_knot_python_semantic/src/semantic_index/predicate.rs

@@ -93,22 +93,22 @@ impl<'db> PatternPredicate<'db> {
 /// until type-inference time. This is essentially the same as a standard visibility constraint,
 /// so we reuse the [`Predicate`] infrastructure to model it.
 ///
-/// To illustrate, say we have a module `a.py` like so:
+/// To illustrate, say we have a module `exporter.py` like so:
 ///
 /// ```py
 /// if <condition>:
 ///     class A: ...
 /// ```
 ///
-/// and we have a module `b.py` like so:
+/// and we have a module `importer.py` like so:
 ///
 /// ```py
 /// A = 1
 ///
-/// from a import *
+/// from importer import *
 /// ```
 ///
-/// Since we cannot know whether or not <test> is true at semantic-index time,
+/// Since we cannot know whether or not <condition> is true at semantic-index time,
 /// we record a definition for `A` in `b.py` as a result of the `from a import *`
 /// statement, but place a predicate on it to record the fact that we don't yet
 /// know whether this definition will be visible from all control-flow paths or not.
@@ -132,15 +132,33 @@ impl<'db> PatternPredicate<'db> {
 pub(crate) struct StarImportPlaceholderPredicate<'db> {
     pub(crate) importing_file: File,
 
+    /// Each symbol imported by a `*` import has a separate predicate associated with it:
+    /// this field identifies which symbol that is.
+    ///
+    /// Note that a [`ScopedSymbolId`] is only meaningful if you also know the scope
+    /// it is relative to. For this specific struct, however, there's no need to store a
+    /// separate field to hold the ID of the scope. `StarImportPredicate`s are only created
+    /// for valid `*`-import definitions, and valid `*`-import definitions can only ever
+    /// exist in the global scope; thus, we know that the `symbol_id` here will be relative
+    /// to the global scope.
     pub(crate) symbol_id: ScopedSymbolId,
 
     pub(crate) referenced_file: File,
 }
 
 impl<'db> StarImportPlaceholderPredicate<'db> {
     pub(crate) fn scope(self, db: &'db dyn Db) -> ScopeId<'db> {
-        // `StarImportPredicate`s are only created for valid `*`-import definitions,
-        // and valid `*`-import definitions can only ever exist in the global scope.
+        // See doc-comment above [`StarImportPlaceholderPredicate::symbol_id`]:
+        // valid `*`-import definitions can only take place in the global scope.
         global_scope(db, self.importing_file(db))
     }
 }
+
+impl<'db> From<StarImportPlaceholderPredicate<'db>> for Predicate<'db> {
+    fn from(predicate: StarImportPlaceholderPredicate<'db>) -> Self {
+        Predicate {
+            node: PredicateNode::StarImportPlaceholder(predicate),
+            is_positive: true,
+        }
+    }
+}