[red-knot] Rework Type::to_instance() to return Option<Type>
#16428
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,3 +1,5 @@ | ||||||
| use std::sync::{LazyLock, Mutex}; | ||||||
|
|
||||||
| use crate::{ | ||||||
| module_resolver::file_to_module, | ||||||
| semantic_index::{ | ||||||
|
|
@@ -18,6 +20,7 @@ use indexmap::IndexSet; | |||||
| use itertools::Itertools as _; | ||||||
| use ruff_db::files::File; | ||||||
| use ruff_python_ast::{self as ast, PythonVersion}; | ||||||
| use rustc_hash::FxHashSet; | ||||||
|
|
||||||
| use super::{ | ||||||
| class_base::ClassBase, infer_expression_type, infer_unpack_types, IntersectionBuilder, | ||||||
|
|
@@ -876,13 +879,61 @@ impl<'db> KnownClass { | |||||
| } | ||||||
|
|
||||||
| pub(crate) fn to_instance(self, db: &'db dyn Db) -> Type<'db> { | ||||||
| self.to_class_literal(db) | ||||||
| .into_class_literal() | ||||||
| .map(|ClassLiteralType { class }| Type::instance(class)) | ||||||
| .unwrap_or_else(Type::unknown) | ||||||
| } | ||||||
|
|
||||||
| pub(crate) fn try_to_class_literal( | ||||||
| self, | ||||||
| db: &'db dyn Db, | ||||||
| ) -> Result<ClassLiteralType<'db>, KnownClassLookupError<'db>> { | ||||||
| let symbol = known_module_symbol(db, self.canonical_module(db), self.as_str(db)); | ||||||
| match symbol { | ||||||
| Symbol::Type(Type::ClassLiteral(class_type), Boundness::Bound) => Ok(class_type), | ||||||
| Symbol::Type(Type::ClassLiteral(class_type), Boundness::PossiblyUnbound) => { | ||||||
| Err(KnownClassLookupError::ClassPossiblyUnbound { class_type }) | ||||||
| } | ||||||
| Symbol::Type(found_type, _) => { | ||||||
| Err(KnownClassLookupError::SymbolNotAClass { found_type }) | ||||||
| } | ||||||
| Symbol::Unbound => Err(KnownClassLookupError::ClassNotFound), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| pub(crate) fn to_class_literal(self, db: &'db dyn Db) -> Type<'db> { | ||||||
| // a cache of the `KnownClass`es that we have already failed to lookup in typeshed | ||||||
| // (and therefore that we've already logged a warning for) | ||||||
| static MESSAGES: LazyLock<Mutex<FxHashSet<KnownClass>>> = LazyLock::new(Mutex::default); | ||||||
|
Comment on lines
+906
to
+908
There was a problem hiding this comment. this approach was inspired by the existing Ruff macro ruff/crates/ruff_linter/src/logging.rs Lines 37 to 55 in 0c7c001 I couldn't find any tests for that macro, and I wasn't sure how to test this (since it deliberately works differently depending on whether the |
||||||
|
|
||||||
| self.try_to_class_literal(db) | ||||||
| .map(Type::ClassLiteral) | ||||||
| .unwrap_or_else(|lookup_error| { | ||||||
| if cfg!(test) { | ||||||
|
There was a problem hiding this comment. Or I suppose this could be
Suggested change
? There was a problem hiding this comment. or instead of being an if/else, this could simply be |
||||||
| panic!("{}", lookup_error.display(db, self)); | ||||||
| } else if MESSAGES.lock().unwrap().insert(self) { | ||||||
| if matches!( | ||||||
| lookup_error, | ||||||
| KnownClassLookupError::ClassPossiblyUnbound { .. } | ||||||
| ) { | ||||||
| tracing::warn!("{}", lookup_error.display(db, self)); | ||||||
| } else { | ||||||
| tracing::warn!( | ||||||
| "{}. Falling back to `Unknown` for the symbol instead.", | ||||||
| lookup_error.display(db, self) | ||||||
| ); | ||||||
| } | ||||||
|
Comment on lines
+916
to
+926
There was a problem hiding this comment. FWIW, I'm not totally convinced that we need to / should warn on this at all. It seems natural that if you don't have a class in your custom typeshed, then any special behavior we have around that class won't occur. If someone is doing this intentionally, it could be irritating to have a non-silencable warning about it (and using a "minimalist" typeshed doesn't seem like a crazy thing for someone to do intentionally). It could be accidental, but there are tons of ways a custom typeshed could quietly break things, it seems odd to warn about this specific one. I don't feel strongly about it, though, what you have here seems fine and we can adjust with more experience. There was a problem hiding this comment. hmmm... yeah, I originally suggested just logging it (so that the information would be available to us when debugging, but wouldn't be printed by default), but @MichaReiser suggested in #16302 (comment) that a warning might be better... I think I'll switch it to being a LOG rather than a warning, so that you only get it if you specify |
||||||
| } | ||||||
|
|
||||||
| match lookup_error { | ||||||
| KnownClassLookupError::ClassPossiblyUnbound { class_type, .. } => { | ||||||
| Type::class_literal(class_type.class) | ||||||
| } | ||||||
| KnownClassLookupError::ClassNotFound { .. } | ||||||
| | KnownClassLookupError::SymbolNotAClass { .. } => Type::unknown(), | ||||||
| } | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| pub(crate) fn to_subclass_of(self, db: &'db dyn Db) -> Type<'db> { | ||||||
|
|
@@ -895,10 +946,8 @@ impl<'db> KnownClass { | |||||
| /// Return `true` if this symbol can be resolved to a class definition `class` in typeshed, | ||||||
| /// *and* `class` is a subclass of `other`. | ||||||
| pub(super) fn is_subclass_of(self, db: &'db dyn Db, other: Class<'db>) -> bool { | ||||||
| self.try_to_class_literal(db) | ||||||
| .is_ok_and(|ClassLiteralType { class }| class.is_subclass_of(db, other)) | ||||||
| } | ||||||
|
|
||||||
| /// Return the module in which we should look up the definition for this class | ||||||
|
|
@@ -931,11 +980,10 @@ impl<'db> KnownClass { | |||||
| | Self::MethodWrapperType | ||||||
| | Self::WrapperDescriptorType => KnownModule::Types, | ||||||
| Self::NoneType => KnownModule::Typeshed, | ||||||
| Self::SpecialForm | Self::TypeVar | Self::StdlibAlias | Self::SupportsIndex => { | ||||||
| KnownModule::Typing | ||||||
| } | ||||||
| Self::TypeAliasType => KnownModule::TypingExtensions, | ||||||
|
Comment on lines
-934
to
+986
There was a problem hiding this comment. The change here is that the canonical module for |
||||||
| Self::NoDefaultType => { | ||||||
| let python_version = Program::get(db).python_version(db); | ||||||
|
|
||||||
|
|
@@ -1164,6 +1212,57 @@ impl<'db> KnownClass { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||||||
| pub(crate) enum KnownClassLookupError<'db> { | ||||||
| ClassNotFound, | ||||||
| SymbolNotAClass { found_type: Type<'db> }, | ||||||
| ClassPossiblyUnbound { class_type: ClassLiteralType<'db> }, | ||||||
| } | ||||||
|
|
||||||
| impl<'db> KnownClassLookupError<'db> { | ||||||
| fn display(&self, db: &'db dyn Db, class: KnownClass) -> impl std::fmt::Display + 'db { | ||||||
| struct ErrorDisplay<'db> { | ||||||
| db: &'db dyn Db, | ||||||
| class: KnownClass, | ||||||
| error: KnownClassLookupError<'db>, | ||||||
| } | ||||||
|
|
||||||
| impl std::fmt::Display for ErrorDisplay<'_> { | ||||||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||||||
| let ErrorDisplay { db, class, error } = *self; | ||||||
|
|
||||||
| let module = class.canonical_module(db).as_str(); | ||||||
| let class = class.as_str(db); | ||||||
| let python_version = Program::get(db).python_version(db); | ||||||
|
|
||||||
| match error { | ||||||
| KnownClassLookupError::ClassNotFound => write!( | ||||||
| f, | ||||||
| "Could not find class `{module}.{class}` in typeshed on Python {python_version}", | ||||||
| ), | ||||||
| KnownClassLookupError::SymbolNotAClass { found_type } => write!( | ||||||
| f, | ||||||
| "Error looking up `{module}.{class}` in typeshed: expected to find a class definition \ | ||||||
| on Python {python_version}, but found a symbol of type `{found_type}` instead", | ||||||
| found_type = found_type.display(db), | ||||||
| ), | ||||||
| KnownClassLookupError::ClassPossiblyUnbound { .. } => write!( | ||||||
| f, | ||||||
| "Error looking up `{module}.{class}` in typeshed on Python {python_version}: \ | ||||||
| expected to find a fully bound symbol, but found one that is possibly unbound", | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| ErrorDisplay { | ||||||
| db, | ||||||
| class, | ||||||
| error: *self, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Enumeration of specific runtime that are special enough to be considered their own type. | ||||||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, salsa::Update)] | ||||||
| pub enum KnownInstanceType<'db> { | ||||||
|
|
@@ -1539,7 +1638,7 @@ pub(super) enum MetaclassErrorKind<'db> { | |||||
| #[cfg(test)] | ||||||
| mod tests { | ||||||
| use super::*; | ||||||
| use crate::db::tests::{setup_db, TestDb, TestDbBuilder}; | ||||||
| use crate::module_resolver::resolve_module; | ||||||
| use strum::IntoEnumIterator; | ||||||
|
|
||||||
|
|
@@ -1557,4 +1656,35 @@ mod tests { | |||||
| ); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| fn setup_db_with_broken_typeshed(builtins_file: &str) -> TestDb { | ||||||
|
There was a problem hiding this comment. I guess this is a wording nit, sort of related to my "not sure we need to warn" comment above -- but I slightly object to the framing of this as a "broken" typeshed. I think we should aim to consider typesheds where certain symbols aren't present or aren't what we expect to still be "working", where some behaviors will just fall back to gradual typing. Maybe it doesn't really matter, but I'd prefer we don't accept the idea that it's OK for things to be "broken" because a minimal typeshed is used. There was a problem hiding this comment. Fair enough. FWIW, we do panic (Salsa cycles etc.) if there's no There was a problem hiding this comment. If it's salsa cycles, I expect the panics will go away with fixpoint iteration? Will check! Not having |
||||||
| TestDbBuilder::new() | ||||||
| .with_custom_typeshed("/typeshed") | ||||||
| .with_file("/typeshed/stdlib/builtins.pyi", builtins_file) | ||||||
| .with_file("/typeshed/stdlib/types.pyi", "class ModuleType: ...") | ||||||
| .with_file("/typeshed/stdlib/VERSIONS", "builtins: 3.8-\ntypes: 3.8-") | ||||||
| .build() | ||||||
| .unwrap() | ||||||
| } | ||||||
|
|
||||||
| #[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: ..."); | ||||||
| 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: ..."); | ||||||
| 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"); | ||||||
| KnownClass::Int.to_subclass_of(&db); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be an invariant that a class' metaclass is always something instantiable? It seems like it should be, since the class itself should be an instance of its metaclass.
Should we have a method that gets the instance type for a class' metaclass and handles this internally with an
unreachable!instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting point -- yes, I think you're right!