Skip to content

lookup manager type via mro#2276

Merged
sobolevn merged 3 commits into
typeddjango:masterfrom
asottile:check-managers-along-mro
Jul 25, 2024
Merged

lookup manager type via mro#2276
sobolevn merged 3 commits into
typeddjango:masterfrom
asottile:check-managers-along-mro

Conversation

@asottile

@asottile asottile commented Jul 25, 2024

Copy link
Copy Markdown
Contributor

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)

sobolevn
sobolevn approved these changes Jul 25, 2024
View reviewed changes

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you! It will be in 5.0.3

sobolevn
sobolevn reviewed Jul 25, 2024
View reviewed changes
Comment thread tests/typecheck/models/test_inheritance.yml Outdated
Comment thread tests/typecheck/models/test_inheritance.yml Outdated
@asottile

asottile commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

looks like I have a few things to fix still as well -- I think I need more Self probably :)

@asottile
lookup manager type via mro
c203556 
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)
```
@asottile asottile force-pushed the check-managers-along-mro branch from 2685dd2 to fab8396 Compare July 25, 2024 19:02
@asottile asottile force-pushed the check-managers-along-mro branch from fab8396 to c1a661a Compare July 25, 2024 19:08
asottile
asottile commented Jul 25, 2024
View reviewed changes
Comment thread tests/typecheck/managers/test_managers.yml
myapp/models:23: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.objects" [django-manager-missing]
myapp/models:24: error: Could not resolve manager type for "myapp.models.TwoUnresolvable.second_objects" [django-manager-missing]
myapp/models:27: error: Could not resolve manager type for "myapp.models.AbstractUnresolvable.objects" [django-manager-missing]
myapp/models:32: error: Could not resolve manager type for "myapp.models.InvisibleUnresolvable.objects" [django-manager-missing]

@asottile asottile Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this makes sense to me at least -- we're no longer redefining an objects here because the base class already has an UnknownManager[Self] definition

@sobolevn sobolevn Jul 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @flaeppe

@flaeppe flaeppe Jul 25, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fine IMO. The removal here goes away in my changes in #2252 too

@asottile

asottile commented Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

@sobolevn should be ready for another round of review now \o/

@asottile-sentry asottile-sentry mentioned this pull request Jul 25, 2024
Merged
@sobolevn sobolevn merged commit 2f5a494 into typeddjango:master Jul 25, 2024
@sobolevn

sobolevn commented Jul 25, 2024

Copy link
Copy Markdown
Member

Thanks everyone!

This was referenced Jul 26, 2024
Merged
Merged
Open
@asottile-sentry asottile-sentry mentioned this pull request Jul 29, 2024
Merged
asottile-sentry added a commit to getsentry/sentry that referenced this pull request Jul 29, 2024
@asottile-sentry
ref: get proper base manager typing (#72221)
89697c3 
an absolute ton of work went into enabling this -- but after this PR
model types / create / update / etc. should mostly be typechecked
whereas they were entirely unchecked (everything `Any`) prior to this.

most of the core problem was that mypy and django-stubs did not
understand our custom `BaseManager` and `BaseQuerySet` since there's a
significant amount of django stuff that goes into making those classes
"real"

fix that involved these issues:

- python/mypy#17402 (worked around)
- typeddjango/django-stubs#2228 (the workaround)

with that fixed, it exposed a handful issues in django-stubs itself:

- typeddjango/django-stubs#2240
- typeddjango/django-stubs#2241
- typeddjango/django-stubs#2244
- typeddjango/django-stubs#2248
- typeddjango/django-stubs#2276
- typeddjango/django-stubs#2281

fixing all of those together exposed around 1k issues in sentry code
itself which was fixed over a number of PRs:
- #75186
- #75108
- #75149
- #75162
- #75150
- #75154
- #75158
- #75148
- #75157
- #75156
- #75152
- #75153
- #75151
- #75113
- #75112
- #75111
- #75110
- #75109
- #75013
- #74641
- #74640
- #73783
- #73787
- #73788
- #73793
- #73791
- #73786
- #73761
- #73742
- #73744
- #73602
- #73596
- #73595
- #72892
- #73589
- #73587
- #73581
- #73580
- #73213
- #73207
- #73206
- #73205
- #73202
- #73198
- #73121
- #73109
- #73073
- #73061
- getsentry/getsentry#14370
- #72965
- #72963
- #72967
- #72877 (eventually was able to remove this when upgrading to mypy
1.11)
- #72948
- #72799
- #72898
- #72899
- #72896
- #72895
- #72903
- #72894
- #72897
- #72893
- #72889
- #72887
- #72888
- #72811
- #72872
- #72813
- #72815
- #72812
- #72808
- #72802
- #72807
- #72809
- #72810
- #72814
- #72816
- #72818
- #72806
- #72801
- #72797
- #72800
- #72798
- #72771
- #72718
- #72719
- #72710
- #72709
- #72706
- #72693
- #72641
- #72591
- #72635


mypy 1.11 also included some important improvements with typechecking
`for model_cls in (M1, M2, M2): ...` which also exposed some problems in
our code. unfortunately upgrading mypy involved fixing a lot of things
as well:
- #75020
- #74982
- #74976
- #74974
- #74972
- #74967
- #74954
- getsentry/getsentry#14739
- getsentry/getsentry#14734
- #74959
- #74958
- #74956
- #74953
- #74955
- #74952
- #74895
- #74892
- #74897
- #74896
- #74893
- #74880
- #74900
- #74902
- #74903
- #74904
- #74899
- #74894
- #74882
- #74881
- #74871
- #74870
- #74855
- #74856
- #74853
- #74857
- #74858
- #74807
- #74805
- #74803
- #74806
- #74798
- #74809
- #74801
- #74804
- #74800
- #74799
- #74725
- #74682
- #74677
- #74680
- #74683
- #74681


needless to say this is probably the largest stacked PR I've ever done
-- and I'm pretty happy with how I was able to split this up into
digestable chunks

<!-- Describe your PR here. -->
roaga pushed a commit to getsentry/sentry that referenced this pull request Jul 31, 2024
@asottile-sentry @roaga
ref: get proper base manager typing (#72221)
2553119 
an absolute ton of work went into enabling this -- but after this PR
model types / create / update / etc. should mostly be typechecked
whereas they were entirely unchecked (everything `Any`) prior to this.

most of the core problem was that mypy and django-stubs did not
understand our custom `BaseManager` and `BaseQuerySet` since there's a
significant amount of django stuff that goes into making those classes
"real"

fix that involved these issues:

- python/mypy#17402 (worked around)
- typeddjango/django-stubs#2228 (the workaround)

with that fixed, it exposed a handful issues in django-stubs itself:

- typeddjango/django-stubs#2240
- typeddjango/django-stubs#2241
- typeddjango/django-stubs#2244
- typeddjango/django-stubs#2248
- typeddjango/django-stubs#2276
- typeddjango/django-stubs#2281

fixing all of those together exposed around 1k issues in sentry code
itself which was fixed over a number of PRs:
- #75186
- #75108
- #75149
- #75162
- #75150
- #75154
- #75158
- #75148
- #75157
- #75156
- #75152
- #75153
- #75151
- #75113
- #75112
- #75111
- #75110
- #75109
- #75013
- #74641
- #74640
- #73783
- #73787
- #73788
- #73793
- #73791
- #73786
- #73761
- #73742
- #73744
- #73602
- #73596
- #73595
- #72892
- #73589
- #73587
- #73581
- #73580
- #73213
- #73207
- #73206
- #73205
- #73202
- #73198
- #73121
- #73109
- #73073
- #73061
- getsentry/getsentry#14370
- #72965
- #72963
- #72967
- #72877 (eventually was able to remove this when upgrading to mypy
1.11)
- #72948
- #72799
- #72898
- #72899
- #72896
- #72895
- #72903
- #72894
- #72897
- #72893
- #72889
- #72887
- #72888
- #72811
- #72872
- #72813
- #72815
- #72812
- #72808
- #72802
- #72807
- #72809
- #72810
- #72814
- #72816
- #72818
- #72806
- #72801
- #72797
- #72800
- #72798
- #72771
- #72718
- #72719
- #72710
- #72709
- #72706
- #72693
- #72641
- #72591
- #72635


mypy 1.11 also included some important improvements with typechecking
`for model_cls in (M1, M2, M2): ...` which also exposed some problems in
our code. unfortunately upgrading mypy involved fixing a lot of things
as well:
- #75020
- #74982
- #74976
- #74974
- #74972
- #74967
- #74954
- getsentry/getsentry#14739
- getsentry/getsentry#14734
- #74959
- #74958
- #74956
- #74953
- #74955
- #74952
- #74895
- #74892
- #74897
- #74896
- #74893
- #74880
- #74900
- #74902
- #74903
- #74904
- #74899
- #74894
- #74882
- #74881
- #74871
- #74870
- #74855
- #74856
- #74853
- #74857
- #74858
- #74807
- #74805
- #74803
- #74806
- #74798
- #74809
- #74801
- #74804
- #74800
- #74799
- #74725
- #74682
- #74677
- #74680
- #74683
- #74681


needless to say this is probably the largest stacked PR I've ever done
-- and I'm pretty happy with how I was able to split this up into
digestable chunks

<!-- Describe your PR here. -->
@andersk andersk mentioned this pull request Aug 1, 2024
Closed
tamir-frenkel pushed a commit to tamir-frenkel/sentry that referenced this pull request Nov 30, 2025
@asottile-sentry @tamir-frenkel
ref: get proper base manager typing (getsentry#72221)
4b033a1 
an absolute ton of work went into enabling this -- but after this PR
model types / create / update / etc. should mostly be typechecked
whereas they were entirely unchecked (everything `Any`) prior to this.

most of the core problem was that mypy and django-stubs did not
understand our custom `BaseManager` and `BaseQuerySet` since there's a
significant amount of django stuff that goes into making those classes
"real"

fix that involved these issues:

- python/mypy#17402 (worked around)
- typeddjango/django-stubs#2228 (the workaround)

with that fixed, it exposed a handful issues in django-stubs itself:

- typeddjango/django-stubs#2240
- typeddjango/django-stubs#2241
- typeddjango/django-stubs#2244
- typeddjango/django-stubs#2248
- typeddjango/django-stubs#2276
- typeddjango/django-stubs#2281

fixing all of those together exposed around 1k issues in sentry code
itself which was fixed over a number of PRs:
- getsentry#75186
- getsentry#75108
- getsentry#75149
- getsentry#75162
- getsentry#75150
- getsentry#75154
- getsentry#75158
- getsentry#75148
- getsentry#75157
- getsentry#75156
- getsentry#75152
- getsentry#75153
- getsentry#75151
- getsentry#75113
- getsentry#75112
- getsentry#75111
- getsentry#75110
- getsentry#75109
- getsentry#75013
- getsentry#74641
- getsentry#74640
- getsentry#73783
- getsentry#73787
- getsentry#73788
- getsentry#73793
- getsentry#73791
- getsentry#73786
- getsentry#73761
- getsentry#73742
- getsentry#73744
- getsentry#73602
- getsentry#73596
- getsentry#73595
- getsentry#72892
- getsentry#73589
- getsentry#73587
- getsentry#73581
- getsentry#73580
- getsentry#73213
- getsentry#73207
- getsentry#73206
- getsentry#73205
- getsentry#73202
- getsentry#73198
- getsentry#73121
- getsentry#73109
- getsentry#73073
- getsentry#73061
- getsentry/getsentry#14370
- getsentry#72965
- getsentry#72963
- getsentry#72967
- getsentry#72877 (eventually was able to remove this when upgrading to mypy
1.11)
- getsentry#72948
- getsentry#72799
- getsentry#72898
- getsentry#72899
- getsentry#72896
- getsentry#72895
- getsentry#72903
- getsentry#72894
- getsentry#72897
- getsentry#72893
- getsentry#72889
- getsentry#72887
- getsentry#72888
- getsentry#72811
- getsentry#72872
- getsentry#72813
- getsentry#72815
- getsentry#72812
- getsentry#72808
- getsentry#72802
- getsentry#72807
- getsentry#72809
- getsentry#72810
- getsentry#72814
- getsentry#72816
- getsentry#72818
- getsentry#72806
- getsentry#72801
- getsentry#72797
- getsentry#72800
- getsentry#72798
- getsentry#72771
- getsentry#72718
- getsentry#72719
- getsentry#72710
- getsentry#72709
- getsentry#72706
- getsentry#72693
- getsentry#72641
- getsentry#72591
- getsentry#72635


mypy 1.11 also included some important improvements with typechecking
`for model_cls in (M1, M2, M2): ...` which also exposed some problems in
our code. unfortunately upgrading mypy involved fixing a lot of things
as well:
- getsentry#75020
- getsentry#74982
- getsentry#74976
- getsentry#74974
- getsentry#74972
- getsentry#74967
- getsentry#74954
- getsentry/getsentry#14739
- getsentry/getsentry#14734
- getsentry#74959
- getsentry#74958
- getsentry#74956
- getsentry#74953
- getsentry#74955
- getsentry#74952
- getsentry#74895
- getsentry#74892
- getsentry#74897
- getsentry#74896
- getsentry#74893
- getsentry#74880
- getsentry#74900
- getsentry#74902
- getsentry#74903
- getsentry#74904
- getsentry#74899
- getsentry#74894
- getsentry#74882
- getsentry#74881
- getsentry#74871
- getsentry#74870
- getsentry#74855
- getsentry#74856
- getsentry#74853
- getsentry#74857
- getsentry#74858
- getsentry#74807
- getsentry#74805
- getsentry#74803
- getsentry#74806
- getsentry#74798
- getsentry#74809
- getsentry#74801
- getsentry#74804
- getsentry#74800
- getsentry#74799
- getsentry#74725
- getsentry#74682
- getsentry#74677
- getsentry#74680
- getsentry#74683
- getsentry#74681


needless to say this is probably the largest stacked PR I've ever done
-- and I'm pretty happy with how I was able to split this up into
digestable chunks

<!-- Describe your PR here. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn approved these changes

@flaeppe flaeppe flaeppe approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@asottile @sobolevn @flaeppe