Skip to content

[mypyc] Implement close method for generators #12042

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

Merged
merged 1 commit into from
Mar 23, 2022

Conversation

MaxShvets
Copy link
Contributor

Description

Closes mypyc/mypyc#790

This PR implements a close method for mypyc generators according to https://www.python.org/dev/peps/pep-0342/#new-generator-method-close

Test Plan

Added tests for the following cases:

  • generator doesn't catch the GeneratorExit
  • generator intercepts the GeneratorExit and raises StopIteration
  • generator ignores GeneratorExit
  • generator intercepts GeneratorExit but raises RuntimeError

@97littleleaf11
Copy link
Collaborator

Thanks for you PR! Could you please add a IRbuild test? You could just create a new irbuild-generators.test file.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

I think it might be a nicer approach to implement a close function in C in exc_ops.c, and then the generated close will just call that. We could probably call the close function out of the vtable, since we can make sure it's always at the same index. That should reduce code size and would make it easier to test the exception using PyErr_ExceptionMatches instead of copying it into sys.exc_info() first, like we do in most generated code.

That said, there's no reason not to merge this as is, because it's definitely an improvement over the status quo.

@97littleleaf11 do you have something particular you're looking for in an IR test? I think the behavioral tests cover this pretty well so I'm happy to not add more IR tests.

@97littleleaf11
Copy link
Collaborator

do you have something particular you're looking for in an IR test?

No. I just didn't found IR test case for generators and thought maybe we should have one. It might be useful if we are going to replace part of IR with C helper functions.

@97littleleaf11
Copy link
Collaborator

@msullivan Shall we merge this PR first and then open a new issue for your suggestion about the C helper function?

@MaxShvets
Copy link
Contributor Author

MaxShvets commented Feb 19, 2022

@97littleleaf11 my understanding is that the IR tests are designed to ensure that the python code generates correct IR. To me, IR tests seem less useful in this case, where we basically handcraft the IR. Please correct me if I'm wrong though.

I'm also not sure if there's an easy way to create an IR test just for the close function since other generator methods will also be included in the output. Or did you mean we should create the test for the whole generator IR?

@97littleleaf11
Copy link
Collaborator

97littleleaf11 commented Feb 19, 2022

To me, IR tests seem less useful in this case, where we basically handcraft the IR.

Yeah, you're right and for current situation it's unnecessary. The IR test is just a double check and someone may optimize it in the future.

Or did you mean we should create the test for the whole generator IR?

Yeah, this is my original idea. I didn't find one for it, or I just miss it?

@MaxShvets
Copy link
Contributor Author

I didn't find one for it, or I just miss it?

There isn't one, as far as I know

@jhance jhance merged commit efe9a31 into python:master Mar 23, 2022
@MaxShvets MaxShvets deleted the generator-close branch March 26, 2022 20:35
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.

Generate proper close() for generators
4 participants