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

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

Merged
merged 4 commits into from
Aug 11, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Apr 8, 2023

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

@AlexWaygood AlexWaygood added type-feature A feature request or enhancement 3.12 bugs and security fixes labels Apr 8, 2023
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

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 serhiy-storchaka self-requested a review April 10, 2023 08:13
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

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.

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@sobolevn sobolevn added 3.13 bugs and security fixes and removed 3.12 bugs and security fixes labels May 28, 2023
@sobolevn
Copy link
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.

@sobolevn
Copy link
Member Author

@serhiy-storchaka thanks for the review! Addressed.

@serhiy-storchaka
Copy link
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.

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