From 950f46cf3986dceda052d24bf32b8b0304745e2a Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 15 Jan 2025 22:46:39 +0100 Subject: [PATCH 1/2] [red-knot] Pure instance variables declared in class body --- .../resources/mdtest/attributes.md | 68 +++++------ .../resources/mdtest/generics.md | 8 +- .../resources/mdtest/pep695_type_aliases.md | 4 +- .../mdtest/scopes/moduletype_attrs.md | 6 +- .../resources/mdtest/sys_version_info.md | 6 +- .../resources/mdtest/type_of/dynamic.md | 4 +- crates/red_knot_python_semantic/src/types.rs | 111 +++++++++++++++--- 7 files changed, 146 insertions(+), 61 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/attributes.md b/crates/red_knot_python_semantic/resources/mdtest/attributes.md index 4246f566136116..e5fbc715914a6b 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/attributes.md +++ b/crates/red_knot_python_semantic/resources/mdtest/attributes.md @@ -34,21 +34,21 @@ c_instance = C(1) # TODO: should be `Literal["value set in __init__"]`, or `Unknown | Literal[…]` to allow # assignments to this unannotated attribute from other scopes. -reveal_type(c_instance.pure_instance_variable1) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable1) # revealed: @Todo(implicit instance attribute) # TODO: should be `int` -reveal_type(c_instance.pure_instance_variable2) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable2) # revealed: @Todo(implicit instance attribute) # TODO: should be `bytes` -reveal_type(c_instance.pure_instance_variable3) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable3) # revealed: @Todo(implicit instance attribute) # TODO: should be `bool` -reveal_type(c_instance.pure_instance_variable4) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable4) # revealed: @Todo(implicit instance attribute) # TODO: should be `str` # We probably don't want to emit a diagnostic for this being possibly undeclared/unbound. # mypy and pyright do not show an error here. -reveal_type(c_instance.pure_instance_variable5) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable5) # revealed: @Todo(implicit instance attribute) # TODO: If we choose to infer a precise `Literal[…]` type for the instance attribute (see # above), this should be an error: incompatible types in assignment. If we choose to infer @@ -74,7 +74,7 @@ c_instance.pure_instance_variable4 = False # in general (we don't know what else happened to `c_instance` between the assignment and the use # here), but mypy and pyright support this. In conclusion, this could be `bool` but should probably # be `Literal[False]`. -reveal_type(c_instance.pure_instance_variable4) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable4) # revealed: @Todo(implicit instance attribute) ``` #### Variable declared in class body and declared/bound in `__init__` @@ -91,8 +91,7 @@ class C: c_instance = C() -# TODO: should be `str` -reveal_type(c_instance.pure_instance_variable) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable) # revealed: str # TODO: we currently plan to emit a diagnostic here. Note that both mypy # and pyright show no error in this case! So we may reconsider this in @@ -123,7 +122,7 @@ c_instance = C() c_instance.set_instance_variable() # TODO: should be `Literal["value set in method"]` or `Unknown | Literal[…]` (see above). -reveal_type(c_instance.pure_instance_variable) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable) # revealed: @Todo(implicit instance attribute) # TODO: We already show an error here, but the message might be improved? # error: [unresolved-attribute] @@ -144,8 +143,7 @@ class C: c_instance = C() -# TODO: should be 'str' -reveal_type(c_instance.pure_instance_variable) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_instance_variable) # revealed: str # TODO: mypy and pyright do not show an error here, but we plan to emit a diagnostic. # The type could be changed to 'Unknown' if we decide to emit an error? @@ -174,12 +172,12 @@ class C: reveal_type(C.pure_class_variable1) # revealed: str # TODO: this should be `Literal[1]`, or `Unknown | Literal[1]`. -reveal_type(C.pure_class_variable2) # revealed: @Todo(Unsupported or invalid type in a type expression) +reveal_type(C.pure_class_variable2) # revealed: Unknown c_instance = C() -# TODO: This should be `str`. It is okay to access a pure class variable on an instance. -reveal_type(c_instance.pure_class_variable1) # revealed: @Todo(instance attributes) +# It is okay to access a pure class variable on an instance. +reveal_type(c_instance.pure_class_variable1) # revealed: str # TODO: should raise an error. It is not allowed to reassign a pure class variable on an instance. c_instance.pure_class_variable1 = "value set on instance" @@ -222,7 +220,7 @@ reveal_type(C.pure_class_variable) # revealed: Unknown c_instance = C() # TODO: should be `Literal["overwritten on class"]` -reveal_type(c_instance.pure_class_variable) # revealed: @Todo(instance attributes) +reveal_type(c_instance.pure_class_variable) # revealed: @Todo(implicit instance attribute) # TODO: should raise an error. c_instance.pure_class_variable = "value set on instance" @@ -239,34 +237,36 @@ attributes. ```py class C: - variable_with_class_default: str = "value in class body" + variable_with_class_default1: str = "value in class body" + variable_with_class_default2 = 1 def instance_method(self): - self.variable_with_class_default = "value set in instance method" + self.variable_with_class_default1 = "value set in instance method" -reveal_type(C.variable_with_class_default) # revealed: str +reveal_type(C.variable_with_class_default1) # revealed: str +reveal_type(C.variable_with_class_default2) # revealed: Literal[1] c_instance = C() -# TODO: should be `str` -reveal_type(c_instance.variable_with_class_default) # revealed: @Todo(instance attributes) +reveal_type(c_instance.variable_with_class_default1) # revealed: str +reveal_type(c_instance.variable_with_class_default2) # revealed: Unknown | Literal[1] -c_instance.variable_with_class_default = "value set on instance" +c_instance.variable_with_class_default1 = "value set on instance" -reveal_type(C.variable_with_class_default) # revealed: str +reveal_type(C.variable_with_class_default1) # revealed: str # TODO: Could be Literal["value set on instance"], or still `str` if we choose not to # narrow the type. -reveal_type(c_instance.variable_with_class_default) # revealed: @Todo(instance attributes) +reveal_type(c_instance.variable_with_class_default1) # revealed: str -C.variable_with_class_default = "overwritten on class" +C.variable_with_class_default1 = "overwritten on class" # TODO: Could be `Literal["overwritten on class"]`, or still `str` if we choose not to # narrow the type. -reveal_type(C.variable_with_class_default) # revealed: str +reveal_type(C.variable_with_class_default1) # revealed: str # TODO: should still be `Literal["value set on instance"]`, or `str`. -reveal_type(c_instance.variable_with_class_default) # revealed: @Todo(instance attributes) +reveal_type(c_instance.variable_with_class_default1) # revealed: str ``` ## Union of attributes @@ -437,8 +437,8 @@ functions are instances of that class: ```py path=a.py def f(): ... -reveal_type(f.__defaults__) # revealed: @Todo(instance attributes) -reveal_type(f.__kwdefaults__) # revealed: @Todo(instance attributes) +reveal_type(f.__defaults__) # revealed: @Todo(full tuple[...] support) | None +reveal_type(f.__kwdefaults__) # revealed: @Todo(generics) | None ``` Some attributes are special-cased, however: @@ -456,8 +456,8 @@ Most attribute accesses on int-literal types are delegated to `builtins.int`, si integers are instances of that class: ```py path=a.py -reveal_type((2).bit_length) # revealed: @Todo(instance attributes) -reveal_type((2).denominator) # revealed: @Todo(instance attributes) +reveal_type((2).bit_length) # revealed: @Todo(bound method) +reveal_type((2).denominator) # revealed: @Todo(@property) ``` Some attributes are special-cased, however: @@ -473,8 +473,8 @@ Most attribute accesses on bool-literal types are delegated to `builtins.bool`, bols are instances of that class: ```py path=a.py -reveal_type(True.__and__) # revealed: @Todo(instance attributes) -reveal_type(False.__or__) # revealed: @Todo(instance attributes) +reveal_type(True.__and__) # revealed: @Todo(bound method) +reveal_type(False.__or__) # revealed: @Todo(bound method) ``` Some attributes are special-cased, however: @@ -489,8 +489,8 @@ reveal_type(False.real) # revealed: Literal[0] All attribute access on literal `bytes` types is currently delegated to `buitins.bytes`: ```py -reveal_type(b"foo".join) # revealed: @Todo(instance attributes) -reveal_type(b"foo".endswith) # revealed: @Todo(instance attributes) +reveal_type(b"foo".join) # revealed: @Todo(bound method) +reveal_type(b"foo".endswith) # revealed: @Todo(bound method) ``` ## References diff --git a/crates/red_knot_python_semantic/resources/mdtest/generics.md b/crates/red_knot_python_semantic/resources/mdtest/generics.md index c8f80eb076bf5b..1f5d1212e86f22 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/generics.md +++ b/crates/red_knot_python_semantic/resources/mdtest/generics.md @@ -17,8 +17,8 @@ box: MyBox[int] = MyBox(5) # TODO should emit a diagnostic here (str is not assignable to int) wrong_innards: MyBox[int] = MyBox("five") -# TODO reveal int -reveal_type(box.data) # revealed: @Todo(instance attributes) +# TODO reveal int, do not leak the typevar +reveal_type(box.data) # revealed: T reveal_type(MyBox.box_model_number) # revealed: Literal[695] ``` @@ -39,7 +39,9 @@ class MySecureBox[T](MyBox[T]): ... secure_box: MySecureBox[int] = MySecureBox(5) reveal_type(secure_box) # revealed: MySecureBox # TODO reveal int -reveal_type(secure_box.data) # revealed: @Todo(instance attributes) +# The @Todo(…) is misleading here. We currently treat `MyBox[T]` as a dynamic base class because we +# don't understand generics and therefore infer `Unknown` for the `MyBox[T]` base of `MySecureBox[T]`. +reveal_type(secure_box.data) # revealed: @Todo(instance attribute on class with dynamic base) ``` ## Cyclical class definition diff --git a/crates/red_knot_python_semantic/resources/mdtest/pep695_type_aliases.md b/crates/red_knot_python_semantic/resources/mdtest/pep695_type_aliases.md index de2eccf81ea81b..e780e5e89251d7 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/pep695_type_aliases.md +++ b/crates/red_knot_python_semantic/resources/mdtest/pep695_type_aliases.md @@ -31,7 +31,7 @@ type IntOrStr = int | str # TODO: This should either fall back to the specified type from typeshed, # which is `Any`, or be the actual type of the runtime value expression # `int | str`, i.e. `types.UnionType`. -reveal_type(IntOrStr.__value__) # revealed: @Todo(instance attributes) +reveal_type(IntOrStr.__value__) # revealed: @Todo(@property) ``` ## Invalid assignment @@ -74,5 +74,5 @@ type ListOrSet[T] = list[T] | set[T] # TODO: Should be `tuple[typing.TypeVar | typing.ParamSpec | typing.TypeVarTuple, ...]`, # as specified in the `typeshed` stubs. -reveal_type(ListOrSet.__type_params__) # revealed: @Todo(instance attributes) +reveal_type(ListOrSet.__type_params__) # revealed: @Todo(@property) ``` diff --git a/crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md b/crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md index 5ee7a6334f79d4..21daddc5af9faf 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md +++ b/crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md @@ -63,7 +63,7 @@ reveal_type(typing.__class__) # revealed: Literal[ModuleType] # TODO: needs support for attribute access on instances, properties and generics; # should be `dict[str, Any]` -reveal_type(typing.__dict__) # revealed: @Todo(instance attributes) +reveal_type(typing.__dict__) # revealed: @Todo(@property) ``` Typeshed includes a fake `__getattr__` method in the stub for `types.ModuleType` to help out with @@ -95,8 +95,8 @@ from foo import __dict__ as foo_dict # TODO: needs support for attribute access on instances, properties, and generics; # should be `dict[str, Any]` for both of these: -reveal_type(foo.__dict__) # revealed: @Todo(instance attributes) -reveal_type(foo_dict) # revealed: @Todo(instance attributes) +reveal_type(foo.__dict__) # revealed: @Todo(@property) +reveal_type(foo_dict) # revealed: @Todo(@property) ``` ## Conditionally global or `ModuleType` attribute diff --git a/crates/red_knot_python_semantic/resources/mdtest/sys_version_info.md b/crates/red_knot_python_semantic/resources/mdtest/sys_version_info.md index 39fb613a10720c..11f0eb52c8281b 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/sys_version_info.md +++ b/crates/red_knot_python_semantic/resources/mdtest/sys_version_info.md @@ -117,9 +117,9 @@ properties on instance types: ```py path=b.py import sys -reveal_type(sys.version_info.micro) # revealed: @Todo(instance attributes) -reveal_type(sys.version_info.releaselevel) # revealed: @Todo(instance attributes) -reveal_type(sys.version_info.serial) # revealed: @Todo(instance attributes) +reveal_type(sys.version_info.micro) # revealed: @Todo(@property) +reveal_type(sys.version_info.releaselevel) # revealed: @Todo(@property) +reveal_type(sys.version_info.serial) # revealed: @Todo(@property) ``` ## Accessing fields by index/slice diff --git a/crates/red_knot_python_semantic/resources/mdtest/type_of/dynamic.md b/crates/red_knot_python_semantic/resources/mdtest/type_of/dynamic.md index 4e60f63bde0362..da7836d4f79b9c 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/type_of/dynamic.md +++ b/crates/red_knot_python_semantic/resources/mdtest/type_of/dynamic.md @@ -33,7 +33,7 @@ in strict mode. ```py def f(x: type): reveal_type(x) # revealed: type - reveal_type(x.__repr__) # revealed: @Todo(instance attributes) + reveal_type(x.__repr__) # revealed: @Todo(bound method) class A: ... @@ -48,7 +48,7 @@ x: type = A() # error: [invalid-assignment] ```py def f(x: type[object]): reveal_type(x) # revealed: type - reveal_type(x.__repr__) # revealed: @Todo(instance attributes) + reveal_type(x.__repr__) # revealed: @Todo(bound method) class A: ... diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 0ce9b0610db59c..6044c88e7a63ec 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -1608,6 +1608,7 @@ impl<'db> Type<'db> { | KnownClass::FrozenSet | KnownClass::Dict | KnownClass::Slice + | KnownClass::Property | KnownClass::BaseException | KnownClass::BaseExceptionGroup | KnownClass::GenericAlias @@ -1665,19 +1666,15 @@ impl<'db> Type<'db> { Type::KnownInstance(known_instance) => known_instance.member(db, name), - Type::Instance(InstanceType { class }) => { - let ty = match (class.known(db), name) { - (Some(KnownClass::VersionInfo), "major") => { - Type::IntLiteral(Program::get(db).python_version(db).major.into()) - } - (Some(KnownClass::VersionInfo), "minor") => { - Type::IntLiteral(Program::get(db).python_version(db).minor.into()) - } - // TODO MRO? get_own_instance_member, get_instance_member - _ => todo_type!("instance attributes"), - }; - ty.into() - } + Type::Instance(InstanceType { class }) => match (class.known(db), name) { + (Some(KnownClass::VersionInfo), "major") => { + Type::IntLiteral(Program::get(db).python_version(db).major.into()).into() + } + (Some(KnownClass::VersionInfo), "minor") => { + Type::IntLiteral(Program::get(db).python_version(db).minor.into()).into() + } + _ => class.instance_member(db, name), + }, Type::Union(union) => { let mut builder = UnionBuilder::new(db); @@ -2291,6 +2288,7 @@ impl<'db> Type<'db> { invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareAnnotated], fallback_type: Type::unknown(), }), + Type::KnownInstance(KnownInstanceType::ClassVar) => Ok(Type::unknown()), Type::KnownInstance(KnownInstanceType::Literal) => Err(InvalidTypeExpressionError { invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareLiteral], fallback_type: Type::unknown(), @@ -2536,6 +2534,7 @@ pub enum KnownClass { FrozenSet, Dict, Slice, + Property, BaseException, BaseExceptionGroup, // Types @@ -2580,6 +2579,7 @@ impl<'db> KnownClass { Self::List => "list", Self::Type => "type", Self::Slice => "slice", + Self::Property => "property", Self::BaseException => "BaseException", Self::BaseExceptionGroup => "BaseExceptionGroup", Self::GenericAlias => "GenericAlias", @@ -2649,7 +2649,8 @@ impl<'db> KnownClass { | Self::Dict | Self::BaseException | Self::BaseExceptionGroup - | Self::Slice => KnownModule::Builtins, + | Self::Slice + | Self::Property => KnownModule::Builtins, Self::VersionInfo => KnownModule::Sys, Self::GenericAlias | Self::ModuleType | Self::FunctionType => KnownModule::Types, Self::NoneType => KnownModule::Typeshed, @@ -2696,6 +2697,7 @@ impl<'db> KnownClass { | Self::List | Self::Type | Self::Slice + | Self::Property | Self::GenericAlias | Self::ModuleType | Self::FunctionType @@ -2770,6 +2772,7 @@ impl<'db> KnownClass { | Self::FrozenSet | Self::Dict | Self::Slice + | Self::Property | Self::GenericAlias | Self::ChainMap | Self::Counter @@ -3965,6 +3968,86 @@ impl<'db> Class<'db> { symbol(db, scope, name) } + /// Returns the `name` attribute of an instance of this class. + /// + /// The attribute could be defined in the class body, but it could also be an implicitly + /// defined attribute that is only present in a method (typically `__init__`). + /// + /// The attribute might also be defined in a superclass of this class. + pub(crate) fn instance_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> { + for superclass in self.iter_mro(db) { + match superclass { + ClassBase::Dynamic(_) => { + return todo_type!("instance attribute on class with dynamic base").into(); + } + ClassBase::Class(class) => { + let member = class.own_instance_member(db, name); + if !member.is_unbound() { + return member; + } + } + } + } + + // TODO: The symbol is not present in any class body, but it could be implicitly + // defined in `__init__` or other methods anywhere in the MRO. + todo_type!("implicit instance attribute").into() + } + + /// A helper function for `instance_member` that looks up the `name` attribute only on + /// this class, not on its superclasses. + fn own_instance_member(self, db: &'db dyn Db, name: &str) -> Symbol<'db> { + // TODO: There are many things that are not yet implemented here: + // - `typing.ClassVar` and `typing.Final` + // - Proper diagnostics + // - Handling of possibly-undeclared/possibly-unbound attributes + // - The descriptor protocol + + let body_scope = self.body_scope(db); + let table = symbol_table(db, body_scope); + + if let Some(symbol) = table.symbol_id_by_name(name) { + let use_def = use_def_map(db, body_scope); + + let declarations = use_def.public_declarations(symbol); + + match declarations_ty(db, declarations) { + Ok(Symbol::Type(declared_ty, _)) => { + if let Some(function) = declared_ty.into_function_literal() { + // TODO: Eventually, we are going to process all decorators correctly. This is + // just a temporary heuristic to provide a broad categorization into properties + // and non-property methods. + if function.has_decorator(db, KnownClass::Property.to_class_literal(db)) { + todo_type!("@property").into() + } else { + todo_type!("bound method").into() + } + } else { + Symbol::Type(declared_ty, Boundness::Bound) + } + } + Ok(Symbol::Unbound) => { + let bindings = use_def.public_bindings(symbol); + let inferred_ty = bindings_ty(db, bindings); + + match inferred_ty { + Symbol::Type(ty, _) => Symbol::Type( + UnionType::from_elements(db, [Type::unknown(), ty]), + Boundness::Bound, + ), + Symbol::Unbound => Symbol::Unbound, + } + } + Err((declared_ty, _)) => { + // Ignore conflicting declarations + declared_ty.into() + } + } + } else { + Symbol::Unbound + } + } + /// Return `true` if this class appears to be a cyclic definition, /// i.e., it inherits either directly or indirectly from itself. /// From b8c920a49ba84cebb3a6ee19f54c44ea4689ce92 Mon Sep 17 00:00:00 2001 From: David Peter Date: Fri, 17 Jan 2025 10:25:33 +0100 Subject: [PATCH 2/2] Add TODO comment and two additional tests --- .../red_knot_python_semantic/resources/mdtest/attributes.md | 5 +++++ crates/red_knot_python_semantic/src/types.rs | 6 +++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/attributes.md b/crates/red_knot_python_semantic/resources/mdtest/attributes.md index e5fbc715914a6b..24f0be14f3b9ea 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/attributes.md +++ b/crates/red_knot_python_semantic/resources/mdtest/attributes.md @@ -179,6 +179,9 @@ c_instance = C() # It is okay to access a pure class variable on an instance. reveal_type(c_instance.pure_class_variable1) # revealed: str +# TODO: Should be `Unknown | Literal[1]`. +reveal_type(c_instance.pure_class_variable2) # revealed: Unknown + # TODO: should raise an error. It is not allowed to reassign a pure class variable on an instance. c_instance.pure_class_variable1 = "value set on instance" @@ -244,6 +247,8 @@ class C: self.variable_with_class_default1 = "value set in instance method" reveal_type(C.variable_with_class_default1) # revealed: str + +# TODO: this should be `Unknown | Literal[1]`. reveal_type(C.variable_with_class_default2) # revealed: Literal[1] c_instance = C() diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index 6044c88e7a63ec..a5ff0358d88c96 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -2288,7 +2288,11 @@ impl<'db> Type<'db> { invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareAnnotated], fallback_type: Type::unknown(), }), - Type::KnownInstance(KnownInstanceType::ClassVar) => Ok(Type::unknown()), + Type::KnownInstance(KnownInstanceType::ClassVar) => { + // TODO: A bare `ClassVar` should rather be treated as if the symbol was not + // declared at all. + Ok(Type::unknown()) + } Type::KnownInstance(KnownInstanceType::Literal) => Err(InvalidTypeExpressionError { invalid_expressions: smallvec::smallvec![InvalidTypeExpression::BareLiteral], fallback_type: Type::unknown(),