-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Fix the signatures of @(async)contextmanager
#12087
base: main
Are you sure you want to change the base?
Fix the signatures of @(async)contextmanager
#12087
Conversation
… signature on `@contextmanager`.
This comment has been minimized.
This comment has been minimized.
@contextmanager
@(async)contextmanager
This comment has been minimized.
This comment has been minimized.
Whoops. Learned something new about renaming branches today. Sorry about that. |
This comment has been minimized.
This comment has been minimized.
Mypy doesn't yet support |
Huh, didn't know that. I thought typeshed had an unofficial policy to not use typing-related symbols until a majority of the supported type checkers supported them, so I assumed mypy understood it EDIT: I know |
Is there any chance mypy_primer could be run here with a different type checker, e.g. pyright, to gauge the impact (EDIT: maybe once with the type checker-specific deprecation flag on vs. off)? AFAIK, the type checker it runs with can be switched, though I don’t know how much trouble this type of adjustment & run would be. |
We don't use features that major type checkers will error on, but mypy just ignores the |
Ah, gotcha. I guess that last primer run was valid, then. |
I think you'll get exactly the same amount of type-checker screaming. It's just that they'll be screaming about the signature being "deprecated" rather than it being "incorrect". But that still manifests itself as "the type checker is suddenly and unexpectedly failing in CI" in exactly the same way as an error telling you that the signature was incorrect. So I think the amount of disruption that this would cause is exactly the same. For mypy, it would of course cause 0 disruption right now, since mypy doesn't yet support |
(Note: Most of my understanding of how type-checkers would visibly react to the presence of Cheers for the feedback! It made me realize that I made an incorrect assumption and didn't test it; for some reason, I thought the original suggestion this PR stems from — the direct swap to Still, I think there would be a major difference in the potential disruption compared to before: The "deprecated" error would have its own dedicated configurable flag in one's type checker of choice, and that flag isn't fundamental enough to be turned on by default in people's CI systems across all uses of those type checkers. Pyright, for instance, currently has I will admit, I haven't checked what the mypy team's plans are with their own deprecation-related setting, e.g. whether it will be disabled by default; however, if it is off by default, that would also give projects more freedom to make changes according to deprecations like this at their own pace while not breaking CI as much. Basically, I'm trying to posit that if the relevant flag isn't enabled by default across type checkers, then this idea becomes a lot less likely to trigger type checker CI warnings on a large scale than the original annotation swap does, making it better than nothing. |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Akuli <18505570+Akuli@users.noreply.github.com>
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.
This comment has been minimized.
This comment has been minimized.
While I'm in favor of this PR – even if it would cause a huge amount of (easy to fix) warnings – I suggest we wait for the release of a mypy that supports |
I believe @hauntsaninja is planning on releasing 1.13 in the next few days, and it will contain only performance improvements over 1.12, so I expect |
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
@@ -75,6 +75,12 @@ class _GeneratorContextManager(AbstractContextManager[_T_co, bool | None], Conte | |||
self, type: type[BaseException] | None, value: BaseException | None, traceback: TracebackType | None | |||
) -> bool | None: ... | |||
|
|||
@overload | |||
def contextmanager(func: Callable[_P, Generator[_T_co]]) -> Callable[_P, _GeneratorContextManager[_T_co]]: ... |
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.
Perhaps the type here should be Generator[_T_co, None, object]
instead? contextmanager
does not actually care what the return type of the generator is. E.g. this:
@contextmanager
def f():
yield
return 1
is fine (though pointless).
A continuation of the work and discussion in #2772, #2773, #7430, and #8698. Seemingly unblocked by #11867.
This attempts to deprecate the current less restrictive and less accurate signatures of
contextlib.(async)contextmanager
using thedeprecated
decorator to provide users with a transition period (hopefully with less type checker screaming than in previous attempts). The text currently indeprecated()
could be modified to convey a time period as well if that could be agreed upon, e.g. "will be disallowed in (3.14 | 6 months | 1 year)".Additional Rationale
I'm not aware if there're any problems with using
deprecated
in this manner, but if so, this might be as good a place as any to inspire a discussion about them. I'd argue it's not the worst idea, since technically typeshed's signatures for these functions have been inconsistent with the runtime for ages. Thus, even if this usage isn't based on a runtime change as other uses ofdeprecated
seem to be, it can still do what it's meant to: provide some sort of user feedback on a use case being explicitly less accurate or less valid, and potentially mark a predefined time frame for transitioning from one to the other.Considering a major blocker in previous attempts to fix the signatures was the amount of "breakage" it would cause, this feels softer, arguably too soft if maintainers don't have the right flag set in their type checkers to generate a "deprecated" warning error. Still, it's better than before; at least with the right type checker setting on, there's some signal about the more correct signature.
Potential Improvements and/or Alternatives
In the undeprecated overload, we could use custom protocols that aren't as narrow as
(Async)Generator
in place of the current parameter annotations, as outlined here. They would need to support__(a)next__()
,(a)throw()
, and probably now(a)close()
, based on what's used at runtime. Something like this, perhaps, going off of the linked example:We can use
sys.version_info
to mark a clear cutoff point where the signature would no longer be "acceptable". From what I can tell, typeshed doesn't have an explicit deprecation policy for this kind of case, though this might be one way to implement it were it to exist. For example:I feel like this case might be exceptional enough to warrant explicit deprecation cutoff points like this, given the concern about breakage before and how long the incorrect signature has existed without anything like a fix merged (the first issue linked above is over 5 years old now, for reference).
We can perform other actions outside of this PR to signal and help with the transition, e.g. pin an informational issue in the issue tracker commenting on the change, the reasons why, and how to adjust.
Deferred until mypy supports
@deprecated
(with release 1.14?)