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

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
+    G,  # error: [possibly-unresolved-reference]
-    H,
+    H,  # error: [possibly-unresolved-reference]
     I,
     J,
     K,
     L,
-    M,  # TODO: could emit diagnostic about being possibly unbound
+    M,  # error: [possibly-unresolved-reference]
-    N,  # TODO: could emit diagnostic about being possibly unbound
+    N,  # error: [possibly-unresolved-reference]
-    O,  # TODO: could emit diagnostic about being possibly unbound
+    O,  # error: [possibly-unresolved-reference]
-    P,  # TODO: could emit diagnostic about being possibly unbound
+    P,  # error: [possibly-unresolved-reference]
-    Q,  # TODO: could emit diagnostic about being possibly unbound
+    Q,  # error: [possibly-unresolved-reference]
-    R,  # TODO: could emit diagnostic about being possibly unbound
+    R,  # error: [possibly-unresolved-reference]
-    S,  # TODO: could emit diagnostic about being possibly unbound
+    S,  # error: [possibly-unresolved-reference]
-    T,  # TODO: could emit diagnostic about being possibly unbound
+    T,  # error: [possibly-unresolved-reference]
-    U,  # TODO: could emit diagnostic about being possibly unbound
+    U,  # TODO: could emit [possibly-unresolved-reference here] (https://github.com/astral-sh/ruff/issues/16996)
-    V,  # TODO: could emit diagnostic about being possibly unbound
+    V,  # error: [possibly-unresolved-reference]
-    W,  # TODO: could emit diagnostic about being possibly unbound
+    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
+# The `*` import is not considered a redefinition
-# of the global variable in this module, as the symbol in
+# 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]`.
+# Thus this still reveals `Literal[True]`.
-reveal_type(Z)  # revealed: Unknown
+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: Literal[True]
-reveal_type(Z)  # revealed: Unknown
 ```
 
 ### 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
-
+    # error: [possibly-unresolved-reference]
-    # TODO: should have a `[possibly-unresolved-reference]` diagnostic
     reveal_type(B)  # revealed: bool
 
-# TODO: should have an `[unresolved-reference]` diagnostic
+# error: [unresolved-reference]
 reveal_type(A)  # revealed: Unknown
-
+# error: [possibly-unresolved-reference]
-# TODO: should have a `[possibly-unresolved-reference]` diagnostic
 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,14 +250,14 @@ 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
 ```
 
@@ -261,7 +265,7 @@ When we access `attr` on the class itself, the descriptor protocol is also invok
 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,
+        }
+    }
+}

crates/red_knot_python_semantic/src/semantic_index/visibility_constraints.rs

@@ -182,6 +182,8 @@ use crate::semantic_index::expression::Expression;
 use crate::semantic_index::predicate::{
     PatternPredicate, PatternPredicateKind, Predicate, PredicateNode, Predicates, ScopedPredicateId,
 };
+use crate::semantic_index::symbol_table;
+use crate::symbol::imported_symbol;
 use crate::types::{infer_expression_type, Truthiness, Type};
 use crate::Db;
 
@@ -650,6 +652,19 @@ impl VisibilityConstraints {
                 ty.bool(db).negate_if(!predicate.is_positive)
             }
             PredicateNode::Pattern(inner) => Self::analyze_single_pattern_predicate(db, inner),
+            PredicateNode::StarImportPlaceholder(star_import) => {
+                let symbol_table = symbol_table(db, star_import.scope(db));
+                let symbol_name = symbol_table.symbol(star_import.symbol_id(db)).name();
+                match imported_symbol(db, star_import.referenced_file(db), symbol_name).symbol {
+                    crate::symbol::Symbol::Type(_, crate::symbol::Boundness::Bound) => {
+                        Truthiness::AlwaysTrue
+                    }
+                    crate::symbol::Symbol::Type(_, crate::symbol::Boundness::PossiblyUnbound) => {
+                        Truthiness::Ambiguous
+                    }
+                    crate::symbol::Symbol::Unbound => Truthiness::AlwaysFalse,
+                }
+            }
         }
     }
 }

crates/red_knot_python_semantic/src/symbol.rs

@@ -11,7 +11,7 @@ use crate::types::{
     binding_type, declaration_type, infer_narrowing_constraint, todo_type, IntersectionBuilder,
     KnownClass, Truthiness, Type, TypeAndQualifiers, TypeQualifiers, UnionBuilder, UnionType,
 };
-use crate::{resolve_module, Db, KnownModule, Module, Program};
+use crate::{resolve_module, Db, KnownModule, Program};
 
 pub(crate) use implicit_globals::module_type_implicit_global_symbol;
 
@@ -255,7 +255,7 @@ pub(crate) fn global_symbol<'db>(
 /// Infers the public type of an imported symbol.
 pub(crate) fn imported_symbol<'db>(
     db: &'db dyn Db,
-    module: &Module,
+    file: File,
     name: &str,
 ) -> SymbolAndQualifiers<'db> {
     // If it's not found in the global scope, check if it's present as an instance on
@@ -273,7 +273,7 @@ pub(crate) fn imported_symbol<'db>(
     // ignore `__getattr__`. Typeshed has a fake `__getattr__` on `types.ModuleType` to help out with
     // dynamic imports; we shouldn't use it for `ModuleLiteral` types where we know exactly which
     // module we're dealing with.
-    external_symbol_impl(db, module.file(), name).or_fall_back_to(db, || {
+    external_symbol_impl(db, file, name).or_fall_back_to(db, || {
         if name == "__getattr__" {
             Symbol::Unbound.into()
         } else {
@@ -311,7 +311,7 @@ pub(crate) fn known_module_symbol<'db>(
     symbol: &str,
 ) -> SymbolAndQualifiers<'db> {
     resolve_module(db, &known_module.name())
-        .map(|module| imported_symbol(db, &module, symbol))
+        .map(|module| imported_symbol(db, module.file(), symbol))
         .unwrap_or_default()
 }
 

crates/red_knot_python_semantic/src/types.rs

@@ -5973,7 +5973,7 @@ impl<'db> ModuleLiteralType<'db> {
             }
         }
 
-        imported_symbol(db, &self.module(db), name).symbol
+        imported_symbol(db, self.module(db).file(), name).symbol
     }
 }
 

crates/red_knot_python_semantic/src/types/narrow.rs

@@ -51,6 +51,7 @@ pub(crate) fn infer_narrowing_constraint<'db>(
             }
         }
         PredicateNode::Pattern(pattern) => all_narrowing_constraints_for_pattern(db, pattern),
+        PredicateNode::StarImportPlaceholder(_) => return None,
     };
     if let Some(constraints) = constraints {
         constraints.get(&definition.symbol(db)).copied()
@@ -237,6 +238,7 @@ impl<'db> NarrowingConstraintsBuilder<'db> {
                 self.evaluate_expression_predicate(expression, self.is_positive)
             }
             PredicateNode::Pattern(pattern) => self.evaluate_pattern_predicate(pattern),
+            PredicateNode::StarImportPlaceholder(_) => return None,
         };
         if let Some(mut constraints) = constraints {
             constraints.shrink_to_fit();
@@ -312,6 +314,7 @@ impl<'db> NarrowingConstraintsBuilder<'db> {
         match self.predicate {
             PredicateNode::Expression(expression) => expression.scope(self.db),
             PredicateNode::Pattern(pattern) => pattern.scope(self.db),
+            PredicateNode::StarImportPlaceholder(definition) => definition.scope(self.db),
         }
     }