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] Avoid cyclic reference in nested functions #16268

Merged
merged 6 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions mypyc/irbuild/builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,11 @@ def non_function_scope(self) -> bool:
# Currently the stack always has at least two items: dummy and top-level.
return len(self.fn_infos) <= 2

def top_level_fn_info(self) -> FuncInfo | None:
if self.non_function_scope():
return None
return self.fn_infos[2]

def init_final_static(
self,
lvalue: Lvalue,
Expand Down
2 changes: 2 additions & 0 deletions mypyc/irbuild/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ def __init__(
contains_nested: bool = False,
is_decorated: bool = False,
in_non_ext: bool = False,
add_nested_funcs_to_env: bool = False,
) -> None:
self.fitem = fitem
self.name = name
Expand All @@ -47,6 +48,7 @@ def __init__(
self.contains_nested = contains_nested
self.is_decorated = is_decorated
self.in_non_ext = in_non_ext
self.add_nested_funcs_to_env = add_nested_funcs_to_env

# TODO: add field for ret_type: RType = none_rprimitive

Expand Down
2 changes: 1 addition & 1 deletion mypyc/irbuild/env_class.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def load_env_registers(builder: IRBuilder) -> None:
load_outer_envs(builder, fn_info.callable_class)
# If this is a FuncDef, then make sure to load the FuncDef into its own environment
# class so that the function can be called recursively.
if isinstance(fitem, FuncDef):
if isinstance(fitem, FuncDef) and fn_info.add_nested_funcs_to_env:
setup_func_for_recursive_call(builder, fitem, fn_info.callable_class)


Expand Down
43 changes: 33 additions & 10 deletions mypyc/irbuild/function.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
ArgKind,
ClassDef,
Decorator,
FuncBase,
FuncDef,
FuncItem,
LambdaExpr,
Expand Down Expand Up @@ -222,6 +223,7 @@ def c() -> None:
is_decorated = fitem in builder.fdefs_to_decorators
is_singledispatch = fitem in builder.singledispatch_impls
in_non_ext = False
add_nested_funcs_to_env = has_nested_func_self_reference(builder, fitem)
class_name = None
if cdef:
ir = builder.mapper.type_to_ir[cdef.info]
Expand All @@ -234,14 +236,15 @@ def c() -> None:
func_name = name
builder.enter(
FuncInfo(
fitem,
func_name,
class_name,
gen_func_ns(builder),
is_nested,
contains_nested,
is_decorated,
in_non_ext,
fitem=fitem,
name=func_name,
class_name=class_name,
namespace=gen_func_ns(builder),
is_nested=is_nested,
contains_nested=contains_nested,
is_decorated=is_decorated,
in_non_ext=in_non_ext,
add_nested_funcs_to_env=add_nested_funcs_to_env,
)
)

Expand All @@ -267,7 +270,13 @@ def c() -> None:
builder.enter(fn_info)
setup_env_for_generator_class(builder)
load_outer_envs(builder, builder.fn_info.generator_class)
if builder.fn_info.is_nested and isinstance(fitem, FuncDef):
top_level = builder.top_level_fn_info()
if (
builder.fn_info.is_nested
and isinstance(fitem, FuncDef)
and top_level
and top_level.add_nested_funcs_to_env
):
setup_func_for_recursive_call(builder, fitem, builder.fn_info.generator_class)
create_switch_for_generator_class(builder)
add_raise_exception_blocks_to_generator_class(builder, fitem.line)
Expand Down Expand Up @@ -344,6 +353,20 @@ def c() -> None:
return func_ir, func_reg


def has_nested_func_self_reference(builder: IRBuilder, fitem: FuncItem) -> bool:
"""Does a nested function contain a self-reference in its body?

If a nested function only has references in the surrounding function,
we don't need to add it to the environment.
"""
if any(isinstance(sym, FuncBase) for sym in builder.free_variables.get(fitem, set())):
return True
return any(
has_nested_func_self_reference(builder, nested)
for nested in builder.encapsulating_funcs.get(fitem, [])
)
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be worth testing mutually recursive functions, something involving:

def f():
    def g():
        h()
    def h():
        g()
    return g, h

or are we already testing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we already have at least the test case testNestedFunctions2 in mypyc/test-data/run-functions.test (mutual_recursion).



def gen_func_ir(
builder: IRBuilder,
args: list[Register],
Expand Down Expand Up @@ -768,7 +791,7 @@ def get_func_target(builder: IRBuilder, fdef: FuncDef) -> AssignmentTarget:
# Get the target associated with the previously defined FuncDef.
return builder.lookup(fdef.original_def)

if builder.fn_info.is_generator or builder.fn_info.contains_nested:
if builder.fn_info.is_generator or builder.fn_info.add_nested_funcs_to_env:
return builder.lookup(fdef)

return builder.add_local_reg(fdef, object_rprimitive)
Expand Down
Loading