NotificationsYou must be signed in to change notification settings
Fork3.1k
Star20.1k

[PEP 695] Fix nested generic classes#17776

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JukkaL merged 7 commits intomasterfromgenerics-nested-class

Sep 18, 2024

Merged

[PEP 695] Fix nested generic classes#17776

JukkaL merged 7 commits intomasterfromgenerics-nested-class

Sep 18, 2024

Conversation

Copy link

Collaborator

JukkaL commentedSep 17, 2024

There was confusion about the fullnames of type variables in nested generic classes. A type variable could be defined internally asm.OuterClass.T, but it was sometimes accessed asm.T. The root cause was that the semantic analyzer didn't initialize the attribute that refers to the enclosing class consistently.

Fixes#17596.Fixes#17630.

JukkaL added7 commits

September 16, 2024 14:19

Add failing test case

a249be5

Fix nested class by setting up enclosing class correctly

41c68ce

Add test case

480fa63

Add another test case

b3c5697

Refactor

71aa04e

Add another test case

d2369c4

Add daemon test case

5c4cbd2

Copy link

Contributor

github-actionsbot commentedSep 17, 2024

According tomypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

svalentin approved these changes

Sep 18, 2024

View reviewed changes

Copy link

Collaborator

svalentin left a comment

Choose a reason for hiding this comment

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

lgtm! Just wondering if we need to worry about triple nested classes or some other odd cases.

Copy link

CollaboratorAuthor

JukkaL commentedSep 18, 2024

Just wondering if we need to worry about triple nested classes or some other odd cases.

Multiply nested classes don't work with this PR, sincelookup_fully_qualified_or_none doesn't support them. I'm leaving it out since they are probably quite rare, but I'll create a follow-up issue.