-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[mypyc] feat: ForFilter generator helper for builtins.filter
#19643
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
base: master
Are you sure you want to change the base?
Changes from 26 commits
b3f3eab
67818c6
74b7a6e
eeb09ab
ddc13b8
fc12cea
5ce8148
54ad04e
eae9209
9941d54
71b27ef
5237f0b
d68b833
c9680dc
5bf4b22
c39bb4a
8e43b2e
9dceb9a
7c8053f
8aff832
0d2c019
cec1a5d
5170a10
ba5a978
572793c
55ed2d6
dbbbb57
0bc1d26
7d56fa9
4bf480d
11b04c3
fa54df2
d2edf7b
715ce46
a3b65b3
26db7f5
1197e4e
75f61e3
58798da
f18eca3
26ad0d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,3 +54,5 @@ These variants of statements have custom implementations: | |
* ``for ... in seq:`` (for loop over a sequence) | ||
* ``for ... in enumerate(...):`` | ||
* ``for ... in zip(...):`` | ||
* ``for ... in filter(...):`` | ||
* ``for ... in itertools.filterfalse(...):`` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
SetExpr, | ||
TupleExpr, | ||
TypeAlias, | ||
Var, | ||
) | ||
from mypyc.ir.ops import ( | ||
ERR_NEVER, | ||
|
@@ -491,6 +492,16 @@ def make_for_loop_generator( | |
for_list = ForSequence(builder, index, body_block, loop_exit, line, nested) | ||
for_list.init(expr_reg, target_type, reverse=True) | ||
return for_list | ||
|
||
elif ( | ||
expr.callee.fullname == "builtins.filter" | ||
and len(expr.args) == 2 | ||
and all(k == ARG_POS for k in expr.arg_kinds) | ||
): | ||
for_filter = ForFilter(builder, index, body_block, loop_exit, line, nested) | ||
for_filter.init(index, expr.args[0], expr.args[1]) | ||
return for_filter | ||
|
||
if isinstance(expr, CallExpr) and isinstance(expr.callee, MemberExpr) and not expr.args: | ||
# Special cases for dictionary iterator methods, like dict.items(). | ||
rtype = builder.node_type(expr.callee.expr) | ||
|
@@ -1166,3 +1177,72 @@ def gen_step(self) -> None: | |
def gen_cleanup(self) -> None: | ||
for gen in self.gens: | ||
gen.gen_cleanup() | ||
|
||
|
||
class ForFilter(ForGenerator): | ||
"""Generate optimized IR for a for loop over filter(f, iterable).""" | ||
|
||
def need_cleanup(self) -> bool: | ||
# The wrapped for loops might need cleanup. We might generate a | ||
# redundant cleanup block, but that's okay. | ||
return True | ||
|
||
def init(self, index: Lvalue, func: Expression, iterable: Expression) -> None: | ||
self.filter_func_def = func | ||
if ( | ||
isinstance(func, NameExpr) | ||
and isinstance(func.node, Var) | ||
and func.node.fullname == "builtins.None" | ||
): | ||
self.filter_func_val = None | ||
else: | ||
self.filter_func_val = self.builder.accept(func) | ||
self.iterable = iterable | ||
self.index = index | ||
|
||
self.gen = make_for_loop_generator( | ||
self.builder, | ||
self.index, | ||
self.iterable, | ||
self.body_block, | ||
self.loop_exit, | ||
self.line, | ||
is_async=False, | ||
nested=True, | ||
) | ||
|
||
def gen_condition(self) -> None: | ||
self.gen.gen_condition() | ||
|
||
def begin_body(self) -> None: | ||
# 1. Assign the next item to the loop variable | ||
self.gen.begin_body() | ||
|
||
# 2. Call the filter function | ||
builder = self.builder | ||
line = self.line | ||
item = builder.read(builder.get_assignment_target(self.index), line) | ||
|
||
if self.filter_func_val is None: | ||
result = item | ||
else: | ||
fake_call_expr = CallExpr(self.filter_func_def, [self.index], [ARG_POS], [None]) | ||
|
||
# I put this here to prevent a circular import | ||
from mypyc.irbuild.expression import transform_call_expr | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my comment in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting! I commented this out and replaced it with builder.accept, and it broke the IR! Check out the test failures. Once you've seen them I'll revert this to its original state so we're not boxing and unboxing unnecessarily. I'll add a TODO comment to debug the builder.accept issue and then replace this hacky import with builder.accept when its ready There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JukkaL FYI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reverted this to its original state so the tests pass again |
||
|
||
result = transform_call_expr(builder, fake_call_expr) | ||
|
||
# Now, filter: only enter the body if func(item) is truthy | ||
cont_block, rest_block = BasicBlock(), BasicBlock() | ||
builder.add_bool_branch(result, rest_block, cont_block) | ||
builder.activate_block(cont_block) | ||
builder.nonlocal_control[-1].gen_continue(builder, line) | ||
builder.goto_and_activate(rest_block) | ||
# At this point, the rest of the loop body (user code) will be emitted | ||
|
||
def gen_step(self) -> None: | ||
self.gen.gen_step() | ||
|
||
def gen_cleanup(self) -> None: | ||
self.gen.gen_cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered supporting
list(filter(...))
as well -- this seems quite common (in a follow-up PR)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I actually have that drafted already. but it won't be a special case for
list(filter(...))
it will be a special case for[list|tuple|set](some_builtin_we_have_a_helper_for_in_for_helpers(...))
which will account for any builtin we have ForGenerator helpers forUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw this was part of the intent behind the list-built-from-range tests
I wasn't actually testing that we can build a list from a range, I was preparing IR to reflect how this helper would change the C implementation. Will work for map, filter, range, zip, enumerate, and future ops with special-case gen helpers