Skip to content

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Apr 15, 2025

Summary

Support dataclasses with order=True:

@dataclass(order=True)
class WithOrder:
    x: int

WithOrder(1) < WithOrder(2)  # no error

Also adds some additional tests to dataclasses.md.

ticket: #16651

Test Plan

New Markdown tests

@sharkdp sharkdp added the ty Multi-file analysis & type inference label Apr 15, 2025
@sharkdp sharkdp force-pushed the david/dataclasses-pt2 branch from e5c8f19 to 2207c55 Compare April 15, 2025 09:51
Copy link
Contributor

github-actions bot commented Apr 15, 2025

mypy_primer results

No ecosystem changes detected ✅

@sharkdp sharkdp force-pushed the david/dataclasses-pt2 branch from 2c6e0ed to acb0747 Compare April 15, 2025 10:24
@sharkdp sharkdp marked this pull request as ready for review April 15, 2025 10:46
Some(Type::none(db)),
);

return Symbol::bound(Type::Callable(CallableType::new(db, init_signature))).into();
Copy link
Member

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

Copy link
Contributor Author

@sharkdp sharkdp Apr 15, 2025

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.

Copy link
Contributor Author

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.

@sharkdp sharkdp force-pushed the david/dataclasses-pt2 branch from acb0747 to aa6c7c4 Compare April 15, 2025 20:01
Comment on lines +822 to +844
Parameters::new([Parameter::positional_or_keyword(Name::new_static(
"other",
))
Copy link
Contributor Author

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.

@sharkdp
Copy link
Contributor Author

sharkdp commented Apr 15, 2025

I re-ran the job five times and it succeeded every single time. I also couldn't reproduce the panic locally.

image

@sharkdp sharkdp force-pushed the david/dataclasses-pt2 branch from 2a31576 to eedb81a Compare April 16, 2025 20:32
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -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)$' \
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor Author

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...

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?

Copy link
Contributor

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.

@sharkdp sharkdp merged commit b4de245 into main Apr 17, 2025
23 checks passed
@sharkdp sharkdp deleted the david/dataclasses-pt2 branch April 17, 2025 06:58
dcreager added a commit that referenced this pull request Apr 17, 2025
* 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)
dcreager added a commit that referenced this pull request Apr 18, 2025
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants