address review

Author: AlexWaygood

crates/red_knot_project/tests/check.rs

@@ -1,12 +1,15 @@
 use anyhow::{anyhow, Context};
 use red_knot_project::{ProjectDatabase, ProjectMetadata};
-use red_knot_python_semantic::{HasType, SemanticModel};
+use red_knot_python_semantic::{HasType, Program, SemanticModel};
 use ruff_db::files::{system_path_to_file, File};
 use ruff_db::parsed::parsed_module;
 use ruff_db::system::{SystemPath, SystemPathBuf, TestSystem};
 use ruff_python_ast::visitor::source_order;
 use ruff_python_ast::visitor::source_order::SourceOrderVisitor;
-use ruff_python_ast::{self as ast, Alias, Expr, Parameter, ParameterWithDefault, Stmt};
+use ruff_python_ast::{
+    self as ast, Alias, Expr, Parameter, ParameterWithDefault, PythonVersion, Stmt,
+};
+use salsa::Setter;
 
 fn setup_db(project_root: &SystemPath, system: TestSystem) -> anyhow::Result<ProjectDatabase> {
     let project = ProjectMetadata::discover(project_root, &system)?;
@@ -84,6 +87,10 @@ fn run_corpus_tests(pattern: &str) -> anyhow::Result<()> {
 
     let mut db = setup_db(&root, system.clone())?;
 
+    Program::get(&db)
+        .set_python_version(&mut db)
+        .to(PythonVersion::latest());
+
     let workspace_root = get_cargo_workspace_root()?;
     let workspace_root = workspace_root.to_string();
 

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

@@ -32,11 +32,18 @@ typeshed = "/typeshed"
 `/typeshed/stdlib/builtins.pyi`:
 
 ```pyi
+class object: ...
 class Custom: ...
 
 custom_builtin: Custom
 ```
 
+`/typeshed/stdlib/types.pyi`:
+
+```pyi
+class ModuleType: ...
+```
+
 `/typeshed/stdlib/typing_extensions.pyi`:
 
 ```pyi
@@ -67,6 +74,12 @@ foo = bar
 bar = 1
 ```
 
+`/typeshed/stdlib/types.pyi`:
+
+```pyi
+class ModuleType: ...
+```
+
 `/typeshed/stdlib/typing_extensions.pyi`:
 
 ```pyi

crates/red_knot_python_semantic/resources/mdtest/mdtest_custom_typeshed.md

@@ -22,11 +22,21 @@ We can then place custom stub files in `/typeshed/stdlib`, for example:
 `/typeshed/stdlib/builtins.pyi`:
 
 ```pyi
+class object: ...
+class type: ...
+class ellipsis: ...
+
 class BuiltinClass: ...
 
 builtin_symbol: BuiltinClass
 ```
 
+`/typeshed/stdlib/types.pyi`:
+
+```pyi
+class ModuleType: ...
+```
+
 `/typeshed/stdlib/sys/__init__.pyi`:
 
 ```pyi
@@ -70,11 +80,18 @@ class OldClass: ...
 class NewClass: ...
 ```
 
+`/typeshed/stdlib/types.pyi`:
+
+```pyi
+class ModuleType: ...
+```
+
 `/typeshed/stdlib/VERSIONS`:
 
 ```text
 old_module: 3.0-
 new_module: 3.11-
+types: 3.9-
 ```
 
 ```py
@@ -102,6 +119,12 @@ typeshed = "/typeshed"
 def reveal_type(obj, /): ...
 ```
 
+`/typeshed/stdlib/types.pyi`:
+
+```pyi
+class ModuleType: ...
+```
+
 ```py
 reveal_type(())  # revealed: tuple[()]
 ```

crates/red_knot_python_semantic/src/types.rs

@@ -730,10 +730,9 @@ impl<'db> Type<'db> {
             // `Literal[str]` is a subtype of `type` because the `str` class object is an instance of its metaclass `type`.
             // `Literal[abc.ABC]` is a subtype of `abc.ABCMeta` because the `abc.ABC` class object
             // is an instance of its metaclass `abc.ABCMeta`.
-            (Type::ClassLiteral(ClassLiteralType { class }), _) => class
-                .metaclass(db)
-                .to_instance(db)
-                .is_some_and(|instance_type| instance_type.is_subtype_of(db, target)),
+            (Type::ClassLiteral(ClassLiteralType { class }), _) => {
+                class.metaclass_instance_type(db).is_subtype_of(db, target)
+            }
 
             // `type[str]` (== `SubclassOf("str")` in red-knot) describes all possible runtime subclasses
             // of the class object `str`. It is a subtype of `type` (== `Instance("type")`) because `str`
@@ -745,7 +744,7 @@ impl<'db> Type<'db> {
             (Type::SubclassOf(subclass_of_ty), _) => subclass_of_ty
                 .subclass_of()
                 .into_class()
-                .and_then(|class| class.metaclass(db).to_instance(db))
+                .map(|class| class.metaclass_instance_type(db))
                 .is_some_and(|metaclass_instance_type| {
                     metaclass_instance_type.is_subtype_of(db, target)
                 }),
@@ -1105,14 +1104,9 @@ impl<'db> Type<'db> {
                 ClassBase::Dynamic(_) => {
                     KnownClass::Type.to_instance(db).is_disjoint_from(db, other)
                 }
-                ClassBase::Class(class) => {
-                    class
-                        .metaclass(db)
-                        .to_instance(db)
-                        .is_some_and(|metaclass_instance_type| {
-                            metaclass_instance_type.is_disjoint_from(db, other)
-                        })
-                }
+                ClassBase::Class(class) => class
+                    .metaclass_instance_type(db)
+                    .is_disjoint_from(db, other),
             },
 
             (Type::KnownInstance(known_instance), Type::Instance(InstanceType { class }))
@@ -1182,11 +1176,8 @@ impl<'db> Type<'db> {
             (Type::ClassLiteral(ClassLiteralType { class }), instance @ Type::Instance(_))
             | (instance @ Type::Instance(_), Type::ClassLiteral(ClassLiteralType { class })) => {
                 !class
-                    .metaclass(db)
-                    .to_instance(db)
-                    .is_some_and(|metaclass_instance_type| {
-                        metaclass_instance_type.is_subtype_of(db, instance)
-                    })
+                    .metaclass_instance_type(db)
+                    .is_subtype_of(db, instance)
             }
 
             (Type::FunctionLiteral(..), Type::Instance(InstanceType { class }))
@@ -2090,13 +2081,9 @@ impl<'db> Type<'db> {
             Type::FunctionLiteral(_) => Truthiness::AlwaysTrue,
             Type::Callable(_) => Truthiness::AlwaysTrue,
             Type::ModuleLiteral(_) => Truthiness::AlwaysTrue,
-            Type::ClassLiteral(ClassLiteralType { class }) => {
-                if let Some(metaclass_instance_type) = class.metaclass(db).to_instance(db) {
-                    metaclass_instance_type.try_bool_impl(db, allow_short_circuit)?
-                } else {
-                    Truthiness::Ambiguous
-                }
-            }
+            Type::ClassLiteral(ClassLiteralType { class }) => class
+                .metaclass_instance_type(db)
+                .try_bool_impl(db, allow_short_circuit)?,
             Type::SubclassOf(subclass_of_ty) => match subclass_of_ty.subclass_of() {
                 ClassBase::Dynamic(_) => Truthiness::Ambiguous,
                 ClassBase::Class(class) => {
@@ -2936,8 +2923,6 @@ impl<'db> Type<'db> {
                 Some(builder.build())
             }
             Type::Intersection(_) => Some(todo_type!("Type::Intersection.to_instance()")),
-            // TODO: calling `.to_instance()` on any of these should result in a diagnostic,
-            // since they already indicate that the object is an instance of some kind:
             Type::BooleanLiteral(_)
             | Type::BytesLiteral(_)
             | Type::FunctionLiteral(_)

crates/red_knot_python_semantic/src/types/class.rs

@@ -188,6 +188,14 @@ impl<'db> Class<'db> {
             .unwrap_or_else(|_| SubclassOfType::subclass_of_unknown())
     }
 
+    /// Return a type representing "the set of all instances of the metaclass of this class".
+    pub(super) fn metaclass_instance_type(self, db: &'db dyn Db) -> Type<'db> {
+        self
+            .metaclass(db)
+            .to_instance(db)
+            .expect("`Type::to_instance()` should always return `Some()` when called on the type of a metaclass")
+    }
+
     /// Return the metaclass of this class, or an error if the metaclass cannot be inferred.
     #[salsa::tracked]
     pub(super) fn try_metaclass(self, db: &'db dyn Db) -> Result<Type<'db>, MetaclassError<'db>> {
@@ -972,16 +980,20 @@ impl<'db> KnownClass {
         self.try_to_class_literal(db)
             .map(Type::ClassLiteral)
             .unwrap_or_else(|lookup_error| {
-                if cfg!(test) {
-                    panic!("{}", lookup_error.display(db, self));
-                } else if MESSAGES.lock().unwrap().insert(self) {
+                assert!(
+                    !cfg!(any(test, debug_assertions)),
+                    "{}",
+                    lookup_error.display(db, self)
+                );
+
+                if MESSAGES.lock().unwrap().insert(self) {
                     if matches!(
                         lookup_error,
                         KnownClassLookupError::ClassPossiblyUnbound { .. }
                     ) {
-                        tracing::warn!("{}", lookup_error.display(db, self));
+                        tracing::debug!("{}", lookup_error.display(db, self));
                     } else {
-                        tracing::warn!(
+                        tracing::debug!(
                             "{}. Falling back to `Unknown` for the symbol instead.",
                             lookup_error.display(db, self)
                         );
@@ -1717,7 +1729,7 @@ mod tests {
         }
     }
 
-    fn setup_db_with_broken_typeshed(builtins_file: &str) -> TestDb {
+    fn setup_db_with_minimal_typeshed(builtins_file: &str) -> TestDb {
         TestDbBuilder::new()
             .with_custom_typeshed("/typeshed")
             .with_file("/typeshed/stdlib/builtins.pyi", builtins_file)
@@ -1730,21 +1742,21 @@ mod tests {
     #[test]
     #[should_panic(expected = "Could not find class `builtins.int` in typeshed")]
     fn known_class_to_class_literal_panics_with_test_feature_enabled() {
-        let db = setup_db_with_broken_typeshed("class object: ...");
+        let db = setup_db_with_minimal_typeshed("class object: ...");
         KnownClass::Int.to_class_literal(&db);
     }
 
     #[test]
     #[should_panic(expected = "Could not find class `builtins.int` in typeshed")]
     fn known_class_to_instance_panics_with_test_feature_enabled() {
-        let db = setup_db_with_broken_typeshed("class object: ...");
+        let db = setup_db_with_minimal_typeshed("class object: ...");
         KnownClass::Int.to_instance(&db);
     }
 
     #[test]
     #[should_panic(expected = "found a symbol of type `Unknown | Literal[42]` instead")]
     fn known_class_to_subclass_of_panics_with_test_feature_enabled() {
-        let db = setup_db_with_broken_typeshed("int = 42");
+        let db = setup_db_with_minimal_typeshed("int = 42");
         KnownClass::Int.to_subclass_of(&db);
     }
 }