-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Allow new-style self-types in classmethods #17381
Conversation
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hm, interestingly the last commit also fixed the primer somehow. Although this may seem good, it is actually suspicious, it may be that we handle access to self-type on unions incorrectly. Let me check this. |
Oh, yup, we incorrectly use |
Diff from mypy_primer, showing the effect of this PR on open source code: pandas (https://github.com/pandas-dev/pandas)
- pandas/core/apply.py:254: error: No overload variant of "__get__" of "AxisProperty" matches argument types "GroupBy[Any]", "type[GroupBy[Any]]" [call-overload]
- pandas/core/apply.py:254: note: Error code "call-overload" not covered by "type: ignore" comment
- pandas/core/apply.py:254: note: Possible overload variants:
- pandas/core/apply.py:254: note: def __get__(self, obj: DataFrame | Series, type: Any) -> Index
- pandas/core/apply.py:254: note: def __get__(self, obj: None, type: Any) -> AxisProperty
- pandas/core/apply.py:254: error: No overload variant of "__get__" of "AxisProperty" matches argument types "SeriesGroupBy", "type[SeriesGroupBy]" [call-overload]
- pandas/core/apply.py:254: error: No overload variant of "__get__" of "AxisProperty" matches argument types "DataFrameGroupBy", "type[DataFrameGroupBy]" [call-overload]
- pandas/core/apply.py:254: error: No overload variant of "__get__" of "AxisProperty" matches argument types "BaseWindow", "type[BaseWindow]" [call-overload]
- pandas/core/apply.py:254: error: No overload variant of "__get__" of "AxisProperty" matches argument types "Resampler", "type[Resampler]" [call-overload]
|
In fairness it looks like this time I contributed to both issues, LOL. In any case I am now happy with |
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.
Nice to see some many bugs fixed!
there's a lot going on in the test but this specifically breaks lookup through `TypeVar` in mypy 1.11 (mypy 1.10 also failed in this way as well -- but was fixed in python/mypy#17381) test originally failing with: ``` /tmp/django-stubs/tests/typecheck/models/test_inheritance.yml:114: E pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: E Actual: E myapp/models:23: error: Incompatible return value type (got "Bound", expected "T") [return-value] (diff) E Expected: E (empty) ```
there's a lot going on in the test but this specifically breaks lookup through `TypeVar` in mypy 1.11 (mypy 1.10 also failed in this way as well -- but was fixed in python/mypy#17381) test originally failing with: ``` /tmp/django-stubs/tests/typecheck/models/test_inheritance.yml:114: E pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: E Actual: E myapp/models:23: error: Incompatible return value type (got "Bound", expected "T") [return-value] (diff) E Expected: E (empty) ```
there's a lot going on in the test but this specifically breaks lookup through `TypeVar` in mypy 1.11 (mypy 1.10 also failed in this way as well -- but was fixed in python/mypy#17381) test originally failing with: ``` /tmp/django-stubs/tests/typecheck/models/test_inheritance.yml:114: E pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: E Actual: E myapp/models:23: error: Incompatible return value type (got "Bound", expected "T") [return-value] (diff) E Expected: E (empty) ```
<!-- Describe your PR here. --> this had three sets of breakage addressed by other PRs: - our foreign key subclass was not functioning, django-stubs added a default TypeVar here which started getting filled in with an unbound TypeVar resulting in thousands of errors: fixed by #75228 - we were able to remove our fork's [descriptor patch](getsentry/sentry-forked-django-stubs#4) (which removed the non-model overload of `__get__` for fields) as mypy [fixed this issue](python/mypy#17381). in doing so it pointed out an unsafe descriptor access through a mixin and so that had to go: #75360 - django-stubs improved some field validation through QuerySets which was only checked through managers before: fixed by #75359
Fixes #16547
Fixes #16410
Fixes #5570
From the upvotes on the issue it looks like an important use case. From what I see this is an omission in the original implementation, I don't see any additional unsafety (except for the same that exists for instance methods/variables). I also incorporate a small refactoring and remove couple unused
get_proper_type()
calls.The fix uncovered an unrelated issue with unions in descriptors, so I fix that one as well.