-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[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
Conversation
e5c8f19 to
2207c55
Compare
|
2c6e0ed to
acb0747
Compare
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, we might consider an impl From<Signature> for Symbol since this pattern is starting to show up a lot
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.
I'll note this down as a (follow up) to-do.
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.
This pattern has since vanished from our code base; removing the to-do.
acb0747 to
aa6c7c4
Compare
| Parameters::new([Parameter::positional_or_keyword(Name::new_static( | ||
| "other", | ||
| )) |
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.
This is only true for bound methods. We currently infer the wrong signature for explicit dunder calls, e.g. MyDataclass.__lt__(left, right). This is a pre-existing problem with __init__ as well. I will tackle this in a separate follow-up.
182d0a1 to
2a31576
Compare
2a31576 to
eedb81a
Compare
carljm
left a comment
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.
Looks good!
| --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 comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
| 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 comment
The 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 self is bound...
Although mypy and pyright both reveal less than we do; they just reveal this as simply () -> str
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.
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
selfis bound...
That sounds potentially useful. What are some cases where this would show two different types? I can imagine something like SomeClass.some_function.__get__(OtherClass()) which should show <bound method SomeFunction.some_function of OtherClass object>. But there are probably more realistic scenarios where this would show up?
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.
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 WithoutRepr does not have its own __repr__ and we are finding the __repr__ from object, that we show WithoutRepr.__repr__, a syntax that implies a __repr__ method defined on WithoutRepr class. I kind of want it to say object.__repr__ instead, but then we would no longer be showing the type of the bound self object.
* main: [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406)
* main: (123 commits) [red-knot] Handle explicit class specialization in type expressions (#17434) [red-knot] allow assignment expression in call compare narrowing (#17461) [red-knot] fix building unions with literals and AlwaysTruthy/AlwaysFalsy (#17451) [red-knot] Type narrowing for assertions (take 2) (#17345) [red-knot] class bases are not affected by __future__.annotations (#17456) [red-knot] Add support for overloaded functions (#17366) [`pyupgrade`] Add fix safety section to docs (`UP036`) (#17444) [red-knot] more type-narrowing in match statements (#17302) [red-knot] Add some narrowing for assignment expressions (#17448) [red-knot] Understand `typing.Protocol` and `typing_extensions.Protocol` as equivalent (#17446) Server: Use `min` instead of `max` to limit the number of threads (#17421) [red-knot] Detect version-related syntax errors (#16379) [`pyflakes`] Add fix safety section (`F841`) (#17410) [red-knot] Add `KnownFunction` variants for `is_protocol`, `get_protocol_members` and `runtime_checkable` (#17450) Bump 0.11.6 (#17449) Auto generate `visit_source_order` (#17180) [red-knot] Initial tests for protocols (#17436) [red-knot] Dataclasses: synthesize `__init__` with proper signature (#17428) [red-knot] Dataclasses: support `order=True` (#17406) [red-knot] Super-basic generic inference at call sites (#17301) ...

Summary
Support dataclasses with
order=True:Also adds some additional tests to
dataclasses.md.ticket: #16651
Test Plan
New Markdown tests