[red-knot] Fix subtle detail in where the `types.ModuleType` attribute lookup should happen in `TypeInferenceBuilder::infer_name_load()` (#16284)

crates/red_knot_python_semantic/src/symbol.rs

@@ -183,20 +183,34 @@ pub(crate) fn symbol<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> S
     symbol_impl(db, scope, name, RequiresExplicitReExport::No)
 }
 
-/// Infers the public type of a module-global symbol as seen from within the same file.
+/// Infers the public type of an explicit module-global symbol as seen from within the same file.
 ///
-/// If it's not defined explicitly in the global scope, it will look it up in `types.ModuleType`
+/// Note that all global scopes also include various "implicit globals" such as `__name__`,
-/// with a few very special exceptions.
+/// `__doc__` and `__file__`. This function **does not** consider those symbols; it will return
+/// `Symbol::Unbound` for them. Use the (currently test-only) `global_symbol` query to also include
+/// those additional symbols.
 ///
 /// Use [`imported_symbol`] to perform the lookup as seen from outside the file (e.g. via imports).
-pub(crate) fn global_symbol<'db>(db: &'db dyn Db, file: File, name: &str) -> Symbol<'db> {
+pub(crate) fn explicit_global_symbol<'db>(db: &'db dyn Db, file: File, name: &str) -> Symbol<'db> {
     symbol_impl(
         db,
         global_scope(db, file),
         name,
         RequiresExplicitReExport::No,
     )
-    .or_fall_back_to(db, || module_type_symbol(db, name))
+}
+
+/// Infers the public type of an explicit module-global symbol as seen from within the same file.
+///
+/// Unlike [`explicit_global_symbol`], this function also considers various "implicit globals"
+/// such as `__name__`, `__doc__` and `__file__`. These are looked up as attributes on `types.ModuleType`
+/// rather than being looked up as symbols explicitly defined/declared in the global scope.
+///
+/// Use [`imported_symbol`] to perform the lookup as seen from outside the file (e.g. via imports).
+#[cfg(test)]
+pub(crate) fn global_symbol<'db>(db: &'db dyn Db, file: File, name: &str) -> Symbol<'db> {
+    explicit_global_symbol(db, file, name)
+        .or_fall_back_to(db, || module_type_implicit_global_symbol(db, name))
 }
 
 /// Infers the public type of an imported symbol.
@@ -204,16 +218,16 @@ pub(crate) fn imported_symbol<'db>(db: &'db dyn Db, module: &Module, name: &str)
     // If it's not found in the global scope, check if it's present as an instance on
     // `types.ModuleType` or `builtins.object`.
     //
-    // We do a more limited version of this in `global_symbol`, but there are two crucial
+    // We do a more limited version of this in `module_type_implicit_global_symbol`,
-    // differences here:
+    // but there are two crucial differences here:
     // - If a member is looked up as an attribute, `__init__` is also available on the module, but
     //   it isn't available as a global from inside the module
     // - If a member is looked up as an attribute, members on `builtins.object` are also available
     //   (because `types.ModuleType` inherits from `object`); these attributes are also not
     //   available as globals from inside the module.
     //
-    // The same way as in `global_symbol`, however, we need to be careful to ignore
+    // The same way as in `module_type_implicit_global_symbol`, however, we need to be careful to
-    // `__getattr__`. Typeshed has a fake `__getattr__` on `types.ModuleType` to help out with
+    // 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, || {
@@ -239,7 +253,7 @@ pub(crate) fn builtins_symbol<'db>(db: &'db dyn Db, symbol: &str) -> Symbol<'db>
                 // We're looking up in the builtins namespace and not the module, so we should
                 // do the normal lookup in `types.ModuleType` and not the special one as in
                 // `imported_symbol`.
-                module_type_symbol(db, symbol)
+                module_type_implicit_global_symbol(db, symbol)
             })
         })
         .unwrap_or(Symbol::Unbound)
@@ -701,13 +715,13 @@ fn module_type_symbols<'db>(db: &'db dyn Db) -> smallvec::SmallVec<[ast::name::N
 /// [`member`] call on the instance type -- we'd just do the [`member`] call on the instance
 /// type, since it has the same end result. The reason to only call [`member`] on [`ModuleType`]
 /// instance when absolutely necessary is that it was a fairly significant performance regression
-/// to fallback to doing that for every name lookup that wasn't found in the module's globals
+/// to fallback to doing that for every name lookup that wasn't found in the module's globals.
-/// ([`global_symbol`]). So we use less idiomatic (and much more verbose) code here as a
+/// So we use less idiomatic (and much more verbose) code here as a micro-optimisation because
-/// micro-optimisation because it's used in a very hot path.
+/// it's used in a very hot path.
 ///
 /// [`member`]: Type::member
 /// [`ModuleType`]: KnownClass::ModuleType
-fn module_type_symbol<'db>(db: &'db dyn Db, name: &str) -> Symbol<'db> {
+pub(crate) fn module_type_implicit_global_symbol<'db>(db: &'db dyn Db, name: &str) -> Symbol<'db> {
     if module_type_symbols(db)
         .iter()
         .any(|module_type_member| &**module_type_member == name)
@@ -839,4 +853,37 @@ mod tests {
         let property_symbol_name = ast::name::Name::new_static("property");
         assert!(!symbol_names.contains(&property_symbol_name));
     }
+
+    #[track_caller]
+    fn assert_bound_string_symbol<'db>(db: &'db dyn Db, symbol: Symbol<'db>) {
+        assert!(matches!(
+            symbol,
+            Symbol::Type(Type::Instance(_), Boundness::Bound)
+        ));
+        assert_eq!(symbol.expect_type(), KnownClass::Str.to_instance(db));
+    }
+
+    #[test]
+    fn implicit_builtin_globals() {
+        let db = setup_db();
+        assert_bound_string_symbol(&db, builtins_symbol(&db, "__name__"));
+    }
+
+    #[test]
+    fn implicit_typing_globals() {
+        let db = setup_db();
+        assert_bound_string_symbol(&db, typing_symbol(&db, "__name__"));
+    }
+
+    #[test]
+    fn implicit_typing_extensions_globals() {
+        let db = setup_db();
+        assert_bound_string_symbol(&db, typing_extensions_symbol(&db, "__name__"));
+    }
+
+    #[test]
+    fn implicit_sys_globals() {
+        let db = setup_db();
+        assert_bound_string_symbol(&db, known_module_symbol(&db, KnownModule::Sys, "__name__"));
+    }
 }

crates/red_knot_python_semantic/src/types.rs

@@ -33,8 +33,8 @@ use crate::semantic_index::{
 };
 use crate::suppression::check_suppressions;
 use crate::symbol::{
-    global_symbol, imported_symbol, known_module_symbol, symbol, symbol_from_bindings,
+    imported_symbol, known_module_symbol, symbol, symbol_from_bindings, symbol_from_declarations,
-    symbol_from_declarations, Boundness, LookupError, LookupResult, Symbol, SymbolAndQualifiers,
+    Boundness, LookupError, LookupResult, Symbol, SymbolAndQualifiers,
 };
 use crate::types::call::{bind_call, CallArguments, CallBinding, CallOutcome};
 use crate::types::class_base::ClassBase;
@@ -5084,7 +5084,7 @@ static_assertions::assert_eq_size!(Type, [u8; 16]);
 pub(crate) mod tests {
     use super::*;
     use crate::db::tests::{setup_db, TestDbBuilder};
-    use crate::symbol::{typing_extensions_symbol, typing_symbol};
+    use crate::symbol::{global_symbol, typing_extensions_symbol, typing_symbol};
     use ruff_db::files::system_path_to_file;
     use ruff_db::parsed::parsed_module;
     use ruff_db::system::DbWithTestSystem;

crates/red_knot_python_semantic/src/types/infer.rs

@@ -50,7 +50,8 @@ use crate::semantic_index::semantic_index;
 use crate::semantic_index::symbol::{FileScopeId, NodeWithScopeKind, NodeWithScopeRef, ScopeId};
 use crate::semantic_index::SemanticIndex;
 use crate::symbol::{
-    builtins_module_scope, builtins_symbol, symbol, symbol_from_bindings, symbol_from_declarations,
+    builtins_module_scope, builtins_symbol, explicit_global_symbol,
+    module_type_implicit_global_symbol, symbol, symbol_from_bindings, symbol_from_declarations,
     typing_extensions_symbol, LookupError,
 };
 use crate::types::call::{Argument, CallArguments};
@@ -90,7 +91,7 @@ use super::slots::check_class_slots;
 use super::string_annotation::{
     parse_string_annotation, BYTE_STRING_TYPE_ANNOTATION, FSTRING_TYPE_ANNOTATION,
 };
-use super::{global_symbol, CallDunderError, ParameterExpectation, ParameterExpectations};
+use super::{CallDunderError, ParameterExpectation, ParameterExpectations};
 
 /// Infer all types for a [`ScopeId`], including all definitions and expressions in that scope.
 /// Use when checking a scope, or needing to provide a type for an arbitrary expression in the
@@ -3571,7 +3572,7 @@ impl<'db> TypeInferenceBuilder<'db> {
             }
 
             Symbol::Unbound
-                // No nonlocal binding? Check the module's globals.
+                // No nonlocal binding? Check the module's explicit globals.
                 // Avoid infinite recursion if `self.scope` already is the module's global scope.
                 .or_fall_back_to(db, || {
                     if file_scope_id.is_global() {
@@ -3588,8 +3589,12 @@ impl<'db> TypeInferenceBuilder<'db> {
                         }
                     }
 
-                    global_symbol(db, self.file(), symbol_name)
+                    explicit_global_symbol(db, self.file(), symbol_name)
                 })
+                // Not found in the module's explicitly declared global symbols?
+                // Check the "implicit globals" such as `__doc__`, `__file__`, `__name__`, etc.
+                // These are looked up as attributes on `types.ModuleType`.
+                .or_fall_back_to(db, || module_type_implicit_global_symbol(db, symbol_name))
                 // Not found in globals? Fallback to builtins
                 // (without infinite recursion if we're already in builtins.)
                 .or_fall_back_to(db, || {
@@ -6248,6 +6253,7 @@ mod tests {
     use crate::semantic_index::definition::Definition;
     use crate::semantic_index::symbol::FileScopeId;
     use crate::semantic_index::{global_scope, semantic_index, symbol_table, use_def_map};
+    use crate::symbol::global_symbol;
     use crate::types::check_types;
     use ruff_db::files::{system_path_to_file, File};
     use ruff_db::system::DbWithTestSystem;

crates/red_knot_python_semantic/src/types/signatures.rs

@@ -345,7 +345,8 @@ pub(crate) enum ParameterKind<'db> {
 mod tests {
     use super::*;
     use crate::db::tests::{setup_db, TestDb};
-    use crate::types::{global_symbol, FunctionType, KnownClass};
+    use crate::symbol::global_symbol;
+    use crate::types::{FunctionType, KnownClass};
     use ruff_db::system::DbWithTestSystem;
 
     #[track_caller]