Skip to content
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

General discussion of using @deprecated() (from PEP 702) to signal typing-only deprecations #12092

Open
Sachaa-Thanasius opened this issue Jun 3, 2024 · 7 comments
Labels
project: policy Organization of the typeshed project

Comments

@Sachaa-Thanasius
Copy link
Contributor

Sachaa-Thanasius commented Jun 3, 2024

This is an attempt to centralize discussion about using @(warnings/typing_extensions).deprecated() in typeshed to convey "typing-only" deprecations, not just runtime-related ones. I'm currently trying to use the decorator that way in #12087, and only just now saw it brought up in another PR as well. @AlexWaygood made some good points (imo) about things to consider when using @deprecated() with an (artificial or natural) overload this way, so I figured moving such discussion to its own dedicated thread was well warranted.

I linked messages with points in favor of using @deprecated() this way below (because I'm too lazy right now to rephrase them into clean bullets, but it'll happen at some point):

Then, a few starting questions and concerns quoted from Alex (hope you don't mind):

  1. Are we planning on removing the [deprecated] overload eventually?
  2. If so, how long a deprecation period are we planning on giving?
  3. Should we communicate that deprecation period in the message to the user?
  4. How will we remember to remove the deprecated overload when we reach the end of the deprecation period?
  5. Should we have a policy around this?
  6. Do we need some clear language here to make clear that there's not actually any indication that runtime support for this will ever be removed, it's just a typing-only deprecation?
  7. ... the main point I want us to remember is that deprecations can still be really disruptive, but we don't have a good sense of how disruptive at all in our CI at the moment, because mypy doesn't support @deprecated() yet, so nothing will show up in mypy_primer.

Hope this is useful and not just me being overzealous.

EDIT: Also including a link to a message by @Akuli in one of the comment chains mentioned earlier, which gives an example for making the choice between using deprecated vs. removing a function stub entirely, and what consideration can go into that.

@Sachaa-Thanasius Sachaa-Thanasius changed the title General discussion of @deprecated usage to signal typing-only deprecations General discussion of using @deprecated() (from PEP 702) to signal typing-only deprecations Jun 3, 2024
@AlexWaygood AlexWaygood added the project: policy Organization of the typeshed project label Jun 3, 2024
@mikeshardmind
Copy link
Contributor

mikeshardmind commented Jun 3, 2024

There's an interesting pair of related questions here

When should deprecated be used?

In the linked PR for contextmanager, the deprecated overload is incorrect (but current) and the updated version is correct and would have caught real-world bugs. Treating this as a deprecation means that tools that treat deprecations as a lower severity won't see that this is an actual bug to pass the wrong type of function to the contextmanager decorator. Using deprecated here does allow providing a custom, and likely better error message, but deprecation errors are often seen as a lower severity error to most people, something they may view as "We can fix this years later when we upgrade supported Python versions" and not "this is surfacing a bug". Some people won't see this if they use a configuration that makes those kinds of assumptions about the purpose of deprecation warnings.

This leads to a few possible questions of when we could use it or things that could change to allow it to have dual use without those problems:

Does this mean we shouldn't use deprecated for this? (likely means worse error messages for users)
Does it mean deprecated should have a way to communicate a severity or whether the deprecated behavior is considered type-safe or not? (likely requires a pep if done by adding a kwarg, or a typing council decision if done by a convention of the message instead)
Does this mean that type checkers should treat typing deprecation hits as type errors? (possibly too noisy)


Then some overloads accurately reflect something intended as allowed by runtime, but which will be changed in the future. An example of this might be the change in asyncio.gather no longer wrapping non-futures in a future. It seems clear that it is possible to use deprecated for cases such as this one, and we have enough information to communicate when the behavior will change as part of the message.

How do we handle cases where making the type more accurately descriptive finds issues?

Sorry if this feels like I'm picking on your contextmanager issue, especially since I think providing a useful error message here for users is the better option. It also seems like we could benefit from a clearer policy on when we can fix issues in types, as the prior attempts at this left an incorrect type in place, and the deprecated use appears to be attempting to appease people concerned about this causing churn.

I'm very unwilling to call this "breaks people" unless it's truly a false positive and not a problem in their types interacting with a pre-existing problem in types or a situation where there's a special case that is specified and therefore something people should be able to rely upon. The contextmanager case is not a false positive for those getting an error.

I see more value in types in the typeshed being descriptive, where the runtime behavior of the libraries and standard lib described within to be what is prescriptive, and that so long as a change is being made to put an accurate type (including Any for cases where an accurate type can't be expressed currently, or changing Any to an accurate description when we can), we should prefer the more accurate type, but the lack of a policy here makes this hard to navigate.

@Sachaa-Thanasius
Copy link
Contributor Author

Sachaa-Thanasius commented Jun 3, 2024

To add my own two cents regarding the questions (mostly written before mikeshardmind's response above), here are some ideas, though I'm not solid on all of them:

  1. I'd prefer the policy be "yes, it will be removed unless compelling reason is provided that shows the deprecation period wasn't enough of a buffer for the change". If some policy like this is instituted, I don't think "potential mass CI errors foretold by mypy_primer" would count as compelling, so long as the buffer period is reasonable and there's enough signal spread in other avenues about the deprecation & future behavior.
  2. I'd prefer a time period that's not tied to CPython development, especially since this is typeshed and iteration on the stubs is already (mostly) not constrained that way. This would especially be useful in cases where the change is about making functions more specific and reflective of the runtime without a change in the runtime being an impetus (e.g. @contextlib.(async)contextmanager()). Maybe it could be a few months? A bot or routine could be set up to make an issue every few months as a checkup reminder, and information about this could be added to the contributing guide as something easily found by new willing contributors.
  3. I'm sure avenues for this exist, but beyond putting the time period in the deprecation message, having that message displayed by type checkers, and maybe pinning an issue to collectively add to, edit, and remove deprecations from as they are added and their time passes, I'm not sure what else there. Hopefully someone else has a better idea (update: see mikeshardmind's message above :D)
  4. Could have special comments with dates near the usages so that we can periodically grep for them (and not just all usages of deprecated); could also set up some automation for it to create issues when their time comes to be removed. The latter idea might be too much noise, but I feel like some sort of automation wouldn't hurt here. Having the massive list I mentioned in 3, in a pinned issue with dates, might be easier to scan on occasion and compare to the current date?
  5. Probably. Being completely transparent, I'm hoping this leads to further conversation about typeshed's policy's regarding changes in general (see the message prior, I got ninja'ed), and possibly writing it down so that more changes can be made safely and we can prevent and/or more easily fix situations where something incorrect is forced to be kept around for years.
  6. Would it be too much to ask that uses of deprecation for this purpose have their messages prefixed with "Typing-only deprecation - ..." or something like that? Not sure how else to convey this with the current capability of deprecated. Severity could be suggested via the warning placed in the category kwarg, though that doesn't feel great.
  7. I'm not entirely sure how to address this, but it inspired another question: pyright's current strict configuration in typeshed has reportDeprecation set to "none"; would mypy have its corresponding setting be off by default in mypy_primer and/or in the other CI checks?

EDIT: Note that these were written with typing-only deprecations in mind, not ones based on runtime deprecations.

@Akuli
Copy link
Collaborator

Akuli commented Jun 3, 2024

We have 3 options:

  1. No stub. Users will get errors. Shows up with red squiggles in editors.
  2. Stub with @deprecated. Shows up as overlined in editors. Users will get a warning that will likely fail their CI, but these can be reasonably turned off for the whole project.
  3. Stub without @deprecated. Editor doesn't tell anything to the user. CI is green.

And at least 5 use cases:

  • A feature of runtime will break your app (e.g. tkinter's .after() method freezes the GUI if you call it the wrong way)
  • A feature of runtime works, but it's misleading and there's a better option (includes most "will be removed in version 3.x" deprecations)
  • You just need an error with a custom message
  • Our type stubs are wrong, but changing them would break many CI's
  • When there's nothing inherently wrong with the feature (so you obviously pick option 3)

I think this is difficult to discuss, because there's so many possible "does this option fit this use case" combinations.


That said, here's how I've decided this in the past:

  • Feature of runtime will break your app: no stub
  • Feature of runtime works, but...: @deprecated
  • Need custom message: @deprecated (not ideal, but best for now)
  • Our type stubs are wrong: too complicated and rare for a general policy, IMO better to figure it out case by case

If we go with this approach, here's how I would answer Alex's 7 questions:

  1. For runtime things, remove @deprecated features when the runtime removes them, just like we would do with any other feature. For typing-only things, let's decide case by case.
  2. There is no generally agreed "deprecation period". We either follow the runtime or decide a reasonable period to fit the situation...
  3. ...and write it in the @deprecated message.
  4. We notice that the deprecated thing is gone when our CI fails. If our CI doesn't fail, having an unnecessary deprecated overload isn't the worst thing IMO.
  5. nah
  6. We should write the @deprecated message so that the user can easily understand the situation. This includes making the user understand that it's a typing-only thing. (It's obvious if the message is showing how to update a type annotation.)
  7. I agree.

@JelleZijlstra
Copy link
Member

In an early draft of PEP 702 (which introduced @deprecated) I added a second decorator, @type_error(), that would trigger a type checker error with a custom message, but without a connection to deprecation. That decorator would be a better fit for the contextmanager use case being discussed here.

If people would like a way to get custom error messages without arguably abusing @deprecated, we should write a new PEP adding this @type_error decorator.

@Sachaa-Thanasius
Copy link
Contributor Author

Sachaa-Thanasius commented Jun 3, 2024

That decorator sounds like it would be useful here and in other cases. Any chance you could point me to this version of the pep with @type_error or somewhere with a description of it, @JelleZijlstra? I'd like to learn more, and I might just be missing the obvious, but I can't seem to find it.

@JelleZijlstra
Copy link
Member

I may have actually removed it before the first version of the PEP. I presented this at a typing Zoom meetup at some point: https://docs.google.com/presentation/d/1yzAh1Tok_wIk3lYo3rq1ieQp8MciVzO7pWI_m_LY30Y/edit?usp=sharing. I think the response for @deprecated was more positive than that for @typing_error, so I dropped the latter when I actually wrote the PEP. The slides have a few examples of where @typing_error would be useful.

@srittau
Copy link
Collaborator

srittau commented Jun 4, 2024

Regarding "deprecations", not general type warnings, I'll repeat my points from the other discussion with one addition:


There are multiple reasons why deprecations are better than just removing functions:

  • "X is deprecated, because ..." is a much better error message than "this function does not exist". Especially, since we can't expect users to look into stubs files to find out the reason for the deprecation.
  • Deprecated methods are usually warnings, while non-existing functions are errors – at least in type checkers, linters, IDEs that make this distinction.
  • Disabling deprecation warnings globally is usually a better idea than disabling "unknown function" warnings. This is especially useful when e.g. upgrading to a new Python version and you have many or hard to fix deprecations. (I'm looking at you, badly handled utcnow() deprecation.)

The advantage of using @deprecated (with category=None at runtime) over runtime warnings is that they have much less impact. They are easier to ignore and there's no danger of them unknowingly becoming runtime warnings. As such, I think that we should more liberal in stubs than at runtime with these kinds of warning. Especially in cases like this, where the use of str -> int/str is clearly deprecated in the documentation.


Another advantage over runtime warnings is that @deprecated only checks your own code, and you always have control over them, while runtime warnings are issued "recursively", i.e. maybe a dependency or a dependency of a dependency issues a warnings, which is out of your control. To repeat myself: As such, we should more liberal in stubs than at runtime with deprecation warnings. Edit: Typecheck-level deprecations are low impact compared to other kinds of type errors or runtime warnings.

For other kinds of type errors, Jelle's @type_error sounds reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

No branches or pull requests

6 participants