-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[red-knot] Dataclasses: support order=True
#17406
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,7 @@ jobs: | |
--type-checker knot \ | ||
--old base_commit \ | ||
--new "$GITHUB_SHA" \ | ||
--project-selector '/(mypy_primer|black|pyp|git-revise|zipp|arrow|isort|itsdangerous|rich|packaging|pybind11|pyinstrument|typeshed-stats|scrapy|werkzeug|bidict|async-utils)$' \ | ||
--project-selector '/(mypy_primer|black|pyp|git-revise|zipp|arrow|isort|itsdangerous|rich|packaging|pybind11|pyinstrument|typeshed-stats|scrapy|werkzeug|bidict|async-utils|python-chess|dacite|python-htmlgen|paroxython|porcupine|psycopg)$' \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 |
||
--output concise \ | ||
--debug > mypy_primer.diff || [ $? -eq 1 ] | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,125 @@ repr(C()) | |
C() == C() | ||
``` | ||
|
||
## Other dataclass parameters | ||
|
||
### `repr` | ||
|
||
A custom `__repr__` method is generated by default. It can be disabled by passing `repr=False`, but | ||
in that case `__repr__` is still available via `object.__repr__`: | ||
|
||
```py | ||
from dataclasses import dataclass | ||
|
||
@dataclass(repr=False) | ||
class WithoutRepr: | ||
x: int | ||
|
||
reveal_type(WithoutRepr(1).__repr__) # revealed: bound method WithoutRepr.__repr__() -> str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I wonder if our display for bound methods really should show both the class the method was fetched from, and the object to which its Although mypy and pyright both reveal less than we do; they just reveal this as simply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That sounds potentially useful. What are some cases where this would show two different types? I can imagine something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main case I was thinking of is simply inheritance without a method override, as in the case shown here. It's a bit confusing that even though |
||
``` | ||
|
||
### `eq` | ||
|
||
The same is true for `__eq__`. Setting `eq=False` disables the generated `__eq__` method, but | ||
`__eq__` is still available via `object.__eq__`: | ||
|
||
```py | ||
from dataclasses import dataclass | ||
|
||
@dataclass(eq=False) | ||
class WithoutEq: | ||
x: int | ||
|
||
reveal_type(WithoutEq(1) == WithoutEq(2)) # revealed: bool | ||
``` | ||
|
||
### `order` | ||
|
||
`order` is set to `False` by default. If `order=True`, `__lt__`, `__le__`, `__gt__`, and `__ge__` | ||
methods will be generated: | ||
|
||
```py | ||
from dataclasses import dataclass | ||
|
||
@dataclass | ||
class WithoutOrder: | ||
x: int | ||
|
||
WithoutOrder(1) < WithoutOrder(2) # error: [unsupported-operator] | ||
WithoutOrder(1) <= WithoutOrder(2) # error: [unsupported-operator] | ||
WithoutOrder(1) > WithoutOrder(2) # error: [unsupported-operator] | ||
WithoutOrder(1) >= WithoutOrder(2) # error: [unsupported-operator] | ||
|
||
@dataclass(order=True) | ||
class WithOrder: | ||
x: int | ||
|
||
WithOrder(1) < WithOrder(2) | ||
WithOrder(1) <= WithOrder(2) | ||
WithOrder(1) > WithOrder(2) | ||
WithOrder(1) >= WithOrder(2) | ||
``` | ||
|
||
Comparisons are only allowed for `WithOrder` instances: | ||
|
||
```py | ||
WithOrder(1) < 2 # error: [unsupported-operator] | ||
WithOrder(1) <= 2 # error: [unsupported-operator] | ||
WithOrder(1) > 2 # error: [unsupported-operator] | ||
WithOrder(1) >= 2 # error: [unsupported-operator] | ||
``` | ||
|
||
This also works for generic dataclasses: | ||
|
||
```py | ||
from dataclasses import dataclass | ||
|
||
@dataclass(order=True) | ||
class GenericWithOrder[T]: | ||
x: T | ||
|
||
GenericWithOrder[int](1) < GenericWithOrder[int](1) | ||
|
||
GenericWithOrder[int](1) < GenericWithOrder[str]("a") # error: [unsupported-operator] | ||
``` | ||
|
||
If a class already defines one of the comparison methods, a `TypeError` is raised at runtime. | ||
Ideally, we would emit a diagnostic in that case: | ||
|
||
```py | ||
@dataclass(order=True) | ||
class AlreadyHasCustomDunderLt: | ||
x: int | ||
|
||
# TODO: Ideally, we would emit a diagnostic here | ||
def __lt__(self, other: object) -> bool: | ||
return False | ||
``` | ||
|
||
### `unsafe_hash` | ||
|
||
To do | ||
|
||
### `frozen` | ||
|
||
To do | ||
|
||
### `match_args` | ||
|
||
To do | ||
|
||
### `kw_only` | ||
|
||
To do | ||
|
||
### `slots` | ||
|
||
To do | ||
|
||
### `weakref_slot` | ||
|
||
To do | ||
|
||
## Inheritance | ||
|
||
### Normal class inheriting from a dataclass | ||
|
@@ -168,13 +287,30 @@ reveal_type(d_int.description) # revealed: str | |
DataWithDescription[int](None, "description") | ||
``` | ||
|
||
## Frozen instances | ||
## Descriptor-typed fields | ||
|
||
To do | ||
```py | ||
from dataclasses import dataclass | ||
|
||
## Descriptor-typed fields | ||
class Descriptor: | ||
_value: int = 0 | ||
|
||
To do | ||
def __get__(self, instance, owner) -> str: | ||
return str(self._value) | ||
|
||
def __set__(self, instance, value: int) -> None: | ||
self._value = value | ||
|
||
@dataclass | ||
class C: | ||
d: Descriptor = Descriptor() | ||
|
||
c = C(1) | ||
reveal_type(c.d) # revealed: str | ||
|
||
# TODO: should be an error | ||
C("a") | ||
``` | ||
|
||
## `dataclasses.field` | ||
|
||
|
@@ -197,18 +333,61 @@ class C: | |
reveal_type(C.__init__) # revealed: (*args: Any, **kwargs: Any) -> None | ||
``` | ||
|
||
### Dataclass with `init=False` | ||
### Dataclass with custom `__init__` method | ||
|
||
To do | ||
If a class already defines `__init__`, it is not replaced by the `dataclass` decorator. | ||
|
||
### Dataclass with custom `__init__` method | ||
```py | ||
from dataclasses import dataclass | ||
|
||
To do | ||
@dataclass(init=True) | ||
class C: | ||
x: str | ||
|
||
def __init__(self, x: int) -> None: | ||
self.x = str(x) | ||
|
||
C(1) # OK | ||
|
||
# TODO: should be an error | ||
C("a") | ||
``` | ||
|
||
Similarly, if we set `init=False`, we still recognize the custom `__init__` method: | ||
|
||
```py | ||
@dataclass(init=False) | ||
class D: | ||
def __init__(self, x: int) -> None: | ||
self.x = str(x) | ||
|
||
D(1) # OK | ||
D() # error: [missing-argument] | ||
``` | ||
|
||
### Dataclass with `ClassVar`s | ||
|
||
To do | ||
|
||
### Return type of `dataclass(...)` | ||
|
||
A call like `dataclass(order=True)` returns a callable itself, which is then used as the decorator. | ||
We can store the callable in a variable and later use it as a decorator: | ||
|
||
```py | ||
from dataclasses import dataclass | ||
|
||
dataclass_with_order = dataclass(order=True) | ||
|
||
reveal_type(dataclass_with_order) # revealed: <decorator produced by dataclasses.dataclass> | ||
|
||
@dataclass_with_order | ||
class C: | ||
x: int | ||
|
||
C(1) < C(2) # ok | ||
``` | ||
sharkdp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
### Using `dataclass` as a function | ||
|
||
To do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -823,21 +823,31 @@ impl<'db> ClassLiteralType<'db> { | |
name: &str, | ||
) -> SymbolAndQualifiers<'db> { | ||
if let Some(metadata) = self.dataclass_metadata(db) { | ||
if name == "__init__" { | ||
if metadata.contains(DataclassMetadata::INIT) { | ||
// TODO: Generate the signature from the attributes on the class | ||
let init_signature = Signature::new( | ||
Parameters::new([ | ||
Parameter::variadic(Name::new_static("args")) | ||
.with_annotated_type(Type::any()), | ||
Parameter::keyword_variadic(Name::new_static("kwargs")) | ||
.with_annotated_type(Type::any()), | ||
]), | ||
Some(Type::none(db)), | ||
if name == "__init__" && metadata.contains(DataclassMetadata::INIT) { | ||
// TODO: Generate the signature from the attributes on the class | ||
let init_signature = Signature::new( | ||
Parameters::new([ | ||
Parameter::variadic(Name::new_static("args")) | ||
.with_annotated_type(Type::any()), | ||
Parameter::keyword_variadic(Name::new_static("kwargs")) | ||
.with_annotated_type(Type::any()), | ||
]), | ||
Some(Type::none(db)), | ||
); | ||
|
||
return Symbol::bound(Type::Callable(CallableType::new(db, init_signature))).into(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As an aside, we might consider an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll note this down as a (follow up) to-do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern has since vanished from our code base; removing the to-do. |
||
} else if matches!(name, "__lt__" | "__le__" | "__gt__" | "__ge__") { | ||
if metadata.contains(DataclassMetadata::ORDER) { | ||
let signature = Signature::new( | ||
Parameters::new([Parameter::positional_or_keyword(Name::new_static( | ||
"other", | ||
)) | ||
Comment on lines
+842
to
+844
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only true for bound methods. We currently infer the wrong signature for explicit dunder calls, e.g. |
||
.with_annotated_type(Type::instance( | ||
self.apply_optional_specialization(db, specialization), | ||
))]), | ||
Some(KnownClass::Bool.to_instance(db)), | ||
); | ||
|
||
return Symbol::bound(Type::Callable(CallableType::new(db, init_signature))) | ||
.into(); | ||
return Symbol::bound(Type::Callable(CallableType::new(db, signature))).into(); | ||
} | ||
} | ||
} | ||
|
Uh oh!
There was an error while loading. Please reload this page.