[red-knot] Improve handling of visibility constraints in external modules when resolving * imports (astral-sh#17286)

Author: AlexWaygood

crates/red_knot_python_semantic/resources/mdtest/import/star.md

@@ -227,23 +227,23 @@ print((
     D,
     E,
     F,
-    G,  # TODO: could emit diagnostic about being possibly unbound
-    H,
+    G,  # error: [possibly-unresolved-reference]
+    H,  # error: [possibly-unresolved-reference]
     I,
     J,
     K,
     L,
-    M,  # TODO: could emit diagnostic about being possibly unbound
-    N,  # TODO: could emit diagnostic about being possibly unbound
-    O,  # TODO: could emit diagnostic about being possibly unbound
-    P,  # TODO: could emit diagnostic about being possibly unbound
-    Q,  # TODO: could emit diagnostic about being possibly unbound
-    R,  # TODO: could emit diagnostic about being possibly unbound
-    S,  # TODO: could emit diagnostic about being possibly unbound
-    T,  # TODO: could emit diagnostic about being possibly unbound
-    U,  # TODO: could emit diagnostic about being possibly unbound
-    V,  # TODO: could emit diagnostic about being possibly unbound
-    W,  # TODO: could emit diagnostic about being possibly unbound
+    M,  # error: [possibly-unresolved-reference]
+    N,  # error: [possibly-unresolved-reference]
+    O,  # error: [possibly-unresolved-reference]
+    P,  # error: [possibly-unresolved-reference]
+    Q,  # error: [possibly-unresolved-reference]
+    R,  # error: [possibly-unresolved-reference]
+    S,  # error: [possibly-unresolved-reference]
+    T,  # error: [possibly-unresolved-reference]
+    U,  # TODO: could emit [possibly-unresolved-reference here] (https://github.com/astral-sh/ruff/issues/16996)
+    V,  # error: [possibly-unresolved-reference]
+    W,  # error: [possibly-unresolved-reference]
     typing,
     OrderedDict,
     Foo,
@@ -479,15 +479,21 @@ reveal_type(s)  # revealed: Unknown
 # error: [unresolved-reference]
 reveal_type(t)  # revealed: Unknown
 
-# TODO: these should all reveal `Unknown | int`.
+# TODO: these should all reveal `Unknown | int` and should not emit errors.
 # (We don't generally model elsewhere in red-knot that bindings from walruses
 # "leak" from comprehension scopes into outer scopes, but we should.)
 # See https://github.com/astral-sh/ruff/issues/16954
+# error: [unresolved-reference]
 reveal_type(g)  # revealed: Unknown
+# error: [unresolved-reference]
 reveal_type(i)  # revealed: Unknown
+# error: [unresolved-reference]
 reveal_type(k)  # revealed: Unknown
+# error: [unresolved-reference]
 reveal_type(m)  # revealed: Unknown
+# error: [unresolved-reference]
 reveal_type(o)  # revealed: Unknown
+# error: [unresolved-reference]
 reveal_type(q)  # revealed: Unknown
 ```
 
@@ -668,15 +674,15 @@ from exporter import *
 
 reveal_type(X)  # revealed: bool
 
-# TODO: should emit error: [unresolved-reference]
+# error: [unresolved-reference]
 reveal_type(Y)  # revealed: Unknown
 
-# TODO: The `*` import should not be considered a redefinition
-# of the global variable in this module, as the symbol in
+# The `*` import is not considered a redefinition
+# of the global variable `Z` in this module, as the symbol in
 # the `a` module is in a branch that is statically known
 # to be dead code given the `python-version` configuration.
-# Thus this should reveal `Literal[True]`.
-reveal_type(Z)  # revealed: Unknown
+# Thus this still reveals `Literal[True]`.
+reveal_type(Z)  # revealed: Literal[True]
 ```
 
 ### Multiple `*` imports with always-false visibility constraints
@@ -707,8 +713,7 @@ from exporter import *
 from exporter import *
 from exporter import *
 
-# TODO: should still be `Literal[True]
-reveal_type(Z)  # revealed: Unknown
+reveal_type(Z)  # revealed: Literal[True]
 ```
 
 ### Ambiguous visibility constraints
@@ -735,7 +740,7 @@ else:
 ```py
 from exporter import *
 
-# TODO: should have a `[possibly-unresolved-reference]` diagnostic
+# error: [possibly-unresolved-reference]
 reveal_type(A)  # revealed: Unknown | Literal[1]
 
 reveal_type(B)  # revealed: Unknown | Literal[2, 3]
@@ -798,16 +803,14 @@ if sys.version_info >= (3, 12):
 else:
     from exporter import *
 
-    # TODO: should have an `[unresolved-reference]` diagnostic
+    # error: [unresolved-reference]
     reveal_type(A)  # revealed: Unknown
-
-    # TODO: should have a `[possibly-unresolved-reference]` diagnostic
+    # error: [possibly-unresolved-reference]
     reveal_type(B)  # revealed: bool
 
-# TODO: should have an `[unresolved-reference]` diagnostic
+# error: [unresolved-reference]
 reveal_type(A)  # revealed: Unknown
-
-# TODO: should have a `[possibly-unresolved-reference]` diagnostic
+# error: [possibly-unresolved-reference]
 reveal_type(B)  # revealed: bool
 ```
 
@@ -1065,7 +1068,7 @@ from exporter import *
 reveal_type(X)  # revealed: bool
 reveal_type(Y)  # revealed: bool
 
-# TODO: should error with [unresolved-reference]
+# error: [unresolved-reference]
 reveal_type(Z)  # revealed: Unknown
 ```
 
@@ -1100,7 +1103,7 @@ from exporter import *
 reveal_type(X)  # revealed: bool
 reveal_type(Y)  # revealed: bool
 
-# TODO should have an [unresolved-reference] diagnostic
+# error: [unresolved-reference]
 reveal_type(Z)  # revealed: Unknown
 ```
 

crates/red_knot_python_semantic/resources/mdtest/properties.md

@@ -196,10 +196,14 @@ reveal_type(c.attr)  # revealed: Unknown
 
 ## Behind the scenes
 
+> TODO: This test is currently disabled pending
+> [an upstream Salsa fix](https://github.com/salsa-rs/salsa/pull/741). Once that has been merged,
+> re-enable this test by changing the language codes below back to `py`.
+
 In this section, we trace through some of the steps that make properties work. We start with a
 simple class `C` and a property `attr`:
 
-```py
+```ignore
 class C:
     def __init__(self):
         self._attr: int = 0
@@ -216,7 +220,7 @@ class C:
 Next, we create an instance of `C`. As we have seen above, accessing `attr` on the instance will
 return an `int`:
 
-```py
+```ignore
 c = C()
 
 reveal_type(c.attr)  # revealed: int
@@ -226,7 +230,7 @@ Behind the scenes, when we write `c.attr`, the first thing that happens is that
 up the symbol `attr` on the meta-type of `c`, i.e. the class `C`. We can emulate this static lookup
 using `inspect.getattr_static`, to see that `attr` is actually an instance of the `property` class:
 
-```py
+```ignore
 from inspect import getattr_static
 
 attr_property = getattr_static(C, "attr")
@@ -237,7 +241,7 @@ The `property` class has a `__get__` method, which makes it a descriptor. It als
 method, which means that it is a *data* descriptor (if there is no setter, `__set__` is still
 available but yields an `AttributeError` at runtime).
 
-```py
+```ignore
 reveal_type(type(attr_property).__get__)  # revealed: <wrapper-descriptor `__get__` of `property` objects>
 reveal_type(type(attr_property).__set__)  # revealed: <wrapper-descriptor `__set__` of `property` objects>
 ```
@@ -246,22 +250,22 @@ When we access `c.attr`, the `__get__` method of the `property` class is called,
 property object itself as the first argument, and the class instance `c` as the second argument. The
 third argument is the "owner" which can be set to `None` or to `C` in this case:
 
-```py
+```ignore
 reveal_type(type(attr_property).__get__(attr_property, c, C))  # revealed: int
 reveal_type(type(attr_property).__get__(attr_property, c, None))  # revealed: int
 ```
 
 Alternatively, the above can also be written as a method call:
 
-```py
+```ignore
 reveal_type(attr_property.__get__(c, C))  # revealed: int
 ```
 
 When we access `attr` on the class itself, the descriptor protocol is also invoked, but the instance
 argument is set to `None`. When `instance` is `None`, the call to `property.__get__` returns the
 property instance itself. So the following expressions are all equivalent
 
-```py
+```ignore
 reveal_type(attr_property)  # revealed: property
 reveal_type(C.attr)  # revealed: property
 reveal_type(attr_property.__get__(None, C))  # revealed: property
@@ -271,7 +275,7 @@ reveal_type(type(attr_property).__get__(attr_property, None, C))  # revealed: pr
 When we set the property using `c.attr = "a"`, the `__set__` method of the property class is called.
 This attribute access desugars to
 
-```py
+```ignore
 type(attr_property).__set__(attr_property, c, "a")
 
 # error: [call-non-callable] "Call of wrapper descriptor `property.__set__` failed: calling the setter failed"
@@ -280,7 +284,7 @@ type(attr_property).__set__(attr_property, c, 1)
 
 which is also equivalent to the following expressions:
 
-```py
+```ignore
 attr_property.__set__(c, "a")
 # error: [call-non-callable]
 attr_property.__set__(c, 1)
@@ -293,7 +297,7 @@ C.attr.__set__(c, 1)
 Properties also have `fget` and `fset` attributes that can be used to retrieve the original getter
 and setter functions, respectively.
 
-```py
+```ignore
 reveal_type(attr_property.fget)  # revealed: Literal[attr]
 reveal_type(attr_property.fget(c))  # revealed: int
 

crates/red_knot_python_semantic/src/semantic_index/builder.rs

@@ -25,6 +25,7 @@ use crate::semantic_index::definition::{
 use crate::semantic_index::expression::{Expression, ExpressionKind};
 use crate::semantic_index::predicate::{
     PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, ScopedPredicateId,
+    StarImportPlaceholderPredicate,
 };
 use crate::semantic_index::re_exports::exported_names;
 use crate::semantic_index::symbol::{
@@ -1182,10 +1183,40 @@ where
                             continue;
                         };
 
-                        for export in exported_names(self.db, module.file()) {
+                        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 };
+                            let star_import = StarImportPlaceholderPredicate::new(
+                                self.db,
+                                self.file,
+                                symbol_id,
+                                referenced_module,
+                            );
+                            let pre_definition = self.flow_snapshot();
                             self.push_additional_definition(symbol_id, node_ref);
+                            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);
+                            self.flow_merge(post_definition);
+                            self.simplify_visibility_constraints(pre_definition);
                         }
 
                         continue;

crates/red_knot_python_semantic/src/semantic_index/predicate.rs

@@ -13,7 +13,8 @@ use ruff_python_ast::Singleton;
 
 use crate::db::Db;
 use crate::semantic_index::expression::Expression;
-use crate::semantic_index::symbol::{FileScopeId, ScopeId};
+use crate::semantic_index::global_scope;
+use crate::semantic_index::symbol::{FileScopeId, ScopeId, ScopedSymbolId};
 
 // A scoped identifier for each `Predicate` in a scope.
 #[newtype_index]
@@ -52,6 +53,7 @@ pub(crate) struct Predicate<'db> {
 pub(crate) enum PredicateNode<'db> {
     Expression(Expression<'db>),
     Pattern(PatternPredicate<'db>),
+    StarImportPlaceholder(StarImportPlaceholderPredicate<'db>),
 }
 
 /// Pattern kinds for which we support type narrowing and/or static visibility analysis.
@@ -85,3 +87,78 @@ impl<'db> PatternPredicate<'db> {
         self.file_scope(db).to_scope_id(db, self.file(db))
     }
 }
+
+/// A "placeholder predicate" that is used to model the fact that the boundness of a
+/// (possible) definition or declaration caused by a `*` import cannot be fully determined
+/// 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 `exporter.py` like so:
+///
+/// ```py
+/// if <condition>:
+///     class A: ...
+/// ```
+///
+/// and we have a module `importer.py` like so:
+///
+/// ```py
+/// A = 1
+///
+/// from importer import *
+/// ```
+///
+/// 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.
+/// Essentially, we model `b.py` as something similar to this:
+///
+/// ```py
+/// A = 1
+///
+/// if <star_import_placeholder_predicate>:
+///     from a import A
+/// ```
+///
+/// At type-check time, the placeholder predicate for the `A` definition is evaluated by
+/// attempting to resolve the `A` symbol in `a.py`'s global namespace:
+/// - If it resolves to a definitely bound symbol, then the predicate resolves to [`Truthiness::AlwaysTrue`]
+/// - If it resolves to an unbound symbol, then the predicate resolves to [`Truthiness::AlwaysFalse`]
+/// - If it resolves to a possibly bound symbol, then the predicate resolves to [`Truthiness::Ambiguous`]
+///
+/// [Truthiness]: [crate::types::Truthiness]
+#[salsa::tracked(debug)]
+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 of the importing file.
+    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> {
+        // 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,
+        }
+    }
+}