FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-101162: Forbid using `issubclass` with `GenericAlias` as the 1st arg by sobolevn · Pull Request #103369 · python/cpython · GitHub

/ cpython Public

gh-101162: Forbid using issubclass with GenericAlias as the 1st arg - #103369

Merged
serhiy-storchaka merged 4 commits into
python:mainfrom
sobolevn:issue-101162
Aug 11, 2023
Merged

serhiy-storchaka merged 4 commits into
python:mainfrom
sobolevn:issue-101162

Conversation

sobolevn commented Apr 8, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Now the implementation of types.GenericAlias and typing._GenericAlias is in line for this feature.

Comment thread Lib/test/test_typing.py
AlexWaygood added type-feature A feature request or enhancement 3.12 only security fixes labels Apr 8, 2023

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The change looks good to me. Strictly speaking, it's not backwards-compatible, but, as @serhiy-storchaka mentioned in the issue, it seems unlikely that anybody is passing a GenericAlias object as the first argument of an issubclass() call.

@Yhg1s, is it okay to make this change in 3.12? Or do we need a deprecation warning first?

serhiy-storchaka self-requested a review April 10, 2023 08:13

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

At this point I feel it's too late for 3.12: the feature freeze was extended only for pending PEPs, and this isn't one of them.

I feel we should go through a proper deprecation cycle here instead of just making the change. The behavior isn't completely unreasonable, so people may be relying on it.

Comment thread Lib/test/test_typing.py Outdated
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
sobolevn added 3.13 bugs and security fixes and removed 3.12 only security fixes labels May 28, 2023

Copy link
Copy Markdown
Member Author

At this point I feel it's too late for 3.12: the feature freeze was extended only for pending PEPs, and this isn't one of them.

Yeap, but I guess it is fine for 3.13 :)

I feel we should go through a proper deprecation cycle here instead of just making the change. The behavior isn't completely unreasonable, so people may be relying on it.

Hm, I don't quite agree. I think that this is just a regular bug (I think that it can even be backported). Here are my points:

  1. First of all, typing._GenericAlias never worked this way. It raises:
>>> from typing import List
>>> issubclass(List[int], type)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: issubclass() arg 1 must be a class

So, we don't ever teach users to rely on this historically.

  1. It is not documented, so it is not a promise.
  2. It is not tested. Other interpreters also do not rely on it.
  3. It only works for types using types.GenericAlias which are mostly builtin types. And most builtin types behave like this:
>>> list[int].__bases__
(<class 'object'>,)
>>> issubclass(list[int], object)
True
>>> issubclass(list[int], type)
False

Since most types just inherit from object (there are exceptions like _collections_abc.MappingView, AbstractContextManager, Queue subclasses, and some others).

  1. I don't see realistical use-cases for this. Yes, it might be used. But, I don't think that it should be used in the first place.
  2. Making a deprecation would make this more complicated in terms of code changes.

If you find my arguments not convincing enough, I am always happy to do a deprecation warning.

Comment thread Lib/test/test_typing.py Outdated
Comment thread Lib/test/test_typing.py
Comment thread Lib/test/test_typing.py Outdated
Comment thread Objects/abstract.c

Copy link
Copy Markdown
Member Author

@serhiy-storchaka thanks for the review! Addressed.

Copy link
Copy Markdown
Member

That you for your contribution @sobolevn.

I do not think that a deprecation warning is needed here. This change could even be considered a bugfix and backported to 3.11 and 3.12, there there is no real need in this too.

Blacklisting __bases__ is a good method. I think that it may be worth to blacklist even more special attributes, or forbid them all, and use the white list to allow redirecting some special attributes. But this is a different issue.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.13 bugs and security fixes type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL