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

Modify infernce tip for typing.Generic and typing.Annotated with __class_getitem__ #931

Merged
merged 5 commits into from
Apr 10, 2021

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Apr 7, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

With Python 3.7, typing.Generic is now subscriptable through __class_getitem__. Same for typing.Annotated which was added in 3.9. As with the __class_getitem__ methods for the collections.abc classes, astroid needs a bit of help inferring them correctly.

Type of Changes

Type
🐛 Bug fix

Related Issue

This will address: pylint-dev/pylint#2822
I would like to add a test case to pylint before closing it, though.

@cdce8p cdce8p requested a review from hippo91 April 7, 2021 15:49
@cdce8p cdce8p changed the title Modify infernce tip for typing.Generic and typing.Annotated with __class_getitem Modify infernce tip for typing.Generic and typing.Annotated with __class_getitem__ Apr 7, 2021
@cdce8p cdce8p marked this pull request as draft April 7, 2021 16:23
@cdce8p cdce8p marked this pull request as ready for review April 7, 2021 23:12
@cdce8p
Copy link
Member Author

cdce8p commented Apr 7, 2021

During testing, I noticed two related issues with typing.Generic which I've also addressed.

  1. The inferrence_tip for Generic isn't loaded if the inheriting class is inferred, only after the subscript itself is. pylint already handles it. However, astroid uses caching, among other things for slots, which led to an incorrect result. To fix it, I deleted the cached result for slots inside the inferrence_tip.
  2. The MRO for Generic wasn't correct. Generic is handled specially in Python as it's possible to inherit from it multiple times which isn't the case for normal classes. Previously astroid didn't account for that. Consider the following example:
from typing import Generic, TypeVar
T = TypeVar('T')
class A:
    pass
class B(Generic[T]):
    pass
class C(A, B[T], Generic[T]):
    pass

print(C.__mro__)

# The result will be
(<class '__main__.C'>, <class '__main__.A'>, <class '__main__.B'>, <class 'typing.Generic'>, <class 'object'>)

# Previously, astroid would have resolved this to
['.C', '.A', '.B', '.Generic', '.Generic', 'builtins.object']

# Now
['.C', '.A', '.B', 'typing.Generic', 'builtins.object']

@hippo91 hippo91 self-assigned this Apr 8, 2021
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

Great job @cdce8p . I just left a couple of comments. The most important IMHO is the one dealing with clean_typing_generic_mro.

astroid/brain/brain_typing.py Show resolved Hide resolved
Comment on lines 107 to 108
"""typing.Generic is allowed to appear multiple times in the initial mro.
The final one however, MUST only contain ONE.
Copy link
Contributor

Choose a reason for hiding this comment

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

The terms initial and final are a bit confusing. What are their meanings?

part of bases_mro. If true, remove it from inferred_bases
as well as its entry the bases_mro.

Format sequences: [[self]] + bases_mro + [inferred_bases]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it could not be clearer to pass 3 arguments instead of one (i.e self, bases_mro and inferred_bases).
What do you think about it?

"""
pos_generic_in_main_bases = -1
# Check if part of inferred_bases
for i, base in enumerate(sequences[-1]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be definitely clearer to manipulate self (or an alias), bases_mro and inferred bases instead of dealing with only one sequence and multiples indexes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to leave list(clean_duplicates_mro(unmerged_mro, self, context)) in _compute_mro untouched. This should be run before the MRO is cleaned from Generics. However, that also means we only have access to the whole sequence. Creating references inside clean_typing_generic_mro might work though.

@cdce8p
Copy link
Member Author

cdce8p commented Apr 8, 2021

@hippo91 I've pushed a new commit to address your comments.

clean_typing_generic_mro
Unfortunately, I couldn't pass self, bases_mro, and inferred_bases individually, since (as explained earlier) I would like clean_duplicates_mro to run first. It was however possible to define aliases/references inside the function, so it should now be at least a bit easier to understand.

I've also redone the comment. As you said, initial and final weren't as clear as they could have been. initial should have referred to the MRO before the c3_merge and final after.

@cdce8p
Copy link
Member Author

cdce8p commented Apr 10, 2021

@hippo91 Do you had time to take it a short look at the changes yet? I was thinking that we could release a new patch version of astroid after this MR is merged.

/CC: @Pierre-Sassoulas What is your opinion?

@Pierre-Sassoulas
Copy link
Member

Yes, I think we should release as soon as possible because the packaging changes in pylint could cause problems we did not anticipate and the less other bugs we get handle for the next release the better.

Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

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

Thanks @cdce8p ! It is clearer! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants