diff --git a/crates/red_knot_python_semantic/resources/mdtest/import/star.md b/crates/red_knot_python_semantic/resources/mdtest/import/star.md index 857eccb063..99aa39e74e 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/import/star.md +++ b/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 ``` diff --git a/crates/red_knot_python_semantic/resources/mdtest/properties.md b/crates/red_knot_python_semantic/resources/mdtest/properties.md index de16979c66..dd0473b44f 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/properties.md +++ b/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: reveal_type(type(attr_property).__set__) # revealed: ``` @@ -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 diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index 41d37c0991..2eba5016af 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/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 : + // 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; diff --git a/crates/red_knot_python_semantic/src/semantic_index/predicate.rs b/crates/red_knot_python_semantic/src/semantic_index/predicate.rs index 9b1dc0c035..c2885022e8 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/predicate.rs +++ b/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 : +/// class A: ... +/// ``` +/// +/// and we have a module `importer.py` like so: +/// +/// ```py +/// A = 1 +/// +/// from importer import * +/// ``` +/// +/// Since we cannot know whether or not 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 : +/// 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> for Predicate<'db> { + fn from(predicate: StarImportPlaceholderPredicate<'db>) -> Self { + Predicate { + node: PredicateNode::StarImportPlaceholder(predicate), + is_positive: true, + } + } +} diff --git a/crates/red_knot_python_semantic/src/semantic_index/visibility_constraints.rs b/crates/red_knot_python_semantic/src/semantic_index/visibility_constraints.rs index 2a428d4cdf..84337680ab 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/visibility_constraints.rs +++ b/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, + } + } } } } diff --git a/crates/red_knot_python_semantic/src/symbol.rs b/crates/red_knot_python_semantic/src/symbol.rs index 08c100486e..24f2f5d77b 100644 --- a/crates/red_knot_python_semantic/src/symbol.rs +++ b/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() } diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index f7b238fd75..e04b4bf744 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/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 } } diff --git a/crates/red_knot_python_semantic/src/types/narrow.rs b/crates/red_knot_python_semantic/src/types/narrow.rs index 05b4c22462..4ba8e4845f 100644 --- a/crates/red_knot_python_semantic/src/types/narrow.rs +++ b/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), } }