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

[mypyc] Add back __name__ attribute on nested functions #11901

Closed
wants to merge 5 commits into from

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented Jan 5, 2022

Description

Since nested functions are implemented as instances of a special class they don't get a __name__ attribute by default. This commit adds it back by generating a init dunder on callable classes which simply assigns the attribute. This brings mypyc's behaviour more in line with Python's.

Fixes mypyc/mypyc#884

Test Plan

Add asserts to the nestedFunctions run test which checks for the __name__ attribute. Also updated the IR build tests to include the new initializers.

Since nested functions are implemented as instances of a special class
they don't get a __name__ attribute by default. This commit adds it back
by generating a init dunder on callable classes which simply assigns the
attribute. This brings mypyc's behaviour more in line with Python's.
@ichard26 ichard26 marked this pull request as draft January 5, 2022 03:10
builder.add_implicit_return does extra some checks that aren't necessary
for this generated __init__ method (and crash the build).
@ichard26 ichard26 marked this pull request as ready for review January 5, 2022 16:46
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

This seems to work fine, but the changes seem to result in about 160 lines of extra C boilerplate per nested C function. This can be bad, since slow compile times due to the volume of generated C is already a problem. Let's figure out a way to do this without such a big code size explosion.

I think that it would be possible to share the extension class we use to represent nested functions, instead of generating one for each nested function. Not sure if this is tricky for some reason, though. This way we'd only have at most 160 lines of boilerplate per module, which is much more reasonable. Do you want to take a stab at sharing the nested function types? I can give some pointers. (This could be a separate PR that we'd merge first.)

@msullivan Do you remember why we generate a class per nested function? Is it just something that nobody has bothered to optimize yet, or is there some fundamental difficulty?

@ichard26
Copy link
Collaborator Author

Do you want to take a stab at sharing the nested function types?

It'd be nice to try although I'm going to be busy for the next two weeks so I won't be able to look into this until then.

@msullivan
Copy link
Collaborator

I think the main problem is just that we don't really have any shared classes that we use ever. We could do it, but it'll get duplicated between compilation groups (which is not the end of the world).

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.

Compiled nested functions don't retain their __name__ attribute
3 participants