address review

crates/red_knot_python_semantic/src/semantic_model.rs

@@ -8,7 +8,7 @@ use crate::module_name::ModuleName;
 use crate::module_resolver::{resolve_module, Module};
 use crate::semantic_index::ast_ids::HasScopedAstId;
 use crate::semantic_index::semantic_index;
-use crate::types::{definition_ty, global_symbol_ty_by_name, infer_scope_types, Type};
+use crate::types::{definition_ty, global_symbol_ty, infer_scope_types, Type};
 use crate::Db;
 
 pub struct SemanticModel<'db> {
@@ -40,7 +40,7 @@ impl<'db> SemanticModel<'db> {
     }
 
     pub fn global_symbol_ty(&self, module: &Module, symbol_name: &str) -> Type<'db> {
-        global_symbol_ty_by_name(self.db, module.file(), symbol_name)
+        global_symbol_ty(self.db, module.file(), symbol_name)
     }
 }
 

crates/red_knot_python_semantic/src/stdlib.rs

@@ -2,76 +2,73 @@ use crate::module_name::ModuleName;
 use crate::module_resolver::resolve_module;
 use crate::semantic_index::global_scope;
 use crate::semantic_index::symbol::ScopeId;
-use crate::types::{symbol_ty_by_name, Type};
+use crate::types::{global_symbol_ty, Type};
 use crate::Db;
 
 /// Enumeration of various core stdlib modules, for which we have dedicated Salsa queries.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub(crate) enum CoreStdlibModule {
+enum CoreStdlibModule {
     Builtins,
     Types,
     Typeshed,
 }
 
 impl CoreStdlibModule {
-    /// Retrieve the global scope of the given module.
-    ///
-    /// Returns `None` if the given module isn't available for some reason.
-    pub(crate) fn global_scope(self, db: &dyn Db) -> Option<ScopeId<'_>> {
-        match self {
-            Self::Builtins => builtins_scope(db),
-            Self::Types => types_scope(db),
-            Self::Typeshed => typeshed_scope(db),
-        }
+    fn name(self) -> ModuleName {
+        let module_name = match self {
+            Self::Builtins => "builtins",
+            Self::Types => "types",
+            Self::Typeshed => "_typeshed",
+        };
+        ModuleName::new_static(module_name)
+            .unwrap_or_else(|| panic!("{module_name} should be a valid module name!"))
     }
+}
 
-    /// Shorthand for `symbol_ty` that looks up a symbol in the scope of a given core module.
-    ///
-    /// Returns `Unbound` if the given module isn't available for some reason.
-    pub(crate) fn symbol_ty_by_name<'db>(self, db: &'db dyn Db, name: &str) -> Type<'db> {
-        self.global_scope(db)
-            .map(|globals| symbol_ty_by_name(db, globals, name))
-            .unwrap_or(Type::Unbound)
-    }
+/// Lookup the type of `symbol` in a given core module
+///
+/// Returns `Unbound` if the given core module cannot be resolved for some reason
+fn core_module_symbol_ty<'db>(
+    db: &'db dyn Db,
+    core_module: CoreStdlibModule,
+    symbol: &str,
+) -> Type<'db> {
+    resolve_module(db, core_module.name())
+        .map(|module| global_symbol_ty(db, module.file(), symbol))
+        .unwrap_or(Type::Unbound)
 }
 
-/// Shorthand for `symbol_ty` that looks up a symbol in the `builtins` scope.
+fn core_module_scope(db: &dyn Db, core_module: CoreStdlibModule) -> Option<ScopeId<'_>> {
+    resolve_module(db, core_module.name()).map(|module| global_scope(db, module.file()))
+}
+
+/// Lookup the type of `symbol` in the builtins namespace.
 ///
 /// Returns `Unbound` if the `builtins` module isn't available for some reason.
 #[inline]
-pub(crate) fn builtins_symbol_ty_by_name<'db>(db: &'db dyn Db, name: &str) -> Type<'db> {
-    CoreStdlibModule::Builtins.symbol_ty_by_name(db, name)
+pub(crate) fn builtins_symbol_ty<'db>(db: &'db dyn Db, symbol: &str) -> Type<'db> {
+    core_module_symbol_ty(db, CoreStdlibModule::Builtins, symbol)
 }
 
 /// Salsa query to get the builtins scope.
 ///
 /// Can return None if a custom typeshed is used that is missing `builtins.pyi`.
-#[salsa::tracked]
-pub(crate) fn builtins_scope(db: &dyn Db) -> Option<ScopeId<'_>> {
-    let builtins_name =
-        ModuleName::new_static("builtins").expect("Expected 'builtins' to be a valid module name");
-    let builtins_file = resolve_module(db, builtins_name)?.file();
-    Some(global_scope(db, builtins_file))
+pub(crate) fn builtins_module_scope(db: &dyn Db) -> Option<ScopeId<'_>> {
+    core_module_scope(db, CoreStdlibModule::Builtins)
 }
 
-/// Salsa query to get the scope for the `types` module.
+/// Lookup the type of `symbol` in the `types` module namespace.
 ///
-/// Can return None if a custom typeshed is used that is missing `types.pyi`.
-#[salsa::tracked]
-pub(crate) fn types_scope(db: &dyn Db) -> Option<ScopeId<'_>> {
-    let types_module_name =
-        ModuleName::new_static("types").expect("Expected 'types' to be a valid module name");
-    let types_file = resolve_module(db, types_module_name)?.file();
-    Some(global_scope(db, types_file))
+/// Returns `Unbound` if the `types` module isn't available for some reason.
+#[inline]
+pub(crate) fn types_symbol_ty<'db>(db: &'db dyn Db, symbol: &str) -> Type<'db> {
+    core_module_symbol_ty(db, CoreStdlibModule::Types, symbol)
 }
 
-/// Salsa query to get the scope for the `_typeshed` module.
+/// Lookup the type of `symbol` in the `_typeshed` module namespace.
 ///
-/// Can return None if a custom typeshed is used that is missing a `_typeshed` directory.
-#[salsa::tracked]
-pub(crate) fn typeshed_scope(db: &dyn Db) -> Option<ScopeId<'_>> {
-    let typeshed_module_name = ModuleName::new_static("_typeshed")
-        .expect("Expected '_typeshed' to be a valid module name");
-    let typeshed_file = resolve_module(db, typeshed_module_name)?.file();
-    Some(global_scope(db, typeshed_file))
+/// Returns `Unbound` if the `_typeshed` module isn't available for some reason.
+#[inline]
+pub(crate) fn typeshed_symbol_ty<'db>(db: &'db dyn Db, symbol: &str) -> Type<'db> {
+    core_module_symbol_ty(db, CoreStdlibModule::Typeshed, symbol)
 }

crates/red_knot_python_semantic/src/types.rs

@@ -8,7 +8,7 @@ use crate::semantic_index::{
     global_scope, semantic_index, symbol_table, use_def_map, DefinitionWithConstraints,
     DefinitionWithConstraintsIterator,
 };
-use crate::stdlib::{builtins_symbol_ty_by_name, CoreStdlibModule};
+use crate::stdlib::{builtins_symbol_ty, types_symbol_ty, typeshed_symbol_ty};
 use crate::types::narrow::narrowing_constraint;
 use crate::{Db, FxOrderSet};
 
@@ -40,7 +40,7 @@ pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics {
 }
 
 /// Infer the public type of a symbol (its type as seen from outside its scope).
-pub(crate) fn symbol_ty<'db>(
+pub(crate) fn symbol_ty_by_id<'db>(
     db: &'db dyn Db,
     scope: ScopeId<'db>,
     symbol: ScopedSymbolId,
@@ -58,21 +58,17 @@ pub(crate) fn symbol_ty<'db>(
 }
 
 /// Shorthand for `symbol_ty` that takes a symbol name instead of an ID.
-pub(crate) fn symbol_ty_by_name<'db>(
-    db: &'db dyn Db,
-    scope: ScopeId<'db>,
-    name: &str,
-) -> Type<'db> {
+pub(crate) fn symbol_ty<'db>(db: &'db dyn Db, scope: ScopeId<'db>, name: &str) -> Type<'db> {
     let table = symbol_table(db, scope);
     table
         .symbol_id_by_name(name)
-        .map(|symbol| symbol_ty(db, scope, symbol))
+        .map(|symbol| symbol_ty_by_id(db, scope, symbol))
         .unwrap_or(Type::Unbound)
 }
 
 /// Shorthand for `symbol_ty` that looks up a module-global symbol by name in a file.
-pub(crate) fn global_symbol_ty_by_name<'db>(db: &'db dyn Db, file: File, name: &str) -> Type<'db> {
-    symbol_ty_by_name(db, global_scope(db, file), name)
+pub(crate) fn global_symbol_ty<'db>(db: &'db dyn Db, file: File, name: &str) -> Type<'db> {
+    symbol_ty(db, global_scope(db, file), name)
 }
 
 /// Infer the type of a [`Definition`].
@@ -335,7 +331,7 @@ impl<'db> Type<'db> {
                 // TODO: attribute lookup on function type
                 Type::Unknown
             }
-            Type::Module(file) => global_symbol_ty_by_name(db, *file, name),
+            Type::Module(file) => global_symbol_ty(db, *file, name),
             Type::Class(class) => class.class_member(db, name),
             Type::Instance(_) => {
                 // TODO MRO? get_own_instance_member, get_instance_member
@@ -409,12 +405,12 @@ impl<'db> Type<'db> {
     fn iterate(&self, db: &'db dyn Db) -> Option<Type<'db>> {
         // `self` represents the type of the iterable;
         // `__iter__` and `__next__` are both looked up on the class of the iterable:
-        let type_of_class = self.to_type_of_class(db);
+        let type_of_class = self.to_meta_type(db);
 
         let dunder_iter_method = type_of_class.member(db, "__iter__");
         if !dunder_iter_method.is_unbound() {
             let iterator_ty = dunder_iter_method.call(db)?;
-            let dunder_next_method = iterator_ty.to_type_of_class(db).member(db, "__next__");
+            let dunder_next_method = iterator_ty.to_meta_type(db).member(db, "__next__");
             return dunder_next_method.call(db);
         }
 
@@ -441,22 +437,22 @@ impl<'db> Type<'db> {
     /// Given a type that is assumed to represent an instance of a class,
     /// return a type that represents that class itself.
     #[must_use]
-    pub fn to_type_of_class(&self, db: &'db dyn Db) -> Type<'db> {
+    pub fn to_meta_type(&self, db: &'db dyn Db) -> Type<'db> {
         match self {
             Type::Unbound => Type::Unbound,
             Type::Never => Type::Never,
             Type::Instance(class) => Type::Class(*class),
-            Type::Union(union) => union.map(db, |ty| ty.to_type_of_class(db)),
-            Type::BooleanLiteral(_) => builtins_symbol_ty_by_name(db, "bool"),
-            Type::BytesLiteral(_) => builtins_symbol_ty_by_name(db, "bytes"),
-            Type::IntLiteral(_) => builtins_symbol_ty_by_name(db, "int"),
-            Type::Function(_) => CoreStdlibModule::Types.symbol_ty_by_name(db, "FunctionType"),
-            Type::Module(_) => CoreStdlibModule::Types.symbol_ty_by_name(db, "ModuleType"),
-            Type::None => CoreStdlibModule::Typeshed.symbol_ty_by_name(db, "NoneType"),
+            Type::Union(union) => union.map(db, |ty| ty.to_meta_type(db)),
+            Type::BooleanLiteral(_) => builtins_symbol_ty(db, "bool"),
+            Type::BytesLiteral(_) => builtins_symbol_ty(db, "bytes"),
+            Type::IntLiteral(_) => builtins_symbol_ty(db, "int"),
+            Type::Function(_) => types_symbol_ty(db, "FunctionType"),
+            Type::Module(_) => types_symbol_ty(db, "ModuleType"),
+            Type::None => typeshed_symbol_ty(db, "NoneType"),
             // TODO not accurate if there's a custom metaclass...
-            Type::Class(_) => builtins_symbol_ty_by_name(db, "type"),
+            Type::Class(_) => builtins_symbol_ty(db, "type"),
             // TODO can we do better here? `type[LiteralString]`?
-            Type::StringLiteral(_) | Type::LiteralString => builtins_symbol_ty_by_name(db, "str"),
+            Type::StringLiteral(_) | Type::LiteralString => builtins_symbol_ty(db, "str"),
             // TODO: `type[Any]`?
             Type::Any => Type::Any,
             // TODO: `type[Unknown]`?
@@ -557,7 +553,7 @@ impl<'db> ClassType<'db> {
     /// Returns the inferred type of the class member named `name`.
     pub fn own_class_member(self, db: &'db dyn Db, name: &str) -> Type<'db> {
         let scope = self.body_scope(db);
-        symbol_ty_by_name(db, scope, name)
+        symbol_ty(db, scope, name)
     }
 
     pub fn inherited_class_member(self, db: &'db dyn Db, name: &str) -> Type<'db> {

crates/red_knot_python_semantic/src/types/builder.rs

@@ -25,7 +25,7 @@
 //!   * No type in an intersection can be a supertype of any other type in the intersection (just
 //!     eliminate the supertype from the intersection).
 //!   * An intersection containing two non-overlapping types should simplify to [`Type::Never`].
-use crate::types::{builtins_symbol_ty_by_name, IntersectionType, Type, UnionType};
+use crate::types::{builtins_symbol_ty, IntersectionType, Type, UnionType};
 use crate::{Db, FxOrderSet};
 use ordermap::set::MutableValues;
 
@@ -65,7 +65,7 @@ impl<'db> UnionBuilder<'db> {
         if let Some(true_index) = self.elements.get_index_of(&Type::BooleanLiteral(true)) {
             if self.elements.contains(&Type::BooleanLiteral(false)) {
                 *self.elements.get_index_mut2(true_index).unwrap() =
-                    builtins_symbol_ty_by_name(self.db, "bool");
+                    builtins_symbol_ty(self.db, "bool");
                 self.elements.remove(&Type::BooleanLiteral(false));
             }
         }
@@ -275,7 +275,7 @@ mod tests {
     use crate::db::tests::TestDb;
     use crate::program::{Program, SearchPathSettings};
     use crate::python_version::PythonVersion;
-    use crate::types::builtins_symbol_ty_by_name;
+    use crate::types::builtins_symbol_ty;
     use crate::ProgramSettings;
     use ruff_db::system::{DbWithTestSystem, SystemPathBuf};
 
@@ -348,7 +348,7 @@ mod tests {
     #[test]
     fn build_union_bool() {
         let db = setup_db();
-        let bool_ty = builtins_symbol_ty_by_name(&db, "bool");
+        let bool_ty = builtins_symbol_ty(&db, "bool");
 
         let t0 = Type::BooleanLiteral(true);
         let t1 = Type::BooleanLiteral(true);

crates/red_knot_python_semantic/src/types/display.rs

@@ -236,9 +236,7 @@ mod tests {
     use ruff_db::system::{DbWithTestSystem, SystemPathBuf};
 
     use crate::db::tests::TestDb;
-    use crate::types::{
-        global_symbol_ty_by_name, BytesLiteralType, StringLiteralType, Type, UnionBuilder,
-    };
+    use crate::types::{global_symbol_ty, BytesLiteralType, StringLiteralType, Type, UnionBuilder};
     use crate::{Program, ProgramSettings, PythonVersion, SearchPathSettings};
 
     fn setup_db() -> TestDb {
@@ -283,16 +281,16 @@ mod tests {
         let vec: Vec<Type<'_>> = vec![
             Type::Unknown,
             Type::IntLiteral(-1),
-            global_symbol_ty_by_name(&db, mod_file, "A"),
+            global_symbol_ty(&db, mod_file, "A"),
             Type::StringLiteral(StringLiteralType::new(&db, Box::from("A"))),
             Type::BytesLiteral(BytesLiteralType::new(&db, Box::from([0]))),
             Type::BytesLiteral(BytesLiteralType::new(&db, Box::from([7]))),
             Type::IntLiteral(0),
             Type::IntLiteral(1),
             Type::StringLiteral(StringLiteralType::new(&db, Box::from("B"))),
-            global_symbol_ty_by_name(&db, mod_file, "foo"),
-            global_symbol_ty_by_name(&db, mod_file, "bar"),
-            global_symbol_ty_by_name(&db, mod_file, "B"),
+            global_symbol_ty(&db, mod_file, "foo"),
+            global_symbol_ty(&db, mod_file, "bar"),
+            global_symbol_ty(&db, mod_file, "B"),
             Type::BooleanLiteral(true),
             Type::None,
         ];