Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[red-knot] Pure instance variables declared in class body #15515

Merged
merged 2 commits into from
Jan 17, 2025
Merged

[red-knot] Pure instance variables declared in class body #15515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
950f46c
[red-knot] Pure instance variables declared in class body
sharkdp Jan 15, 2025
b8c920a
Add TODO comment and two additional tests
sharkdp Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 39 additions & 34 deletions crates/red_knot_python_semantic/resources/mdtest/attributes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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__`
Expand All @@ -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
Expand Down Expand Up @@ -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]
Expand All @@ -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?
Expand Down Expand Up @@ -174,12 +172,15 @@ 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 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"
Expand Down Expand Up @@ -222,7 +223,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"
Expand All @@ -239,34 +240,38 @@ 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

# TODO: this should be `Unknown | Literal[1]`.
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]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct that we want Unknown | Literal[1] for the instance attribute access, but `Literal[1] for the class variable access? (above)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think we should probably add the union with Unknown for class attributes also; the same principles apply. (Though perhaps less strongly in practice, since mutation of class attributes is much less common -- but pyright and mypy both allow it, and we should too since it's allowed at runtime.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I added an additional test with a TODO for this case.


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
Expand Down Expand Up @@ -437,8 +442,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:
Expand All @@ -456,8 +461,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)
Comment on lines +464 to +465
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/python/typeshed/blob/2a461a2f7931dd58df8346aca626f5bc048e8455/stdlib/builtins.pyi#L243-L246

```

Some attributes are special-cased, however:
Expand All @@ -473,8 +478,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:
Expand All @@ -489,8 +494,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
Expand Down
8 changes: 5 additions & 3 deletions crates/red_knot_python_semantic/resources/mdtest/generics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
```
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/red_knot_python_semantic/resources/mdtest/pep695_type_aliases.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
```
6 changes: 3 additions & 3 deletions crates/red_knot_python_semantic/resources/mdtest/scopes/moduletype_attrs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions crates/red_knot_python_semantic/resources/mdtest/sys_version_info.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions crates/red_knot_python_semantic/resources/mdtest/type_of/dynamic.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: ...

Expand All @@ -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: ...

Expand Down
Loading
Loading